chore(datastore): npm packages and versions management reads

Hayley Swimelarrequested to merge
hswimelar/s17-phase3-step-8-npm-packages-versions-reads into main

Why

Step 11 and Step 12 of the Phase 3 plan serve the npm package, version, and file endpoints, and no npm store can answer them today:

  • npm packages have no list method at all.
  • NpmVersionsByPackage carries the packument generator's fixed (created_at, id) order, its own after-cursor pair, and package_json. The management list needs a selectable sort, hasMore, and none of the jsonb.
  • NpmPackageByID is keyed by (namespace_id, id) alone, so it would return a package the repository in the URL does not contain. Detail reads on the management surface have to verify the parent chain.

This step adds the four reads those two handler steps consume. Nothing is wired into a route: Steps 11 and 12 own that.

What is worth a second look

The version list is a single-table scan on purpose. It scopes on (namespace_id, npm_package_id) and does not join npm_packages, so it does not check the parent's repository or soft-delete state. The spec's resolution flow already requires the handler to read the package detail first, to tell 404 from an empty 200, and FindNpmPackageInRepository is that read. Joining again per row would buy a check the request already performed. ListNpmVersionsByPackage's doc states the obligation so a handler author cannot skip it.

The version reads do not project package_json. No management response renders it and it is up to 20000 bytes per row, so a 100-row page would otherwise pull 2 MB of jsonb the handler discards. npmVersionPageForFileCascade is the existing precedent for a partial projection into model.NpmVersions.

The version detail read is two primary-key seeks, not one statement. Relating npm_versions to npm_packages in one query leaves namespace_id as the only constant Postgres can push into the package side, because the package id lives in the version row the query has not read yet. On a 5000-package fixture that planned a merge join estimating 5002 package rows for a one-row answer, and the cost grows with the namespace. Writing the parent check as a correlated EXISTS did not help, because the planner unnests it into the same join. The two seeks cost 3 buffer hits each and do not grow. The Database Review Evidence below has both plans.

Both version sorts and the package sort ride existing indexes, so this step adds no migration and does not depend on Step 2. index_npm_versions_on_ns_id_pkg_id_created_at_id was already in place with the matching soft-delete predicate.

MR size. 2951 lines, of which 773 are production code and the rest is the test matrix (four keyset walks per sort and direction, soft-delete exclusion, chain scoping, and per-sort EXPLAIN). Splitting packages from versions would put the shared publish-committer fixture in one MR and half its callers in the other.

Spec coverage

Spec / plan requirement Covered by Test
Artifact lists: packages sort name asc default, keyset, hasMore ListNpmPackages TestNpmPackageStore_ListNpmPackages, ..._KeysetNoGapsOrDuplicates
Artifact lists: versions default created_at desc, sort=version lexicographic ListNpmVersionsByPackage TestNpmVersionStore_ListNpmVersionsByPackage, ..._KeysetNoGapsOrDuplicates, ..._CreatedAtTieBreak
AC 19: npm package rows carry name, scope, versions_count, tags_count npmPackageColumns TestNpmPackageStore_ListNpmPackages (projection subtest)
AC 20: version detail carries the publish attribution, null when absent FindNpmVersionByID TestNpmVersionStore_FindNpmVersionByID, ..._ListNpmVersionsByPackage (projection subtest)
Artifact read routes: a break anywhere in the chain returns not-found FindNpmPackageInRepository, FindNpmVersionByID TestNpmPackageStore_FindNpmPackageInRepository, TestNpmVersionStore_FindNpmVersionByID
Security Considerations: tenant isolation by namespace_id every predicate ..._ChainScoping, ..._Scoping, both pruning tests
Soft-delete exclusion at every level of the chain all four reads ..._ExcludesSoftDeleted, soft-deleted-parent subtest
Plan: EXPLAIN shows index backing per sort statement builders ..._DeepPageIsIndexBacked (5 sort and direction pairs)

Test plan

go test ./internal/datastore/...

