Normalize the requested version in NuGet package lookups

José M. Requena Plensrequested to merge
gitlab-community/gitlab-org/gitlab:jmrp-nuget-normalized-duplicates into master

What does this MR do and why?

Packages::Nuget::PackageFinder matches a version against the package version or the stored normalized version, but Metadatum.normalized_version_in only downcases the requested one. The duplicate check of both NuGet upload services passes the .nuspec version, so a push of 3.0.0.0, 3.0 or 3.0.0+meta after 3.0.0 finds no package: it is accepted where duplicates are not allowed, and creates a second package for the same NuGet version.

This MR:

  1. Normalizes a requested version that is a valid NuGet version without a prerelease label with the existing algorithm, now a class method of VersionNormalizable; anything else is only downcased, as before, because the algorithm reads the dots and leading zeros of a prerelease label as version parts (1.0-beta and 1.0-beta.0 both become 1.0-beta.0).
  2. In both services, names a file added to a package of an equivalent version after that package's version, through Packages::Nuget::Package#nuget_file_name. Otherwise the newest file is never served, since the download endpoint (format_filename) builds the file name from the found package's version. A prerelease push keeps its .nuspec version in the file name, as on master: the normalized version stored for 1.0-rc.1.0 is 1.0-rc.1, so a push of 1.0-rc.1 still finds that package, and a file named after 1.0-rc.1.0 would be served for that version.
  3. In the download endpoint (format_filename), compares the stored normalized version with the requested version as the lookup normalizes it, not as written, so a download of 3.0.0.0 or 3.0.0.00 that finds the 3.0.0 package serves its newest file, as the metadata and delete endpoints already find that package.
  4. Where equivalent versions are stored as separate packages, makes the finder return the one whose version is the requested one first, then the newest, so that a download or DELETE of 3.0.0.0 acts on the 3.0.0.0 package and not on a newer 3.0 one. A normalized release version, which NuGet clients request, keeps the newest first, as on master. Build metadata does not change a NuGet version, so a package whose version only adds it, such as 2.0.0-alpha+build for 2.0.0-alpha, counts as one with the requested version.
  5. Keys the lease of CreateOrUpdatePackageService on the version the duplicate check matches, so that pushes of 1.0.0 and 1.0.0.0 at the same time take the same lease. A prerelease version and the same version with build metadata still take two leases, as on master.

Behavior changes:

  • Where duplicates are allowed (the default), a push of 3.0.0.0 after 3.0.0 adds a file to 3.0.0 instead of a new package: a download of 3.0.0 or 3.0.0.0 returns it, and the cleanup policy counts both files as duplicates.
  • Clients that send an X-NuGet-Client-Version below 3 keep their exact-only lookup. After that push there is no 3.0.0.0 package, so their metadata and download requests for 3.0.0.0 answer 404 (master: 200, from the separate package), and so does the download link the v2 OData feed builds from the requested version; 3.0.0 serves them the pushed file. I kept their lookup exact: normalizing it would also change which package their DELETE marks.
  • The metadata, download and delete endpoints share the finder: a metadata, download or DELETE request for 3.0.0.0 or 3.0.0.00 now finds 3.0.0 when there is no package with that version (master: 404). DELETE then marks 3.0.0 for destruction, which cannot be undone. Master already does this for a normalized version in the path: DELETE of 3.0.0 marks a stored 3.0.0.0. Downloads by NuGet clients, which use normalized versions, are unchanged, also where the bug already stored 3.0.0 and a newer 3.0.0.0 as separate packages: a download or DELETE of 3.0.0 acts on the newest, as on master. The same holds for 2.0.0-alpha and a newer 2.0.0-alpha+build, which a push still stores as two packages.
  • nuget_duplicate_exception_regex is still matched against the existing package the finder returns, which can now be an equivalent version. With duplicates not allowed, a regex 1\.0\.0\.0 that let a 1.0.0.0 push through after 1.0.0 now answers 409 Conflict, because the existing package is 1.0.0. Master answers the same when the two pushes come in the other order.
  • A symbol package whose .nuspec writes 3.0.0.0 is linked to 3.0.0 instead of failing as invalid.

No feature flag: this is a bug fix, the query plan keeps its shape, and the DELETE and exception regex changes are what master already does when the two spellings come in the other order. If you would rather avoid them, I can use the normalized lookup only to refuse a push. No new API parameter or field, and no GraphQL or schema change.

What I ran

On master 4a59659e, with the changed files under app/ from master and the specs of the first commit, the five unit spec files gave 228 examples, 22 failures, all of them new rows or examples, and the first request context 2 examples, 2 failures.

For the review fixes, with the files under app/ as the second commit leaves them, the changed unit spec files gave 259 examples, 22 failures, and the two new request contexts 7 examples, 2 failures. Twelve of the unit failures are the two new methods; the others are the prerelease pushes that find the other package (two in each service), the three lease rows, the finder order (three targets), and the download and DELETE of 3.0.0.0. On this branch: no failures.

