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.

  1. 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. errContainerZeroID plus two zero-id branches in fake_container_readers_test.go restate the production stores' sentinels (errContainerImageZeroID, errZeroImageTag), which is what makes the new assertions falsifiable.
  2. The contract_test.go image-detail row is a rename, not new coverage. That row already sent containerImagePathFor(uuid.Nil) and passed under the mutation, so it was vacuous, and its bare name hid that.
  3. The two container list fakes deliberately carry no zero-id guard. A list runs only after findContainerImageForRequest returns 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.
  4. 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.go are deliberate additions on top of the issue's three. handler_test.go matters because newTestDeps() 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 detail
GET .../{format}/images/{image_id}
TestContainerImageDetail_NotFound_IsOneResponse/nil_uuid image detail 404 nil image id image detail with the nil id
tag list
GET .../images/{image_id}/tags
TestContainerTagList_NilImageID_Returns404 tag list 404 nil image id tag list with the nil image id
tag detail
GET .../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 list
GET .../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.ValidateResponse accepts a 500 on all four routes, so what catches the regression here is the wantStatus precondition, not contract validation.
  • Integration. The only layer that proves the production sentinels (errContainerImageZeroID, errZeroImageTag) sit outside datastore.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_ChildRowAbsent to the container routes. Rejected because a container child-row miss is a deliberate logged 500 per container_list.go's TODO(#506), so the ported assertion would pin the wrong contract.
  • Folding the zero id into fakeContainerRepositoryFinder's notFound branch, the way fakeMavenRepositoryResolver does. Rejected because it changes a fake every container test uses. The one-field fix in handler_test.go covers the same case.

Non-goals

  • Asserting the 404 Error.Message. No container test asserts Error.Message anywhere, so a writeArtifactNotFound to writeRepositoryNotFound swap still passes. That is a package-wide convention, so it belongs in a change of its own rather than here.
  • Adding assertErrorEnvelope to 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)

Edited by Hayley Swimelar

Merge request reports

Loading
Loading