ARTIFACT_REGISTRY_DATABASE_TEST_DSN="postgres://...:5432/artifact_registry_test?sslmode=disable" \
  go test -tags=integration -count=1 -run 'TestNpmPackageStore_|TestNpmVersionStore_' ./internal/datastore/

The test:integration job runs the integration suite on the PostgreSQL 16, 17, and 18 matrix. The new tests add roughly 8 seconds.

No e2e scenario changes: the catalogs cover protocol-client journeys, and artifact browsing lands with the monolith slices that consume these endpoints.

Related to #312 (closed)

Database Review Evidence

Queries

Note

Plans are from EXPLAIN (ANALYZE, BUFFERS) against PostgreSQL 16.14 with synthesized seed data, torn down after the run. The project's canonical version is 17 (GL_PG_CURR_VERSION), and test:integration asserts these same plan shapes on 16, 17, and 18. Numbers reflect moderate cardinality and do not capture production-scale effects. See Database review evidence for seed sizing and methodology. Expand each row for the seed shape, rendered SQL, bound args, and raw plan.

Method Plan node Index Rows (plan / actual) Cost Time Buffers (hit / read) Partitions
datastore.ListNpmPackages.name-asc Index Scan npm_packages_p24_namespace_id_npm_repository_id_name_idx 21 / 21 3.64 0.027ms 3 / 0 1
datastore.ListNpmPackages.name-desc Index Scan Backward npm_packages_p24_namespace_id_npm_repository_id_name_idx 21 / 21 3.64 0.020ms 3 / 0 1
datastore.FindNpmPackageInRepository Index Scan npm_packages_p24_pkey 1 / 1 8.31 0.029ms 3 / 0 1
datastore.ListNpmVersionsByPackage.created_at-desc Index Scan Backward npm_versions_p24_namespace_id_npm_package_id_created_at_id_idx 21 / 21 3.71 0.039ms 3 / 0 1
datastore.ListNpmVersionsByPackage.version-asc Index Scan npm_versions_p24_namespace_id_npm_package_id_version_idx 21 / 21 3.61 0.027ms 7 / 0 1
datastore.FindNpmVersionByID.version-leg Index Scan npm_versions_p24_pkey 1 / 1 8.30 0.021ms 3 / 0 1
datastore.FindNpmVersionByID.parent-leg Index Scan npm_packages_p24_pkey 1 / 1 8.31 0.016ms 3 / 0 1
datastore.ListNpmPackages.name-asc

Summary: Forward scan of the partial unique name index with every predicate in the Index Cond, so the soft-delete filter and the repository scoping are index-resident rather than a post-scan filter. Plan rows equal actual rows at the fetch limit, and 3 buffer hits means the page came from the index and its heap pages alone. The 5000 sibling-repository packages in the same partition cost nothing.

Seed shape: npm_packages=10002, npm_versions=10000, npm_repositories=2, repositories=2

Rendered SQL:

SELECT npm_packages.namespace_id AS "npm_packages.namespace_id",
     npm_packages.id AS "npm_packages.id",
     npm_packages.npm_repository_id AS "npm_packages.npm_repository_id",
     npm_packages.last_downloaded_at AS "npm_packages.last_downloaded_at",
     npm_packages.soft_deleted_at AS "npm_packages.soft_deleted_at",
     npm_packages.versions_count AS "npm_packages.versions_count",
     npm_packages.tags_count AS "npm_packages.tags_count",
     npm_packages.name AS "npm_packages.name",
     npm_packages.scope AS "npm_packages.scope"
FROM public.npm_packages
WHERE (((npm_packages.namespace_id = $1::uuid) AND (npm_packages.npm_repository_id = $2::uuid)) AND (npm_packages.soft_deleted_at IS NULL)) AND (npm_packages.name > $3::text)
ORDER BY npm_packages.name ASC
LIMIT $4;