For the newest-first rule, with the finder as the third commit leaves it, the finder spec gave 54 examples, 3 failures, and the two new request contexts 6 examples, 6 failures: a download and a DELETE of 1.0.0 acted on the older 1.0.0 package, not on a newer 1.0.0.0, 1.0 or 1.0.0+meta. On this branch: no failures.

For the build metadata rule, with package.rb as the fourth commit leaves it, the model spec gave 35 examples, 2 failures, the finder spec 60 examples, 3 failures, and the new request context 1 example, 1 failure: a download of 2.0.0-alpha returned the older 2.0.0-alpha file, not the newer 2.0.0-alpha+build one. The rows that keep the exact package first for 1.0.0-beta.7abc with a newer 1.0.0-beta.007abc, and for a request that carries build metadata, pass with and without the change. On this branch: no failures.

For the download file name, with lib/api/nuget_project_packages.rb from master and without .to_s in normalized_lookup_version, the push round trip, the 3.0.0.00 download and the new service example gave 5 examples, 3 failures: a download of 1.2.3.0 after a push of 1.2.3.0 onto 1.2.3, and one of 3.0.0.00, answered 404, and a push whose .nuspec has no version raised NoMethodError in the lease key instead of answering 400. This round also cut request and service examples that repeated the finder and model specs, so some examples named above are gone; their rows are in package_finder_spec.rb and package_spec.rb. On this branch: no failures.

For the prerelease file name, with both services passing target_package.version to nuget_file_name instead of the pushed version, the two service spec files gave 101 examples, 2 failures: the two new examples, where a push of 1.0-rc.1 onto 1.0-rc.1.0 named its file after 1.0-rc.1.0. With master's file name in both services instead, the same 101 examples gave 2 failures, the release examples. On this branch: no failures.

Also on this branch: the six spec files it changes, 1221 examples, 0 failures (Ruby 3.3.11). RuboCop on the 13 files: no offenses. The commit linter: no problems.

Database query and plan

The scope keeps its query, and only the value compared with packages_nuget_metadata.normalized_version changes. With a version that is not a normalized release version, the finder also orders first the package with that version, or with that version and build metadata. The duplicate check of a push of 3.0.0.0 in a project runs the query below. It is the finder's to_sql, printed in a spec, with LIMIT 1 in place of the finder's LIMIT 300, because every caller with a version (both upload services and the API helper's find_package) takes .first:

SELECT "packages_packages".* FROM "packages_packages"
LEFT OUTER JOIN "packages_nuget_metadata" ON "packages_nuget_metadata"."package_id" = "packages_packages"."id"
WHERE "packages_packages"."package_type" = 4 AND "packages_packages"."project_id" = 1722
  AND "packages_packages"."status" IN (0, 1) AND "packages_packages"."version" IS NOT NULL
  AND LOWER("packages_packages"."name") = 'probe.package'
  AND ((LOWER(version) = '3.0.0.0') OR "packages_nuget_metadata"."normalized_version" = '3.0.0')
ORDER BY CASE WHEN (LOWER("packages_packages"."version") = '3.0.0.0' OR "packages_packages"."version" ILIKE '3.0.0.0+%') THEN 0 ELSE 1 END,
  "packages_packages"."created_at" DESC
LIMIT 1
-- master: ... OR "packages_nuget_metadata"."normalized_version" = '3.0.0.0') ORDER BY "packages_packages"."created_at" DESC LIMIT 1

The group endpoints run the same query with "packages_packages"."project_id" IN (SELECT "projects"."id" ...), the projects the user can read in the group, in place of "project_id" = 1722.

Plans mkhalifa3 ran on Database Lab (gitlab-production-main), master against this branch: the plan shape is the same as master in every case, timings and buffers are the same within noise, and since master already sorts because of the OR filter, the CASE key only adds a sort key over at most a few rows. A request for a normalized version, as in the first two rows, now runs the same query as master, without the CASE key. These plans predate the build metadata rule, which adds an ILIKE to the CASE key; the local plan below, run again with it, keeps its shape and buffers.

