feat(managementapi): npm version stats (S17 Phase 8 plan: 20/13)
Why
The npm arm of .../{format}/versions/{version_id}/statistics answered 501.
This serves it: the version's visible file count, the distinct bytes those files
occupy, and its stored last_downloaded_at. files_count is the figure the
scope exists for, because a files page carries no total and no column holds one.
size_bytes derives per request rather than reading npm_versions.size_bytes.
That column holds the same footprint, and unlike hosted Maven's it has no flush
window: the publish commit writes it and a hard delete refolds it in the
delete's own transaction. Neither column has a reconciliation loop, so one that
has drifted stays drifted, and its specified correction is this same walk run
again. The derived figure is exact at read time whatever
the column holds, which is what the read demonstrates by answering over a
stamped-wrong one.
Step 19 landed first
The family-shared VersionStatistics DTO, the versionStatisticsScope verdict
type, and the handleVersionStatistics dispatch all landed in
feat(managementapi): maven version stats (S17 P... (!2021 - merged) • Hayley Swimelar • 19.4, whose npm arm was the placeholder this
fills.
This branch was stacked on that MR and now targets main. The parent merged as
a squash, which leaves no patch-id match against it, so the rebase used --onto
against refs/merge-requests/2021/head: nine parent commits dropped, the five
here kept. The Deps seam pin was re-derived from the walk's own failure on the
rebased tree rather than by arithmetic, because sibling steps moved main's count
while this branch was open.
The footprint's join, and the gate over it
The plan entry for this step names one external gate. ADR-007 declines the
blob_storage_blobs_by_namespace shadow for a per-version walk, on the premise
that a walk holding its own digests prunes the base table, and the spec's
version scope fixes the shadow as the join target
(Spec: Phase 8 statistics reads).
The measurement that settles it is delivered on Measure the artifact-level size lookup: base ta... (#564) • Hayley Swimelar and its disposition recorded on S17 Phase 8: docs corrections from wave-1 build... (#916) • Hayley Swimelar • 19.4: the shadow join wins on PostgreSQL 16, 17, and 18, and npm's base-table form prunes nothing, opening all 64 partitions on every call and beating the shadow in none of the 123 cells measured. The verdict is the shadow, and both version steps were released against it.
Two document fixes remain, and neither gates this code. While
Amend AR ADR-007: artifact-level walks read the... (gitlab-com/content-sites/handbook!20894 - merged) • Hayley Swimelar is open the docs/adr/ mirror carries
ADR-007's pre-amendment conclusion, and once it merges the daily sync:adrs job
brings the amended text over. The spec's version scope keeps its conclusion and
swaps its stated reason for the measured one.
What is not obvious
The two base-table walks stay where they are. NpmFileStore answers this
footprint twice already: SumDistinctNpmFileSizesByVersion as a pool read with
no production caller, and RecomputeNpmVersionSizeBytesTx as the fold that
keeps npm_versions.size_bytes current on a hard delete. Neither is reused. The
fold is an UPDATE and returns no figure, and the pool twin returns the right
bytes on the wrong path: blob_storage_blobs is partitioned by HASH(sha256),
which a runtime digest set cannot prune. Both belong to the storage accounting
workstream and keep their shape, so the package now answers one question twice,
across npm_files.go and npm_versions.go, which the plan's Research Findings
names as a deliberate near-miss. This walk composes the shared shadow-joining
sum instead, writing no join, no aggregate, and no alias of its own.
The arm keeps its own hosted verdict. It does not reuse
hostedVersionFileRepository, the gate the Phase 3 version reads ride, and the
reason is what that gate becomes rather than what it answers: the remote
artifact reads narrow it to virtual, while statistics stay hosted-only. The two
answer the same bytes today, since writeRemoteArtifactReadPending calls
writeRepositoryNotFound, so no test can tell them apart and none claims to.
npm has no package-level file row. npm_files.npm_version_id is NOT NULL
and the table carries no package column, so Maven's third scope, the
maven-metadata.xml row a version walk must skip, does not exist here. The
scope fixtures seed a neighbouring package's version instead, which keeps the
same three-way leak detectable: dropping the version predicate answers 1111,
reaching one neighbour alone 1011 or 111.
The mirrored format-mismatch row lands here, because the Maven arm's
addressing matrix could not drive a Maven repository under the npm segment
while this arm answered the placeholder.
One Deps field, four literals. handler_test.go, pact_provider_test.go,
read_handlers_integration_test.go, and
cmd/artifact-registry/wire_management_test.go each construct one, and
requireNonNilSeams panics at construction on a nil leaf, so all four move
together. The seam count pin moves by one, re-derived from the walk's own failure
message after each rebase rather than by arithmetic, because sibling steps add
their own fields. No hand-written nil guard: the reflective walk covers every
field.
Eight //nolint:dupl directives, four of them on the Maven twins. dupl
reports both halves of a pair, so a per-format suite cannot suppress it from one
side alone. Each was measured by dropping it and re-running. The same
measurement is why four //nolint:nilnil tokens leave
fake_version_file_readers_test.go in this MR: that linter sits in
.golangci.yaml's disable list, so they suppressed nothing, and the file comes
out consistent with the measured-token rule rather than with its neighbours.
Diff size
Past the 500-LOC guideline against main, and the overrun is test mass. Splitting the reads
from the handler was interrogated when the plan was written and rejected, and
the measurement holds here: the split lands at roughly 988 and 903 LOC, so
neither half clears 500 and it buys a duplicated fixture and nothing else.
| Group | Added + removed |
|---|---|
Production, internal/datastore |
101 |
Production, internal/managementapi |
121 |
Production, cmd/artifact-registry |
2 |
Tests, internal/datastore |
887 |
Tests, internal/managementapi |
750 |
Tests, cmd/artifact-registry |
30 |
| Total | 1891 |
Test plan
go test ./internal/managementapi/ ./internal/datastore/ ./cmd/artifact-registry/go test -tags=integration ./internal/datastore/ ./internal/managementapi/, both packages whole rather than filtered to this step, against a PostgreSQL 16 carrying CI's own.pg-service-options(max_locks_per_transaction=1024included, without which the 64-partition tables exhaust the lock table and unrelated suites fail in their namespace-delete cleanup)go-lint-ci ./internal/... ./cmd/..., plus the integration-tagged run with--max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false, which is the only run that compiles the//go:build integrationfiles at all
Both plan pins are measured, not reasoned, and neither names an index. The count
plans as an Index Only Scan over one npm_files partition with both scoping
columns in the Index Cond. Every index that can answer its predicate leads with
that pair, so the assertion holds whichever the planner picks.
The footprint cannot promise that, and the plan's Acceptance line asks it to. The
line names "index-assisted on index_npm_files_on_ns_id_ver_id", which this walk
cannot bind: that index carries no digest column, so DISTINCT blob_sha256 would
pay a sort, and measured on PostgreSQL 16 the planner takes
(namespace_id, blob_sha256) and applies the version as a Filter. Maven's twin
can name its index only because index_maven_files_on_ns_id_ver_id is three
columns wide. So the pin asserts what every correct plan must bind and nothing
more: one partition, the namespace in the Index Cond, and the version at that
same scan, as an Index Cond term or as its Filter. Reading the npm_files
scan node alone is what keeps that honest, because a predicate rendered above the
scan narrows a range already read, and npmFilesScanNode carries a five-case
test, two cases of which exist to kill one break condition each. Nothing asserts
the absence of a sequential scan: explainListPlan sets enable_seqscan=off, so
that assertion could never fail. A step MR cannot edit the plan, so this is where
the correction is recorded.
Both queries were mutation-tested. Tautology-swapping either namespace
predicate, either version predicate, the count's soft-delete predicate, or the
digest subquery's DISTINCT fails at least one behavioural subtest in every
case. The shadow join's own namespace predicate survives this step's subtests
and is killed by TestSumDistinctBlobSizes, which is the seam that owns it.
No e2e scenario is added or affected. The statistics consumers are the monolith UI slices, whose journeys land with those slices, and no protocol-client catalog scenario reads statistics, which the plan's Testing Strategy records. No protocol behavior changes, so conformance does not apply, and the MR adds no configuration, no migration, and no OpenAPI operation.
Spec coverage
Spec coverage: acceptance criteria, error cases, and security considerations
Spec coverage
Spec: docs/specs/S17-rest-management-api.md
Acceptance criteria
| # | Criterion | Tests |
|---|---|---|
| AC-116 | files_count matches the version's visible file list |
TestNpmVersionStore_CountVisibleNpmFilesByVersion (the count read against ListNpmFilesByVersion's own page, a marked row, a sibling version's row and another package's version's row excluded, an empty version reads 0, a cross-namespace id reads 0), TestNpmVersionVisibleFilesCountStmt_BindsBothScopesAndTheMark, TestNpmVersionStatistics_SerializesBothFiguresAndTheStoredInstant |
| AC-116 | size_bytes counts a blob two of the version's files share once |
TestNpmVersionStore_SumNpmVersionFootprint (a digest two of the version's files share counts once asserts 4096 rather than 8192, and asserts the count 2 beside it), TestNpmVersionFootprintDigestsStmt_ScopesTheWalkToOneVersion (the DISTINCT and the alias the shared sum joins on) |
| AC-116 | The figure is derived, so it may differ from the stored column. The spec sentence names maven_versions.size_bytes, and the npm arm derives the same way, against npm_versions.size_bytes |
TestNpmVersionStore_SumNpmVersionFootprint (the derived figure answers where the stored column disagrees, stamping 999999 against a 64-byte fixture) |
| AC-117 | A blob two versions share counts once in each, so the two footprints sum to more than the distinct bytes | TestNpmVersionStore_SumNpmVersionFootprint (a digest two versions share counts in full in each, which asserts both figures and that their sum exceeds the distinct bytes) |
| AC-119 | A tombstoned file's bytes stay in the live size_bytes walk while every live count drops it from the mark onward - version-scope half |
TestNpmVersionStore_SumNpmVersionFootprint (a marked file's bytes stay while its row leaves the count, asserting 137 bytes against 1 file in one fixture), TestNpmVersionFootprintDigestsStmt_KeepsMarkedFilesInTheWalk (the absent predicate, which no EXPLAIN can show). The stored-counter half is S22's own settlement rule. |
| AC-120 | Phase 3 addressing: a missing, foreign, malformed, or soft-deleted target returns 404 |
TestNpmVersionStatistics_AddressingMatrix (unresolved slug, missing repository, the repository under another slug, soft-deleted repository, a non-UUID, unhyphenated, and nil-UUID version id, and an id addressing no row), TestNpmVersionStatistics_SoftDeletedVersionIs404, TestNpmVersionStatistics_VersionUnderAnotherRepositoryIs404 |
| AC-120 | A :format segment differing from the repository's format returns 404 |
TestNpmVersionStatistics_AddressingMatrix (maven repository under the npm format segment, the direction the Maven arm's row could not drive while this arm answered the placeholder, and docker repository under its own format segment) |
| AC-120 | The route on a virtual or remote repository returns 404 |
TestNpmVersionStatistics_NonHostedRepositoryIs404 (both kinds, asserting the repository-class message and that neither figure nor the chain read ran), TestFindNpmVersionInScope_RejectsNonHostedScope, and the family-shared TestVersionStatisticsScopeFor_StoresTheHostedVerdict and TestVersionStatisticsScope_ZeroValueIsNotHosted |
| AC-121 | Every statistics error path returns the S01 envelope with the correlation request_id |
assertErrorEnvelope inside every 404 and 500 case above, plus TestStatisticsNpmVersion_ResponsesMatchOpenAPIContract (the 404 and 500 rows through validateAgainstContract) |
| AC-113, AC-114, AC-115, AC-118, AC-122, AC-123 | Other statistics scopes, the withdrawn per-artifact paths, and the connection test | Other steps of this plan, or other phases. This step serves the npm version arm alone, which drains the last getVersionStatistics entry from contractPendingOperations. 24 entries remain, the 20 upstream-repository operations and the 4 other statistics scopes. |
Error cases
| Condition | Tests |
|---|---|
Version statistics: any Artifact-routes condition (missing, foreign, malformed, soft-deleted, format mismatch) is 404 not_found |
TestNpmVersionStatistics_AddressingMatrix, TestNpmVersionStatistics_SoftDeletedVersionIs404, TestNpmVersionStatistics_VersionUnderAnotherRepositoryIs404 |
Version statistics: repository kind not hosted is 404, even on the remote repositories the artifact routes serve |
TestNpmVersionStatistics_NonHostedRepositoryIs404. The remote body is byte-identical to the artifact routes' own remote answer, so the case pins the shared shape and the durable reason travels in prose: remote artifact reads narrow that gate to virtual, statistics stay hosted-only |
A statistics read that fails is a logged 500, never a masked 404 |
TestNpmVersionStatistics_SeamFailureReturns500 (both figures, the child-row resolve, the chain read, and a nil row with no error, each with its own log line) |
| Store argument guards: nil context, zero namespace, zero version id | TestNpmVersionStore_StatisticsReads_ArgumentGuards, TestNpmVersionStore_StatisticsReads_RejectTheZeroIDsSeparately |
Security considerations
| Concern | Tests |
|---|---|
| Existence hiding: a break at the repository is indistinguishable from a missing one, and a break below it from a missing artifact | TestNpmVersionStatistics_AddressingMatrix asserts the message class per row, so no response separates a well-formed guess from a malformed one within its class |
Tenant isolation: every query is scoped by the resolved namespace_id, and the parent chain is verified down from the repository in the URL |
TestNpmVersionStore_CountVisibleNpmFilesByVersion (a version id under another namespace counts nothing), TestNpmVersionStore_SumNpmVersionFootprint (a version id under another namespace sums nothing, which seeds the foreign digest under the addressed namespace at a different size so the walk's own predicate is the only thing holding the answer at zero), TestNpmVersionStatistics_VersionUnderAnotherRepositoryIs404, and the ids asserted on both recorded reads in TestNpmVersionStatistics_SerializesBothFiguresAndTheStoredInstant |
| Read cost: the npm walks prune to one partition and bind both scopes at their scan | TestNpmVersionStore_CountVisibleNpmFilesByVersion_PlanBindsBothScopingColumns (one partition, both columns in one Index Cond), TestNpmVersionStore_SumNpmVersionFootprint_PlanPrunesAndBindsTheVersion (one partition, the namespace in an Index Cond, the version at the same scan). Neither names an index. The footprint walk does not require the version in the Index Cond: no npm_files index carries both the version and the digest, so DISTINCT trades a free ordering against a narrower range, and measured on PostgreSQL 16 it takes the ordering index at this fixture's row count. Both pins read the npm_files scan node alone, so a predicate applied above the scan fails them |
Statistics authorization: the version scope binds to read_artifact |
S09's enforcement step owns it. No authorization runs on this route yet, so nothing here can assert it. |
| Attribution exposure, echoed input, bounded blast radius, aggregate disclosure | Not reachable at this scope: the response carries three figures and no name, identifier, or submitted value. |
Related to #316