Bound args: [c9436e39-6349-40ad-985d-708af9e30c72 019faadb-11e1-7482-931e-c89a0e931654 dbreview-2500 21]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Limit  (cost=0.29..3.64 rows=21 width=118) (actual time=0.010..0.013 rows=21 loops=1)
  Buffers: shared hit=3
  ->  Index Scan using npm_packages_p24_namespace_id_npm_repository_id_name_idx on npm_packages_p24 npm_packages  (cost=0.29..399.85 rows=2500 width=118) (actual time=0.009..0.011 rows=21 loops=1)
        Index Cond: ((namespace_id = 'c9436e39-6349-40ad-985d-708af9e30c72'::uuid) AND (npm_repository_id = '019faadb-11e1-7482-931e-c89a0e931654'::uuid) AND (name > 'dbreview-2500'::text))
        Buffers: shared hit=3
Planning:
  Buffers: shared hit=287
Planning Time: 0.844 ms
Execution Time: 0.027 ms

Timings: planning 0.844ms, execution 0.027ms, total 0.871ms.

datastore.ListNpmPackages.name-desc

Summary: The same index read backward, which is why one ascending btree serves both directions with no post-scan Sort. Identical cost and buffers to the ascending case.

Seed shape: npm_packages=10002, npm_versions=10000, npm_repositories=2, repositories=2

Rendered SQL:

SELECT npm_packages.namespace_id AS "npm_packages.namespace_id",
     npm_packages.id AS "npm_packages.id",
     npm_packages.npm_repository_id AS "npm_packages.npm_repository_id",
     npm_packages.last_downloaded_at AS "npm_packages.last_downloaded_at",
     npm_packages.soft_deleted_at AS "npm_packages.soft_deleted_at",
     npm_packages.versions_count AS "npm_packages.versions_count",
     npm_packages.tags_count AS "npm_packages.tags_count",
     npm_packages.name AS "npm_packages.name",
     npm_packages.scope AS "npm_packages.scope"
FROM public.npm_packages
WHERE (((npm_packages.namespace_id = $1::uuid) AND (npm_packages.npm_repository_id = $2::uuid)) AND (npm_packages.soft_deleted_at IS NULL)) AND (npm_packages.name < $3::text)
ORDER BY npm_packages.name DESC
LIMIT $4;

Bound args: [c9436e39-6349-40ad-985d-708af9e30c72 019faadb-11e1-7482-931e-c89a0e931654 dbreview-2500 21]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Limit  (cost=0.29..3.64 rows=21 width=118) (actual time=0.009..0.011 rows=21 loops=1)
  Buffers: shared hit=3
  ->  Index Scan Backward using npm_packages_p24_namespace_id_npm_repository_id_name_idx on npm_packages_p24 npm_packages  (cost=0.29..399.87 rows=2501 width=118) (actual time=0.009..0.010 rows=21 loops=1)
        Index Cond: ((namespace_id = 'c9436e39-6349-40ad-985d-708af9e30c72'::uuid) AND (npm_repository_id = '019faadb-11e1-7482-931e-c89a0e931654'::uuid) AND (name < 'dbreview-2500'::text))
        Buffers: shared hit=3
Planning:
  Buffers: shared hit=5
Planning Time: 0.108 ms
Execution Time: 0.020 ms

Timings: planning 0.108ms, execution 0.020ms, total 0.128ms.

datastore.FindNpmPackageInRepository

Summary: Primary-key seek on (id, namespace_id) with the repository and soft-delete predicates as a filter on the single row it returns. The filter here fans out nothing: the Index Cond already pins one row, so the cost does not move with the number of packages in the namespace.

Seed shape: npm_packages=10002, npm_versions=10000, npm_repositories=2, repositories=2

Rendered SQL:

SELECT npm_packages.namespace_id AS "npm_packages.namespace_id",
     npm_packages.id AS "npm_packages.id",
     npm_packages.npm_repository_id AS "npm_packages.npm_repository_id",
     npm_packages.last_downloaded_at AS "npm_packages.last_downloaded_at",
     npm_packages.soft_deleted_at AS "npm_packages.soft_deleted_at",
     npm_packages.versions_count AS "npm_packages.versions_count",
     npm_packages.tags_count AS "npm_packages.tags_count",
     npm_packages.name AS "npm_packages.name",
     npm_packages.scope AS "npm_packages.scope"
