From 7ad14d994889e9bcc5bc624a0b952b84635e1205 Mon Sep 17 00:00:00 2001 From: krynju Date: Fri, 9 Oct 2026 16:25:26 +0200 Subject: [PATCH] fix: send package-search sorts the API accepts; download a small dataset in e2e `jh package search` sent `sorts[1]=name` as its tiebreak. The packages API requires every sort key to carry a direction sign (`^[+-].+$`) and rejects the unsigned key with a 400. Send `-name`: any fixed direction keeps paging stable, and the signed form is accepted. TestDatasetDownloadFirst downloaded the first Blob listed, which on a shared instance can be a large blob left by another suite and outlast the command timeout. It now downloads the smallest uploaded Blob whose listed size is at most 20 MiB, and skips with a clear message when there is none. Co-Authored-By: Claude Opus 5.5 --- e2e/dataset_test.go | 14 +++++-- e2e/helpers_test.go | 94 +++++++++++++++++++++++++++++++++++++++++++-- packages.go | 6 ++- packages_test.go | 21 +++++++++- 4 files changed, 125 insertions(+), 10 deletions(-) diff --git a/e2e/dataset_test.go b/e2e/dataset_test.go index 1246f67..af03a48 100644 --- a/e2e/dataset_test.go +++ b/e2e/dataset_test.go @@ -50,15 +50,21 @@ func TestDatasetStatusFirst(t *testing.T) { } } -// TestDatasetDownloadFirst lists datasets, takes the first Blob-type one -// with an uploaded version, downloads it to a temp path, and asserts the file was written and is +// downloadSizeCap bounds the dataset TestDatasetDownloadFirst will fetch. +// Shared instances keep large blobs left over by other suites, and the first +// one listed can take longer than the command timeout to download. +const downloadSizeCap = 20 << 20 // 20 MiB + +// TestDatasetDownloadFirst lists datasets, takes the smallest Blob-type one +// with an uploaded version and a listed size within downloadSizeCap, +// downloads it to a temp path, and asserts the file was written and is // non-empty. func TestDatasetDownloadFirst(t *testing.T) { requireCreds(t) list := runOK(t, "dataset", "list").combined() - id := firstIDOfType(list, "Blob") + id := smallestIDOfType(list, "Blob", downloadSizeCap) if id == "" { - t.Skip("no Blob-type datasets with an uploaded version on this instance to download") + t.Skipf("no Blob-type datasets with an uploaded version of at most %d bytes on this instance to download", downloadSizeCap) } dest := filepath.Join(t.TempDir(), "dataset.bin") diff --git a/e2e/helpers_test.go b/e2e/helpers_test.go index 2e6ec9c..90f10e9 100644 --- a/e2e/helpers_test.go +++ b/e2e/helpers_test.go @@ -5,6 +5,7 @@ package e2e import ( "fmt" "regexp" + "strconv" "strings" "testing" ) @@ -84,8 +85,42 @@ func firstID(out string) string { // has no version to inspect or download, so status/download fail on it by // design and it cannot drive a happy-path test. func firstIDOfType(out, dtype string) string { + if entries := datasetEntriesOfType(out, dtype); len(entries) > 0 { + return entries[0].id + } + return "" +} + +// smallestIDOfType is firstIDOfType restricted to entries whose listed size is +// known and in (0, maxBytes], returning the smallest. Download tests use it so +// they never pull whatever large blob another suite left behind on a shared +// instance. +func smallestIDOfType(out, dtype string, maxBytes int64) string { + best, bestSize := "", int64(-1) + for _, e := range datasetEntriesOfType(out, dtype) { + if e.size <= 0 || e.size > maxBytes { + continue + } + if bestSize < 0 || e.size < bestSize { + best, bestSize = e.id, e.size + } + } + return best +} + +type datasetEntry struct { + id string + size int64 // -1 when the entry has no "Size:" line +} + +var reSizeLine = regexp.MustCompile(`(?m)^Size:\s*(\d+)\s*bytes\s*$`) + +// datasetEntriesOfType returns, in listing order, the `dataset list` entries of +// type dtype with at least one uploaded version. +func datasetEntriesOfType(out, dtype string) []datasetEntry { typeRe := regexp.MustCompile(`(?m)^Type:\s*` + regexp.QuoteMeta(dtype) + `\s*$`) verRe := regexp.MustCompile(`(?m)^Version:\s*v[1-9][0-9]*\s*$`) + var entries []datasetEntry ids := reIDLine.FindAllStringSubmatchIndex(out, -1) for i, loc := range ids { end := len(out) @@ -93,11 +128,18 @@ func firstIDOfType(out, dtype string) string { end = ids[i+1][0] } entry := out[loc[0]:end] - if typeRe.MatchString(entry) && verRe.MatchString(entry) { - return out[loc[2]:loc[3]] + if !typeRe.MatchString(entry) || !verRe.MatchString(entry) { + continue + } + size := int64(-1) + if m := reSizeLine.FindStringSubmatch(entry); len(m) == 2 { + if n, err := strconv.ParseInt(m[1], 10, 64); err == nil { + size = n + } } + entries = append(entries, datasetEntry{id: out[loc[2]:loc[3]], size: size}) } - return "" + return entries } // TestFirstIDOfType pins the listing-parse behaviour firstIDOfType relies on: @@ -144,6 +186,52 @@ func TestFirstIDOfType(t *testing.T) { } } +// TestSmallestIDOfType pins the download tests' dataset choice: the smallest +// uploaded Blob within the cap, never one over it, one of unknown size, or an +// empty one. Pure output parsing. +func TestSmallestIDOfType(t *testing.T) { + listing := "Found 5 dataset(s):\n" + + "\n" + + "ID: 1aaaaaaa-0000-0000-0000-000000000001\n" + + "Name: large_blob\n" + + "Size: 1073741824 bytes\n" + + "Type: Blob\n" + + "Version: v1\n" + + "\n" + + "ID: 1aaaaaaa-0000-0000-0000-000000000002\n" + + "Name: medium\n" + + "Size: 4096 bytes\n" + + "Type: Blob\n" + + "Version: v3\n" + + "\n" + + "ID: 1aaaaaaa-0000-0000-0000-000000000003\n" + + "Name: tiny_tree\n" + + "Size: 10 bytes\n" + + "Type: BlobTree\n" + + "Version: v1\n" + + "\n" + + "ID: 1aaaaaaa-0000-0000-0000-000000000004\n" + + "Name: small\n" + + "Size: 51 bytes\n" + + "Type: Blob\n" + + "Version: v1\n" + + "\n" + + "ID: 1aaaaaaa-0000-0000-0000-000000000005\n" + + "Name: no_size_line\n" + + "Type: Blob\n" + + "Version: v1\n" + + if got := smallestIDOfType(listing, "Blob", 10<<20); got != "1aaaaaaa-0000-0000-0000-000000000004" { + t.Errorf("smallestIDOfType(Blob) = %q, want the 51-byte Blob", got) + } + if got := smallestIDOfType(listing, "Blob", 50); got != "" { + t.Errorf("smallestIDOfType(Blob, cap 50) = %q, want empty (every Blob is over the cap or unsized)", got) + } + if got := firstIDOfType(listing, "Blob"); got != "1aaaaaaa-0000-0000-0000-000000000001" { + t.Errorf("firstIDOfType(Blob) = %q, want the first Blob regardless of size", got) + } +} + // firstField returns the value of the first ": " line (case-insensitive). func firstField(out, field string) string { re := regexp.MustCompile(`(?mi)^` + regexp.QuoteMeta(field) + `:\s*(.+)$`) diff --git a/packages.go b/packages.go index 7dc1c5c..c1f1363 100644 --- a/packages.go +++ b/packages.go @@ -341,6 +341,10 @@ func planRESTPages(limit, offset int) (pages []restPage, skip int) { // stars (the server's default). Name breaks ties: packages of one repository // share a score, and without a tiebreak their order changes between requests, // so consecutive pages could repeat or skip rows. +// +// Every sort key carries a direction sign: the API rejects unsigned keys +// (`^[+-].+$`), and servers only honour the `-` form, so the tiebreak is +// descending. Any fixed direction keeps the paging stable. func buildPackagesInfoQuery(search string, registryNames []string, page restPage) url.Values { q := url.Values{} q.Set("sorts[0]", "-stargazers_count") @@ -348,7 +352,7 @@ func buildPackagesInfoQuery(search string, registryNames []string, page restPage q.Set("name", search) q.Set("sorts[0]", "-score") } - q.Set("sorts[1]", "name") + q.Set("sorts[1]", "-name") if len(registryNames) > 0 { q.Set("registries", strings.Join(registryNames, ",")) } diff --git a/packages_test.go b/packages_test.go index b16b29b..1ac45aa 100644 --- a/packages_test.go +++ b/packages_test.go @@ -6,7 +6,9 @@ import ( "net/http" "net/http/httptest" "reflect" + "regexp" "strconv" + "strings" "testing" ) @@ -129,15 +131,30 @@ func TestPlanRESTPages(t *testing.T) { func TestBuildPackagesInfoQuery(t *testing.T) { q := buildPackagesInfoQuery("plots", []string{"General", "MyReg"}, restPage{Page: 2, PerPage: 25}) - want := "name=plots&pagination%5Bpage%5D=2&pagination%5Bper_page%5D=25&pagination%5Btype%5D=offset®istries=General%2CMyReg&sorts%5B0%5D=-score&sorts%5B1%5D=name" + want := "name=plots&pagination%5Bpage%5D=2&pagination%5Bper_page%5D=25&pagination%5Btype%5D=offset®istries=General%2CMyReg&sorts%5B0%5D=-score&sorts%5B1%5D=-name" if got := q.Encode(); got != want { t.Errorf("query = %s\nwant %s", got, want) } // Without a search term: most-starred first, still with the name tiebreak. q = buildPackagesInfoQuery("", nil, restPage{1, 10}) - if q.Has("name") || q.Get("sorts[0]") != "-stargazers_count" || q.Get("sorts[1]") != "name" { + if q.Has("name") || q.Get("sorts[0]") != "-stargazers_count" || q.Get("sorts[1]") != "-name" { t.Errorf("unexpected params without a search term: %s", q.Encode()) } + // The API's Sorts schema requires a direction sign on every key. + signed := regexp.MustCompile(`^[+-].+$`) + for _, search := range []string{"plots", ""} { + q = buildPackagesInfoQuery(search, nil, restPage{1, 10}) + for key, vals := range q { + if !strings.HasPrefix(key, "sorts[") { + continue + } + for _, v := range vals { + if !signed.MatchString(v) { + t.Errorf("search %q: %s=%q does not match the API's sort pattern ^[+-].+$", search, key, v) + } + } + } + } } func TestFetchRESTPackagesFromWindow(t *testing.T) {