feat(npm): thread the delivery-mode override into hosted metadata reads
What this changes
The hosted packument and dist-tags reads passed no open options to their
cache-blob read, so a namespace that set namespaces.delivery_mode_override was
answered on the instance default by those two routes while every other npm read
honored the column.
ADR-005
makes the per-namespace setting available regardless of that default. !2043 (merged)
closed the resolver field and four of the six npm reads that open a blob; this
closes the remaining two, and with them #708 (closed).
Both routes resolve their document through HostedDocumentReader.Read, whose
signature already ends in opts ...storage.BlobOpenOption and which already
forwards them to OpenBlob. Each call site now appends blobOpenOptions(res),
the helper the tarball and kind=remote reads already use:
| Route | Site |
|---|---|
Hosted packument GET |
internal/format/npm/packument_get.go:221 |
Hosted dist-tags GET |
internal/format/npm/disttags.go:720 |
No new query, no finder or constructor signature change, no migration. The production diff is two argument lists and one doc comment.
Both arms honor it, which is a change from what the tree documented
HostedDocumentReader opens the cache blob at one place
(hosted_document_read.go:265), reached from both of its arms: the fresh row,
and the row a cache miss fills before answering from it. So threading the option
at the two Read call sites covers both arms, and a namespace gets one answer
whatever the cache's warmth.
That contradicts what remote_document_serve.md and the
e2e.npm.consume.delivery-mode-override row said. Both described a
cache-hit-only blob read, naming DistTagsHandler.serveCacheHit,
PackumentHandler.serveCacheHit and serveInlineBuild. Those three symbols no
longer exist in production code: !1985 (merged) routed both handlers through
HostedDocumentReader, and !1990 (merged) converged the packument cache miss on the
fenced rebuild. The two documents are corrected here rather than only losing
their #708 pointer, because the caveat they carried is now false in a way that
would mislead an e2e run into controlling cache warmth it does not need to
control.
Those same two merge requests are why !2043 (merged) left this out: both were open against these files at the time, and an implementation opened alongside them would have conflicted without either pipeline reporting it. Both have since merged, which is what unblocks this.
Governing ADRs
ADR-005 is the one ADR this touches, and it is the reason for the change rather than a constraint on it: the two-axis model it specifies is what the hosted metadata reads were not implementing. Nothing here deviates from it, so no handbook amendment MR is opened. The local mirror was read, not inferred from its title.
Tests
New internal/format/npm/hosted_delivery_mode_test.go, mirroring
remote_delivery_mode_test.go rather than growing either handler suite, which
sit at 81 KB and 69 KB and lose per-line diff anchors as they approach 100 KB.
TestHostedMetadataReads_ThreadTheDeliveryModeOverride: 6 subtests, both routes over a proxy, a redirect and aNULLcolumn, asserting the option that reachesOpenBlob. Its fake always streams, so a threading assertion is not also an assertion about the shape.TestHostedMetadataReads_DeliveryModeSelectsTheServedShape: 8 subtests, both routes over both instance defaults, asserting the response a client receives. Both diagonals are covered (a proxy override under a redirect default, a redirect override under a proxy default), so aNULLrow and an override naming the instance default are distinguishable rather than the same assertion twice.TestHostedMetadataReads_MissArmSelectsTheServedShape: 4 subtests driving a real miss through its fenced fill, which is what pins the second arm. It wires the process-global rebuild dependencies, so it is not parallel and says so. Its307branch asserts the validator per route, because the two307s are not shaped alike:hdmRoute.redirectCarriesValidatorrecords which route carries the strong ETag, and the dist-tags assertion is taken over the row the fill committed rather than over a fixture. Inverting both expectations fails both redirect subtests.TestHostedMetadataReads_DistTagsSelfHealThreadsTheOverride: one test over the path a gone cache blob takes, where a fresh row's blob is missing and the read falls through to the rebuild before answering. It asserts the rebuild ran once, the read still answers200, and the lastOpenBlobstill carried the override. It needsfillStore.seedRow, which is whyinternal/format/npm/packument_fill_test.gois in the diff. The self-heal shares itsbuildInlinecall with the miss arm (hosted_document_read.go:224), so this is path coverage rather than a mutation nothing else catches: dropping the opts at that call, or atbuildInline's post-fillfromCacheRow, fails this test and the four miss-arm subtests, while dropping it on the cache-hitfromCacheRowfails only the fresh-row tests.
Each of the two threading sites was reverted in turn and the suite failed, so neither is asserted only by a test that would pass without it.
go test ./internal/format/npm/... is green.
golangci-lint run --max-same-issues=0 --max-issues-per-linter=0 --uniq-by-line=false ./internal/format/npm/...
reports 0 issues. The --build-tags=integration run reports 66 findings across
the package's integration files, none of them in a file this MR touches, so they
are pre-existing.
e2e scenarios
docs/testing/e2e/npm.md's e2e.npm.consume.delivery-mode-override row is
updated: it now tells a run to assert all three reads rather than the tarball
GET alone, and to assert the validator per route, because a dist-tags 307
carries the strong ETag and a packument 307 carries none
(cachedDelivery sets it for NpmMetadataKindDistTags only).
Reviewable LOC
589 changed lines, split:
| File group | Lines |
|---|---|
internal/format/npm/hosted_delivery_mode_test.go (new test) |
514 |
internal/format/npm/remote_document_serve.md (sidecar prose) |
51 |
internal/format/npm/packument_fill_test.go (seedRow helper) |
16 |
docs/testing/e2e/npm.md (one table row) |
2 |
Production Go (disttags.go, packument_get.go, resolver.go) |
6 |
Splitting would not help: the production change is 6 lines across three files and the two threading sites are the same defect, so a split would separate a one-line change from the test that proves it. The growth over the original 500 is review-requested coverage on the same two routes, which belongs with the change it constrains.
File overlap with open merge requests
git diff --name-only main...HEAD was checked against every open MR (70 at the
time of writing). One shares a file and neither shares a changed line: !1916 (merged)
touches docs/testing/e2e/npm.md at rows 49-55, 182-188 and 259-265, and this
MR touches row 130. No open MR touches the Go files.
Why this closes the issue rather than relating to it
#708 (closed)'s stated work is four items. Three landed in !2043 (merged) and are on main: the
npm.Resolution.DeliveryModeOverride field and its resolver mapping, the hosted
tarball GET, and the three kind=remote reads. The fourth is the two hosted
metadata reads, which is this MR. After it merges nothing the issue asks for is
left, so the form is Closes.
The evidence for the three earlier items is on main rather than in this diff.
A reviewer can check it with
git grep -n 'blobOpenOptions' -- internal/format/npm/, which after this MR
lists all six threading sites and the helper.
Closes #708 (closed)