feat(managementapi): serve the container artifact reads
Why
The five container artifact routes answered 501. The monolith
repository-detail and artifact-detail slices need the container_images,
container_tags, and container_manifests rows the S12 push path writes, and
Step 3 seeded these placeholders for this step to fill.
This is Step 10 of the merged plan in
docs/plans/2026-07-22-s17-phase3-format-artifact-reads.md,
covering the container handlers: images list and detail, tags list and detail,
and the per-image manifests list with its include_referrers toggle.
Retargeted to main. This step depended on Steps 1, 3, 4, and 5, and all
four have merged, so it no longer needs the integration branch. Rebasing onto
main absorbed three changes those steps made after this branch forked: the
per-family resolver split, the kind return FindByNameInNamespace gained, and
the staged markers Step 1's contract carries.
What (the non-obvious parts)
Query parameters are validated before the repository resolve. Resolving
first would answer 400 for a repository the caller can see and 404 for one it
cannot, which turns ?limit=0 into the existence oracle the rest of the surface
denies. The 400 tables assert zero store calls, the repository reader included.
A container-format repository with no container_repositories row is a logged
500, not a 404. RepositoryStore.Create writes parent and child in one
transaction, so a miss is a broken invariant, and folding it into the
existence-hiding 404 would report every image of that repository absent while the
defect stayed invisible.
The two child lists read their parent image first, and tag detail does not.
The lists need it for the 404-versus-200 [] distinction the spec draws, and
the manifests list needs it for isolation as well, because
container_manifests carries no repository column and its store scopes by
namespace and image alone. Tag detail's own query joins up to the image and pins
the repository, so one round trip verifies the whole chain.
Three shared test files gained a per-step block. The new Deps fields are
required, so every construction site (newTestDeps, newIntegrationHandler,
managementWireDeps) needs them. The two 501 route sweeps now discriminate per
row, the wire sweep by an expected status and the handler sweep by a skip flag,
so Steps 11 and 12 flip only their own rows. Integration coverage went into a
new file rather than the shared one for the same reason.
The diff is 4066 lines across 18 files, over the 500 ceiling. 1113 of that is production code, much of it doc comments, and the rest is the test suite this step's acceptance criteria call for.
Test plan
go test ./internal/managementapi/ ./cmd/artifact-registry/
go test -race ./internal/managementapi/
ARTIFACT_REGISTRY_DATABASE_TEST_DSN=... go test -tags=integration ./internal/managementapi/
go-lint-ci ./internal/managementapi/... ./cmd/artifact-registry/...
go-lint-ci --build-tags=integration ./internal/managementapi/...Ran locally: unit green, -race green, managementapi integration green
(14 s) against PostgreSQL via testcontainers, both lint runs report 0 issues.
The contract sweep validates all thirteen container response classes against
api/openapi/v1.yaml.
Conformance: not applicable. This is management-API surface, not Maven, npm, or OCI protocol behavior.
E2e scenarios: no catalog change. The plan's Testing Strategy settles it. The catalogs cover protocol-client journeys, and artifact browsing is a UI journey that lands with the monolith S05 and S06 slices, which own the catalog additions.
Spec coverage
| Spec | Implementation | Tests |
|---|---|---|
AC #15 images list by name with keyset pagination, detail fields, missing id 404 |
handleContainerImageList, handleContainerImageDetail |
TestContainerImageList_Defaults, ..._KeysetWalk, TestContainerImageDetail_Returns200, ..._NotFound_IsOneResponse, TestContainerReadsIntegration_ImageKeysetWalk |
AC #16 (closed) tags list by name, detail carries manifest_digest, missing name 404 |
handleContainerTagList, handleContainerTagDetail |
TestContainerTagList_Defaults, ..._KeysetWalk, TestContainerTagDetail_Returns200, ..._NotFound_IsOneResponse, TestContainerReadsIntegration_TagDetailJoinsManifestDigest |
| AC #17 (closed) manifests list newest-first with the eight documented fields | handleContainerManifestList, containerManifestFromRow |
TestContainerManifestList_Defaults, ..._ResourceFields, ..._Order |
AC #18 (closed) referrers excluded by default, returned on include_referrers=true, any other value 400 |
parseContainerManifestListQuery |
TestContainerManifestList_IncludeReferrers, ..._ReferrerFields, ..._WalkKeepsIncludeReferrers, TestContainerReadsIntegration_ManifestReferrerPartition |
AC #24 format-segment mismatch, an id outside the chain, and a non-UUID id all 404 |
resolveContainerRepository, findContainerImageForRequest, parsePathID |
TestContainerImageList_RepositoryResolution, TestContainerImageDetail_NotFound_IsOneResponse, TestContainerTagList_ImageOfAnotherRepository_Returns404, TestContainerReadsIntegration_FormatSegmentMismatch, ..._ImageDetailIsRepositoryScoped |
AC #25 an existing parent with no children returns 200 [] |
parent read before each list | TestContainerImageList_EmptyPage_ReturnsEmptyJSONArray, TestContainerTagList_TaglessImage_Returns200Empty, TestContainerManifestList_ParentResolution |
AC #26 (closed) invalid sort, order, limit, or cursor returns 400 with the S01 envelope |
the two query parsers | TestContainerImageList_InvalidParams_Return400, TestContainerTagList_InvalidParams_Return400, TestContainerManifestList_InvalidParams_Return400 |
AC #27 (closed) hosted responses carry no cache key |
the three DTOs | the exact-key assertions in TestContainerImageDetail_Returns200, TestContainerTagDetail_Returns200, TestContainerManifestList_ResourceFields |
AC #23 (soft-delete invisibility) does not apply: no container table carries
soft_deleted_at.
Context for LLM agents
Design rationale
Resolve the repository through Step 3's shared helper, then the child row
separately. Every container query is scoped by container_repository_id, which
lives on container_repositories and not on the shared repositories row, so
each request costs two by-name lookups. Rejected: resolving through
ContainerRepositoryStore.FindByNameInNamespace alone, which returns the format
and would collapse the two into one query. It would bypass
resolveContainerArtifactRepository, so the format-mismatch semantics and the
unmapped-enum 500 would diverge from Steps 11 and 12, and it would duplicate
logic Step 3 landed and tested. Collapsing the two properly needs a datastore
method keyed on repository_id, which is Step 4's scope.
One shared parse function for the images and tags lists. Both take the
identical vocabulary (sort=name, order, limit, cursor), so the parsed form
carries a plain descending bool that each handler maps to its own store's order
enum. The two stores declare separate order types, which is what would otherwise
force two parsers.
Cursor codecs and Link builders are container-scoped, not shared with Steps 11 and 12. They are generic over the row and cursor type, so the sibling steps could reuse them, but naming them for the whole artifact surface would have made three concurrent branches declare the same package-level symbols. Unifying them once all three land is a follow-up worth taking.
Every new package-level symbol is container-prefixed, including the sort-value
constants. containerSortValueName rather than sortValueName, because the
packages list also sorts by name and Step 11 would plausibly declare the
unprefixed form. In the previous round two steps independently declared
negativeLimitCase in one package, both MRs were individually green, git
reported no conflict, and the package still failed to compile once both landed.
A 500 for a missing child row, weighed against a 404. The 404 would be
consistent with the surrounding existence-hiding, and it is what a caller would
see for any other chain break. It loses because the chain break here is
impossible through the write path: answering 404 would silently report an entire
repository empty while a data-integrity defect went unlogged. The 500 is logged
with the store's wrapped cause.
Non-goals
- No datastore change. Steps 4 and 5 own the reads this calls, including the
AppSec fix that added
container_repository_idtoListContainerTagsParams. - No OpenAPI schema change. Step 1 landed the contract for all thirteen
routes. This step only clears the staged markers on the five it serves:
x-not-implementedand the "Not yet implemented." description lead.TestContractOperations_RegisteredOrMarkedStagedenforces both directions, so a marker left on a served route fails as loudly as one cleared with no handler behind it. - No narrowed manifest projection.
ListContainerManifestsprojectsannotations, up to roughly 520 KB per row, which the resource withholds. Step 5 documented and accepted that, and the handler clampslimitto 100. Changing it needs a row type of its own, in the store. - No plan Status table edit. Three sibling steps would edit the same rows. It gets filled once, afterwards.
Pipeline state
Both failures this section previously described are gone on main. The
golangci_lint job failed on the integration branch because Steps 6 through 9
each crossed goconst's three-occurrence threshold only once the others were
present; those steps have landed and the findings were fixed with them, so
go-lint-ci ./... is clean here. The project-wide Danger gem break
(Base is not a class) is also fixed, so danger-review runs normally.
Known local-gate deviations
TestWireStorage_CloudCDNPresentincmd/artifact-registryfails locally with "could not find default credentials". Pre-existing, needs GCP ADC, andwire_storage_test.gois byte-identical to the target branch.golangci-lint --build-tags=integration ./cmd/artifact-registry/...reports 8 issues, all inwire_npm_*andwire_oci_boot_*files this diff does not touch. CI never lints integration files. The same run over./internal/managementapi/...is clean.
Merge-order notes for a sibling step
Six files carry edits Steps 11 and 12 also need. The four anchored regions of
wire_management.go and the per-step blocks in handler.go (the Deps struct,
requireDeps, and the route table) should merge cleanly.
The rest will not. An actual merge of Steps 10 and 11 conflicts at all three
test construction sites, and handler_test.go conflicts for certain: Step 11
adds the same implemented field and sweep guard with different comments, and
Step 12 uses a name switch rather than the flag, so the two mechanisms do not
converge without a hand merge. Whoever merges second should expect to resolve
those by hand rather than trusting a clean apply.
wireManagementAPI's doc comment also lost its "no artifact store is
constructed here yet" clause, which this step made false.
Heads-up on a landed-after dependency: refactor(datastore): jet-convert container_mani... (!1230 - merged) • Hayley Swimelar • 19.3
narrows the manifests-list row type, which retypes this MR's manifest seam and
stales the annotations sentence in container_resources.go. That MR promises
to absorb the retype, so nothing is owed here, but the next reader of this seam
should know it moves.
Related to #312 (closed)