Request Scope Master This branch
Normalized version (NuGet client download) Project 64.8 ms, 31,543 buffers 74.9 ms, 31,546 buffers
Normalized version Group 93.1 ms, 39,069 buffers 84.9 ms, 39,087 buffers
.0 spelling (duplicate push) Project 66.0 ms, 31,543 buffers, 0 rows 64.1 ms, 31,546 buffers, 1 row
.0 spelling Group 86.3 ms, 39,054 buffers, 0 rows 104.3 ms, 39,087 buffers, 1 row
New version (first push) Project 61.1 ms, 31,543 buffers 62.9 ms, 31,546 buffers
New version Group 88.8 ms, 39,054 buffers 86.1 ms, 39,072 buffers
Setup and notes

  • Target: the NuGet package name with the most versions on GitLab.com (7,624 versions), in a private project two levels deep in its group. The group runs use a real reporter of that project, for the permission subquery.

  • SQL: generated with PackageFinder#execute.limit(1).to_sql on GDK, then the production IDs and values were put in. Every caller with a version takes .first, so LIMIT 1.

  • Order of runs: master, then branch, then master again. The table gives the second master run, so a warm cache does not favor the branch. Both plans use idx_pkgs_project_id_lower_name_when_nuget_installable_version, then a nested loop over idx_packages_nuget_metadata_on_pkg_id_and_normalized_version, then a Sort and a Limit.

  • Existing cost, also on master: with a cold cache, the first run took 14.2 s (project) and 6.0 s (group). The query reads every version of the name because the OR filter blocks early stopping. This MR does not change that.

  • Six of these runs return 0 rows on purpose, so the console shows its "Query returned 0 rows" warning on them. The warning does not apply here.

    • New version, master and branch: a first push checks a version that does not exist yet, so 0 rows is the real production case.
    • .0 spelling, master: finding nothing is the bug this MR fixes. The branch run of the same request returns 1 row.

    The 0-row runs read the same buffers as the 1-row runs (project: 31,543 against 31,543 to 31,546; group: 39,054 against 39,069 to 39,087). The plan cannot stop early: it filters every version of the name, then sorts. So 0 rows is the worst case for this query, not a cheaper one.

Before those, I ran this plan locally, not on GitLab.com data: a scratch PostgreSQL 17.11 database with the two tables and every index db/structure.sql gives them, seeded with 405,020 NuGet packages, 385,020 metadata rows and 200,000 Maven and npm packages. It runs the query above for a name with 5,000 versions where nothing matches (project 1000001, popular.package). Run again for this round's query, which drops LOWER before the ILIKE: the same query ordered only by created_at, as master orders it, and the query before the build metadata rule get the same plan, and the scan under the sort reads the same 15,073 buffers in all three: the sort only sees the rows that pass the filter. For a name with 20 versions, among them 3.0.0.0 and a newer 3.0, the plan is the same, and it returns 3.0.0.0 in 0.5 ms. The timings are only indicative.

 Limit (actual time=32.064..32.066 rows=0 loops=1)
   Buffers: shared hit=15079
   ->  Sort (actual time=32.062..32.064 rows=0 loops=1)
         Sort Key: (CASE WHEN ((lower((packages_packages.version)::text) = '3.0.0.0'::text) OR ((packages_packages.version)::text ~~* '3.0.0.0+%'::text)) THEN 0 ELSE 1 END), packages_packages.created_at DESC
         Sort Method: quicksort  Memory: 25kB
         Buffers: shared hit=15079
         ->  Nested Loop Left Join (actual time=31.920..31.922 rows=0 loops=1)
               Filter: ((lower((packages_packages.version)::text) = '3.0.0.0'::text) OR (packages_nuget_metadata.normalized_version = '3.0.0'::text))
               Rows Removed by Filter: 5000
               Buffers: shared hit=15073
               ->  Bitmap Heap Scan on packages_packages (actual time=3.197..4.627 rows=5000 loops=1)
                     Recheck Cond: ((project_id = 1000001) AND (lower((name)::text) = 'popular.package'::text) AND (package_type = 4) AND (version IS NOT NULL) AND (status = ANY ('{0,1}'::integer[])))
                     Heap Blocks: exact=62
                     Buffers: shared hit=72
                     ->  Bitmap Index Scan on idx_pkgs_project_id_lower_name_when_nuget_installable_version (actual time=3.165..3.166 rows=5000 loops=1)
                           Index Cond: ((project_id = 1000001) AND (lower((name)::text) = 'popular.package'::text))
                           Buffers: shared hit=10
               ->  Index Only Scan using idx_packages_nuget_metadata_on_pkg_id_and_normalized_version on packages_nuget_metadata (actual time=0.004..0.004 rows=1 loops=5000)
                     Index Cond: (package_id = packages_packages.id)
                     Heap Fetches: 0
                     Buffers: shared hit=15001
 Planning:
   Buffers: shared hit=855
 Planning Time: 9.499 ms
 Execution Time: 32.331 ms

I will ask for the database review after the backend review.

References

Closes #636767

  • !121260 (merged) added the stored normalized version and the finder's match against it.
  • The normalized version stored for a prerelease version still reads the dots and leading zeros of its label as version parts. Fixing it needs a backfill of normalized_version, so it is left for #637536, which also covers the build metadata of a requested prerelease version in the lookup, the file name and the lease key.
  • The lease of UpdatePackageFromMetadataService is keyed on a temporary package when no package exists yet (see #635559).

I found this while working on gitlab-mcp-server.

Screenshots or screen recordings

Not applicable: backend change with no UI.

How to set up and validate locally

  1. In a group with Allow duplicates off for NuGet and no exceptions, push a package to a project as 3.0.0, then as 3.0.0.0: master answers 201 Created, this branch 409 Conflict.
  2. In a group that allows duplicates, push 3.0.0.0, then 3.0.0 with another description, and download 3.0.0: master returns the first push, this branch the second.

MR acceptance checklist

Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.

Edited by José M. Requena Plens

Merge request reports