test(managementapi): pin the nil UUID 404 on the container id routes
Why
parsePathID (internal/managementapi/artifact.go) rejects uuid.Nil for every artifact id route, and that clause is load-bearing rather than defensive. The all-zeros UUID spells canonically, so the String() round trip admits it, and the artifact stores answer a zero id with a sentinel that does not wrap datastore.ErrNotFound. Delete the clause and the handlers render a logged 500 for a URL any client can type, where the contract promises an existence-hiding 404.
The version/file and package/dist-tag route families assert that at the route level. The four container routes binding {image_id} did not. Deleting the clause left their suites green, so the guard was unprotected on those four routes.
The container behavior was already correct, so this closes a coverage gap rather than fixing a defect, and it changes no production files.
What
Four things a reviewer will misread from the diff alone.
- The load-bearing change is in the fakes, not the assertions. The container fakes answered any unmatched id with wrapped
datastore.ErrNotFound, so a nil-id assertion would have passed with or without the guard.errContainerZeroIDplus two zero-id branches infake_container_readers_test.gorestate the production stores' sentinels (errContainerImageZeroID,errZeroImageTag), which is what makes the new assertions falsifiable. - The
contract_test.goimage-detail row is a rename, not new coverage. That row already sentcontainerImagePathFor(uuid.Nil)and passed under the mutation, so it was vacuous, and its bare name hid that. - The two container list fakes deliberately carry no zero-id guard. A list runs only after
findContainerImageForRequestreturns a row, and neither store nor fake can return one for a zero id, so a guard there is unreachable through the handlers.errContainerZeroID's doc comment carries the full rationale. - Scope is 7 test files, not the 3 the issue names. The fake file is where the load-bearing change goes. The contract sweep, the integration file, and
handler_test.goare deliberate additions on top of the issue's three.handler_test.gomatters becausenewTestDeps()handed out a zero repository id with a nil error, which the new fake branch would otherwise trip.
Nil-UUID 404 coverage on the container {image_id} routes
Every row below is a route-level assertion that the all-zeros UUID answers 404, not a 500. All of them fail when the id == uuid.Nil clause is deleted from parsePathID (internal/managementapi/artifact.go).
| Route | Unit (fakes) | Contract sweep row | Integration (real stores) |
|---|---|---|---|
image detailGET .../{format}/images/{image_id} |
TestContainerImageDetail_NotFound_IsOneResponse/nil_uuid |
image detail 404 nil image id |
image detail with the nil id |
tag listGET .../images/{image_id}/tags |
TestContainerTagList_NilImageID_Returns404 |
tag list 404 nil image id |
tag list with the nil image id |
tag detailGET .../images/{image_id}/tags/{tag_name} |
TestContainerTagDetail_NotFound_IsOneResponse/nil_image_id |
tag detail 404 nil image id |
tag detail with the nil image id |
manifest listGET .../images/{image_id}/manifests |
TestContainerManifestList_ParentResolution/nil_image_id |
manifest list 404 nil image id |
manifest list with the nil image id |
The contract rows live in TestContainerHandlers_ResponsesMatchOpenAPIContract and the integration rows in TestContainerReadsIntegration_NilImageID_Returns404.
parsePathID's rule itself is covered at the function level by the pre-existing TestParsePathID_RejectedSpellings_Returns404/nil_uuid. The gap this change closes is per-route, not per-function: that test pins the rule in isolation while all four container routes reached their store with a nil id undetected.
What each layer adds:
- Unit. Image detail and tag detail inherit their tables' byte-identical-body comparison, so the nil id is provably indistinguishable from an unknown id. The two list tests also assert the image finder and the list store are never reached.
- Contract. The two list rows are the first
404-envelope validation on those routes.openapi3filter.ValidateResponseaccepts a500on all four routes, so what catches the regression here is thewantStatusprecondition, not contract validation. - Integration. The only layer that proves the production sentinels (
errContainerImageZeroID,errZeroImageTag) sit outsidedatastore.ErrNotFound. The fakes model that, and these tests prove the model.
Test plan
Delete id == uuid.Nil from parsePathID and run go test ./internal/managementapi/ -count=1. Seven top-level tests and 12 leaves fail. Narrowing with -run TestContainer gives 5 and 8. With -tags=integration, TestContainerReadsIntegration_NilImageID_Returns404 fails as one top-level test and four named subtest leaves, one per route, while the other 5 TestContainerReadsIntegration_* tests pass. Restore the clause and everything is green. Measured on the final tree.
Context for LLM agents
Rationale
- Guarding the two container list fakes against a zero id. Rejected because the guard is unreachable through the handlers, and it would answer a loud 500 rather than an empty list. Neither list handler's store-error branch consults
datastore.ErrNotFound, so naming a sentinel there buys nothing. - Porting
TestPackageSubtreeRoutes_ChildRowAbsentto the container routes. Rejected because a container child-row miss is a deliberate logged 500 percontainer_list.go'sTODO(#506), so the ported assertion would pin the wrong contract. - Folding the zero id into
fakeContainerRepositoryFinder's notFound branch, the wayfakeMavenRepositoryResolverdoes. Rejected because it changes a fake every container test uses. The one-field fix inhandler_test.gocovers the same case.
Non-goals
- Asserting the 404
Error.Message. No container test assertsError.Messageanywhere, so awriteArtifactNotFoundtowriteRepositoryNotFoundswap still passes. That is a package-wide convention, so it belongs in a change of its own rather than here. - Adding
assertErrorEnvelopeto the integration loop. It is unused across all four read-integration files, and the three sibling 404 assertions there are equally exposed. - Pinning
parsePathID's position relative to repository resolution. Unpinned in this suite and in the version/file mirror alike. - Tightening
runContractOperation's tolerance of a 500.
Related to #522 (closed)