FROM public.npm_packages
WHERE (((npm_packages.namespace_id = $1::uuid) AND (npm_packages.id = $2::uuid)) AND (npm_packages.npm_repository_id = $3::uuid)) AND (npm_packages.soft_deleted_at IS NULL)
LIMIT $4;

Bound args: [c9436e39-6349-40ad-985d-708af9e30c72 dfaccdad-b11f-4f98-9f2f-a583dec0ce1d 019faadb-11e1-7482-931e-c89a0e931654 1]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Limit  (cost=0.29..8.31 rows=1 width=118) (actual time=0.016..0.017 rows=1 loops=1)
  Buffers: shared hit=3
  ->  Index Scan using npm_packages_p24_pkey on npm_packages_p24 npm_packages  (cost=0.29..8.31 rows=1 width=118) (actual time=0.016..0.016 rows=1 loops=1)
        Index Cond: ((id = 'dfaccdad-b11f-4f98-9f2f-a583dec0ce1d'::uuid) AND (namespace_id = 'c9436e39-6349-40ad-985d-708af9e30c72'::uuid))
        Filter: ((soft_deleted_at IS NULL) AND (npm_repository_id = '019faadb-11e1-7482-931e-c89a0e931654'::uuid))
        Buffers: shared hit=3
Planning Time: 0.091 ms
Execution Time: 0.029 ms

Timings: planning 0.091ms, execution 0.029ms, total 0.120ms.

datastore.ListNpmVersionsByPackage.created_at-desc

Summary: Backward scan of the created_at keyset index with the row-value bound folded into the Index Cond, which is the whole point of writing it as ROW(created_at, id) rather than the expanded boolean form. A mid-dataset cursor over 5000 versions still reads 3 buffers.

Seed shape: npm_packages=10002, npm_versions=10000, npm_repositories=2, repositories=2

Rendered SQL:

SELECT npm_versions.namespace_id AS "npm_versions.namespace_id",
     npm_versions.id AS "npm_versions.id",
     npm_versions.npm_package_id AS "npm_versions.npm_package_id",
     npm_versions.last_downloaded_at AS "npm_versions.last_downloaded_at",
     npm_versions.soft_deleted_at AS "npm_versions.soft_deleted_at",
     npm_versions.created_at AS "npm_versions.created_at",
     npm_versions.version AS "npm_versions.version",
     npm_versions.gitlab_user_id AS "npm_versions.gitlab_user_id",
     npm_versions.gitlab_project_id AS "npm_versions.gitlab_project_id",
     npm_versions.gitlab_git_commit_sha AS "npm_versions.gitlab_git_commit_sha"
FROM public.npm_versions
WHERE (((npm_versions.namespace_id = $1::uuid) AND (npm_versions.npm_package_id = $2::uuid)) AND (npm_versions.soft_deleted_at IS NULL)) AND ((npm_versions.created_at, npm_versions.id) < ($3::timestamp with time zone, $4::uuid))
ORDER BY npm_versions.created_at DESC, npm_versions.id DESC
LIMIT $5;

Bound args: [c9436e39-6349-40ad-985d-708af9e30c72 019faadb-11ea-7cd2-9bc6-cef792b88d1b 2026-01-02 17:40:00 +0000 UTC 019faadb-15de-76ed-9df7-f19af0d727c5 21]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Limit  (cost=0.29..3.71 rows=21 width=176) (actual time=0.010..0.013 rows=21 loops=1)
  Buffers: shared hit=3
  ->  Index Scan Backward using npm_versions_p24_namespace_id_npm_package_id_created_at_id_idx on npm_versions_p24 npm_versions  (cost=0.29..407.85 rows=2500 width=176) (actual time=0.009..0.012 rows=21 loops=1)
        Index Cond: ((namespace_id = 'c9436e39-6349-40ad-985d-708af9e30c72'::uuid) AND (npm_package_id = '019faadb-11ea-7cd2-9bc6-cef792b88d1b'::uuid) AND (ROW(created_at, id) < ROW('2026-01-02 17:40:00+00'::timestamp with time zone, '019faadb-15de-76ed-9df7-f19af0d727c5'::uuid)))
        Buffers: shared hit=3
Planning:
  Buffers: shared hit=321
Planning Time: 0.955 ms
Execution Time: 0.039 ms

Timings: planning 0.955ms, execution 0.039ms, total 0.994ms.

datastore.ListNpmVersionsByPackage.version-asc

Summary: Forward scan of the partial unique version index. The single-column bound on version is a complete keyset because version is unique per package among active rows, so no id tiebreaker appears and no Sort node is needed.

Seed shape: npm_packages=10002, npm_versions=10000, npm_repositories=2, repositories=2

Rendered SQL:

SELECT npm_versions.namespace_id AS "npm_versions.namespace_id",
     npm_versions.id AS "npm_versions.id",
     npm_versions.npm_package_id AS "npm_versions.npm_package_id",
     npm_versions.last_downloaded_at AS "npm_versions.last_downloaded_at",
     npm_versions.soft_deleted_at AS "npm_versions.soft_deleted_at",
     npm_versions.created_at AS "npm_versions.created_at",
     npm_versions.version AS "npm_versions.version",
     npm_versions.gitlab_user_id AS "npm_versions.gitlab_user_id",
     npm_versions.gitlab_project_id AS "npm_versions.gitlab_project_id",
     npm_versions.gitlab_git_commit_sha AS "npm_versions.gitlab_git_commit_sha"
FROM public.npm_versions
WHERE (((npm_versions.namespace_id = $1::uuid) AND (npm_versions.npm_package_id = $2::uuid)) AND (npm_versions.soft_deleted_at IS NULL)) AND (npm_versions.version > $3::text)
ORDER BY npm_versions.version ASC
LIMIT $4;

Bound args: [c9436e39-6349-40ad-985d-708af9e30c72 019faadb-11ea-7cd2-9bc6-cef792b88d1b 1.0.2500 21]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Limit  (cost=0.29..3.61 rows=21 width=176) (actual time=0.010..0.018 rows=21 loops=1)
  Buffers: shared hit=7
  ->  Index Scan using npm_versions_p24_namespace_id_npm_package_id_version_idx on npm_versions_p24 npm_versions  (cost=0.29..524.37 rows=3315 width=176) (actual time=0.010..0.016 rows=21 loops=1)
        Index Cond: ((namespace_id = 'c9436e39-6349-40ad-985d-708af9e30c72'::uuid) AND (npm_package_id = '019faadb-11ea-7cd2-9bc6-cef792b88d1b'::uuid) AND (version > '1.0.2500'::text))
        Buffers: shared hit=7
Planning:
  Buffers: shared hit=1
Planning Time: 0.098 ms
Execution Time: 0.027 ms

Timings: planning 0.098ms, execution 0.027ms, total 0.125ms.

datastore.FindNpmVersionByID.version-leg

Summary: Primary-key seek returning the one version row. This leg replaced a single statement that related npm_versions to npm_packages: that shape planned a merge join estimating 5002 package rows and 69 buffer hits, because the package id it needs lives in the version row it has not read yet, so namespace_id was the only constant it could push into the package side.

Seed shape: npm_packages=10002, npm_versions=10000, npm_repositories=2, repositories=2

Rendered SQL:

SELECT npm_versions.namespace_id AS "npm_versions.namespace_id",
     npm_versions.id AS "npm_versions.id",
     npm_versions.npm_package_id AS "npm_versions.npm_package_id",
     npm_versions.last_downloaded_at AS "npm_versions.last_downloaded_at",
     npm_versions.soft_deleted_at AS "npm_versions.soft_deleted_at",
     npm_versions.created_at AS "npm_versions.created_at",
     npm_versions.version AS "npm_versions.version",
     npm_versions.gitlab_user_id AS "npm_versions.gitlab_user_id",
     npm_versions.gitlab_project_id AS "npm_versions.gitlab_project_id",
     npm_versions.gitlab_git_commit_sha AS "npm_versions.gitlab_git_commit_sha"
FROM public.npm_versions
WHERE ((npm_versions.namespace_id = $1::uuid) AND (npm_versions.id = $2::uuid)) AND (npm_versions.soft_deleted_at IS NULL)
LIMIT $3;

Bound args: [c9436e39-6349-40ad-985d-708af9e30c72 854d4961-1c1f-496d-9d7d-037db9a73f4a 1]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Limit  (cost=0.29..8.30 rows=1 width=176) (actual time=0.011..0.012 rows=1 loops=1)
  Buffers: shared hit=3
  ->  Index Scan using npm_versions_p24_pkey on npm_versions_p24 npm_versions  (cost=0.29..8.30 rows=1 width=176) (actual time=0.011..0.011 rows=1 loops=1)
        Index Cond: ((id = '854d4961-1c1f-496d-9d7d-037db9a73f4a'::uuid) AND (namespace_id = 'c9436e39-6349-40ad-985d-708af9e30c72'::uuid))
        Filter: (soft_deleted_at IS NULL)
        Buffers: shared hit=3
Planning:
  Buffers: shared hit=1
Planning Time: 0.064 ms
Execution Time: 0.021 ms

Timings: planning 0.064ms, execution 0.021ms, total 0.085ms.

datastore.FindNpmVersionByID.parent-leg

Summary: Primary-key seek verifying the parent package is active and belongs to the repository in the URL. Together with the version leg this is 6 buffer hits, and neither leg's cost grows with how many packages or versions the namespace holds.

Seed shape: npm_packages=10002, npm_versions=10000, npm_repositories=2, repositories=2

Rendered SQL:

SELECT npm_packages.id AS "npm_packages.id"
FROM public.npm_packages
WHERE (((npm_packages.namespace_id = $1::uuid) AND (npm_packages.id = $2::uuid)) AND (npm_packages.npm_repository_id = $3::uuid)) AND (npm_packages.soft_deleted_at IS NULL)
LIMIT $4;

Bound args: [c9436e39-6349-40ad-985d-708af9e30c72 019faadb-11ea-7cd2-9bc6-cef792b88d1b 019faadb-11e1-7482-931e-c89a0e931654 1]

Plan (EXPLAIN (ANALYZE, BUFFERS) output):

Limit  (cost=0.29..8.31 rows=1 width=16) (actual time=0.007..0.007 rows=1 loops=1)
  Buffers: shared hit=3
  ->  Index Scan using npm_packages_p24_pkey on npm_packages_p24 npm_packages  (cost=0.29..8.31 rows=1 width=16) (actual time=0.007..0.007 rows=1 loops=1)
        Index Cond: ((id = '019faadb-11ea-7cd2-9bc6-cef792b88d1b'::uuid) AND (namespace_id = 'c9436e39-6349-40ad-985d-708af9e30c72'::uuid))
        Filter: ((soft_deleted_at IS NULL) AND (npm_repository_id = '019faadb-11e1-7482-931e-c89a0e931654'::uuid))
        Buffers: shared hit=3
Planning Time: 0.049 ms
Execution Time: 0.016 ms

Timings: planning 0.049ms, execution 0.016ms, total 0.065ms.

Note

No migrations in this MR, so migration mode did not run. No flags: every statement is a single-partition index read whose plan rows match its actual rows, and no plan carries a post-scan Sort or a Rows Removed by Filter that fans out. The one anomaly the measurement found (the version detail read planning a namespace-wide merge join) is fixed in 67a283e5, and the plans above are from the fixed shape.

Context for LLM agents

What this is

Step 8 of the merged plan docs/plans/2026-07-22-s17-phase3-format-artifact-reads.md (lines 394-414). Datastore only. Steps 11 and 12 own the handlers, DTOs, and the handler.go / wire_management.go wiring, and nothing here touches them.

Design decisions and rejected alternatives

Version list does not join npm_packages. Rejected: joining to enforce npm_repository_id and the parent's soft-delete state. The join is affordable (a nested loop keeps the outer index order), but the spec's resolution flow already mandates a parent-detail read per request, so the join re-runs a check the handler just performed. Maven's ListMavenVersionsByPackage (Step 6) is specified the same way, and the Step 12 handler dispatches to both, so the two must agree.

Excluded package_json from the version projection. Rejected: returning full rows. The plan's word is "full rows", meaning the columns the resource needs rather than every column, contrasted against protocol listers that return names only. Precedent for a partial projection into model.NpmVersions is npmVersionPageForFileCascade.

Per-store page-size ceilings (maxNpmPackagesPageSize, reusing maxNpmVersionsPageSize) rather than relying on the handler's maxPageSize clamp of 100. They are backstops for a caller wiring the store directly. maxNpmVersionsPageSize already established the pattern in npm_versions.go.

NpmPackageCursor carries only Name, no id. name is unique per (namespace_id, npm_repository_id) among active rows, so a single-column bound is a complete keyset and a row-value tuple over (name, id) would reintroduce a column the index does not carry, forcing a post-index filter. Same reasoning as repositoryKeysetBound's name special case.

pg.WRAP not pg.ROW for the (created_at, id) bound. listByImageStmt documents that pg.WRAP emits the bare-parenthesized tuple the planner folds into a multicolumn index range, while pg.ROW emits the ROW keyword. The ..._DeepPageIsIndexBacked cases assert the fold empirically.

Own cleanup sweep (cleanupNpmManagementRows) instead of reusing cleanupNpmRowsForNamespace. The publish committer mints a blob_storage_attachments row that no seedAttachment per-row cleanup covers, and the existing sweep deletes attachments before npm_files, so the committer's attachment would survive and fail the namespace teardown. The helper's own doc explains why the existing ordering is fine for its own callers.

Non-goals

  • NpmPackageByName still hand-lists the same nine columns npmPackageColumns now returns. The plan says protocol methods stay untouched. Refactoring a protocol read to share the helper would widen the protocol-path review surface for no behavior change.
  • errNpmPackage*NonPositive* sentinel names predate the bigserial-to-UUID migration and read oddly next to their "must not be the zero UUID" messages. Renaming them touches every existing npm guard test and belongs in its own change.
  • No handler, DTO, route, or OpenAPI change. Steps 1, 11, and 12 own those.
  • No EXPLAIN (ANALYZE, BUFFERS) output in this description. The plan's acceptance is that the assertions live in the suite, and they do: five list cases pin the index by name and assert no post-scan Sort and no Rows Removed by Filter, and two detail cases assert single-partition pruning.

Review history

Two full /review-branch passes ran before this MR opened. Pass 1 raised two warnings, both fixed: the detail reads built their statements inline so nothing could EXPLAIN them (fixed by extracting the two builders plus pruning tests), and nothing distinguished a transient database failure from not-found (fixed by two cancelled-context subtests). Pass 2 raised one: a doc comment claimed whole-package unpublish leaves an active version under a soft-deleted package "for a while", which NpmPackageUnpublishDeleter contradicts because it cascades in one transaction. The predicate is still correct and the comment now says why.

The EXPLAIN measurement afterwards found what both review passes missed: the version detail read planned a namespace-wide merge join. Neither pass caught it because the plan is only visible from a real plan at scale, and the pruning test that pass 1 added asserted partition count rather than plan shape, so it passed the bad shape. Fixed in 67a283e5, with a plan-shape assertion that would not.

The automated AppSec review raised one medium design finding, declined with rationale in its thread: ListNpmVersionsByPackage delegates the parent-chain check to its caller, which the merged plan settles as the handler's job for every by-parent list in Phase 3. Its proposed proof-of-resolution token is also forgeable, since an empty composite literal needs no field names. Its fallback was taken: the obligation now sits on the params struct a handler author fills in.

Edited by Hayley Swimelar

Merge request reports