chore(managementapi): add the bulk delete selector decode
Why
Six bulk-delete routes each accept one of two selector bodies, and all six need the identical strict decode: exactly one of a 1-1,000-entry list or delete_all: true, entries spelled per selector, repeats applied as a set. Landing that decode once, ahead of the routes that consume it, is what keeps six handlers from growing six copies of it.
Nothing routes to it yet. The bulk-route steps wire the six selectors, so this MR ships no behavior a caller can reach.
Step 27 of the S17 Phase 4 plan, against the S17 spec.
The spec was silent on repeated property names, so this MR states the rejection in it: the body-shape paragraph of Bulk delete, its acceptance criterion, and the bad-request row of Error Cases. Numbering is unchanged, the criterion is rewritten in place.
What
Three calls a reviewer would otherwise have to reverse-engineer:
- The body decodes into
map[string]json.RawMessage, not a per-route struct. The accepted key varies by route, and "exactly one of two keys" is a property of the object's key set rather than of a Go type. One consequence is load-bearing: the strict decoder's unknown-field check does not apply to a map, so the key count and lookup are what reject a key the route does not accept. - The entry list is read under the cap, not built and then measured. Building it first cost heap proportional to the body rather than to the cap: 94 MB allocated for a 7 MB body against a 512Mi pod limit, so a few concurrent requests exceeded that limit on heap alone. Reading under the bound costs 24 MB on the same body.
internal/jsonsafe's package doc names this payload shape and prescribes this element-count cap. - The all-zeros UUID is refused even though it spells canonically. An entry naming no artifact is otherwise a no-op, but the stores answer a zero id with a validation error rather than the not-found sentinel, so admitting one would accept a batch no worker can resolve after the caller already holds its
202. - A repeated property name is refused, and the selector reads its own object to do it. The shared object decode is the
encoding/jsonv1 shim, which keeps the last of two members, so{"delete_all": false, "delete_all": true}emptied a collection while its body opened by saying it did not. Names compare with escapes resolved, so the plain and escaped spellings of one name repeat each other in either order. Failures still route throughtransport.ClassifyDecodeFailure, so the size-cap413is unchanged and no other caller ofDecodeJSONBodymoves in this MR.
Size
1,513 added lines against the 500 ceiling: 420 production, 1,089 test, and 4 spec. The remainder is the decode matrix: six selectors across both branches, the cap boundary from both sides, each selector's entry grammar, and the allocation budgets that pin the two bounded reads. Splitting it would separate the decode from the table that defines it.
Spec coverage
Scoped to the selector decode. decode reads the request body and nothing else, so every route, worker, schema, and contract row of S17 belongs to another step of the plan and travels with that step's MR.
Acceptance criteria
| # | Criterion | Tests |
|---|---|---|
| AC-47 | Every bulk selector takes the identifier its artifact's own resource is addressed by, and a non-canonical UUID, a rejected tag name, or a non-canonical digest returns 400 |
TestBulkSelectorSpecs_MapEachRoutePropertyToItsEntryRule, TestBulkSelectorSpec_Decode_AcceptsASubsetOfItsOwnEntryType, TestBulkSelectorSpec_Decode_RejectsAnotherRoutesListProperty, TestBulkSelectorSpec_Decode_RejectsEntriesItsGrammarRejects |
| AC-55 | A batch of exactly the cap is accepted, one over is rejected including when the excess is a duplicate, and an empty, omitted, or null list is rejected | TestBulkSelectorSpec_Decode_ListLengthBounds (each rejecting row asserts which rule answered it). The 202 half is the route steps'. |
| AC-56 | A batch of repeated entries is applied as a set | TestBulkSelectorSpec_Decode_CollapsesDuplicatesInFirstSeenOrder (order, not just count), TestBulkSelectorSpec_Decode_ListLengthBounds (the cap of one repeated entry). The 202 half is the route steps'. |
| AC-58 | Entry values and list length are validated separately from the body shape | TestBulkSelectorSpec_Decode_ListLengthBounds, TestBulkSelectorSpec_Decode_RejectsEntriesItsGrammarRejects. The oneOf and generated-client half is the contract steps'. |
| AC-59 | A body carrying both properties, neither, or "delete_all": false returns 400, as does a JSON type mismatch, and so does a repeated property name |
TestBulkSelectorSpec_Decode_RejectsBodiesMatchingNeitherBranch, TestBulkSelectorSpec_Decode_AcceptsDeleteAllOnEveryRoute, TestBulkSelectorSpec_Decode_RejectsARepeatedPropertyName, TestBulkSelectorSpec_Decode_AcceptsAnEscapedPropertyName |
| AC-64 | A non-canonical UUID and a rejected tag name return 400, and no failure response echoes any submitted entry |
TestBulkSelectorSpec_Decode_RejectsEntriesItsGrammarRejects (every row asserts the no-echo property and which rule rejected it), TestIsCanonicalUUIDEntry, TestIsCanonicalUUIDEntry_NarrowsBeyondUUIDParse, TestIsCanonicalUUIDEntry_RejectsTheAllZerosUUID, TestIsContainerTagNameEntry, TestIsCanonicalDigestEntry |
| AC-66 | A tag named delete_all, all, or bulk_delete is an ordinary tag name |
TestBulkSelectorSpec_Decode_TakesSelectorSpellingsAsOrdinaryTagNames, TestIsContainerTagNameEntry. The tag-list and removal half is the tag steps'. |
Error cases
| Condition | Status | Code | Tests |
|---|---|---|---|
| Malformed JSON body | 400 |
bad_request |
TestBulkSelectorSpec_Decode_RejectsBodiesMatchingNeitherBranch (non-object, empty, trailing data), TestBulkSelectorSpec_Decode_NonObjectBody_NamesNoGoType |
Body matching neither selector branch: both properties, neither, or "delete_all": false |
400 |
bad_request |
TestBulkSelectorSpec_Decode_RejectsBodiesMatchingNeitherBranch |
Empty, omitted, or oversized list, a non-canonical UUID, a rejected tag_names entry, a JSON type mismatch, an unknown field, a repeated property name, or trailing data |
400 |
bad_request |
TestBulkSelectorSpec_Decode_ListLengthBounds, TestBulkSelectorSpec_Decode_RejectsEntriesItsGrammarRejects, TestBulkSelectorSpec_Decode_RejectsBodiesMatchingNeitherBranch, TestBulkSelectorSpec_Decode_RejectsAnotherRoutesListProperty, TestBulkSelectorSpec_Decode_RejectsARepeatedPropertyName, TestBulkSelectorSpec_Decode_RejectsElementsThatAreNotStrings |
| Body over the request size cap | 413 |
request_entity_too_large |
TestBulkSelectorSpec_Decode_OversizedBodyWithoutContentLength_Returns413 (the chunked path, where the cap trips mid-read) |
| Job backend unavailable at enqueue | 503 |
service_unavailable |
Owned by the enqueue seam step. Decode never reaches the job backend. |
Slug resolves to no namespace, auth 401 and 403, method 405 |
- | - | Router and middleware. Not reachable from a body decode. |
Security considerations
| Concern | Tests |
|---|---|
| Echoed input: no failure repeats a submitted entry | TestBulkSelectorSpec_Decode_RejectsEntriesItsGrammarRejects (per row), TestBulkSelectorSpec_Decode_NonObjectBody_NamesNoGoType extends it to the Go decode-target type |
| Input validation before any database access | Every test drives decode with no store, namespace, or mux in reach, so a decode that touched one would not compile against them |
| Bounded blast radius: both reads are bounded, not just checked afterwards | TestBulkSelectorSpec_Decode_OverCapListBoundsItsAllocation (the entry list), TestBulkSelectorSpec_Decode_ManyMemberObjectBoundsItsAllocation (the enclosing object) |
| Authentication, authorization, existence hiding, tenant isolation | Not reached: the decode holds no namespace, no path value, and no query. Covered by the route and handler steps. |
Test plan
go test ./internal/managementapi/ and -race, plus golangci-lint in both the default and --build-tags=integration modes. No database, no rig: the decode reaches its whole contract without a mux, a namespace, or a store.
Context for LLM agents
Rationale
- Per-route decode struct with
DisallowUnknownFields. Rejected: six near-identical structs, and a struct admits a sibling route's property through the decode before any check can reject it, so the exactly-one rule would still be hand-written on top. []stringunmarshal then bound the length. Rejected on measurement: 94 MB allocated for a 7 MB body versus 24 MB read under the bound, against a 512Mi pod limit with noGOMEMLIMITand no in-flight limiter.- Reject an over-long raw list by byte length before decoding. Rejected as unsound: whitespace and
\uXXXXescapes make a legal in-cap list arbitrarily long in bytes, so the bound would refuse valid batches on a destructive route. internal/jsonsafefor the bound. It does not fit: its own package doc says it bounds nesting depth and object-key count only, never flat-array width, and names this shape as the case needing a caller-supplied element-count cap.- Adopting
oci.ParseDigest. Rejected: it accepts uppercase hex and canonicalizes it, and an alias spelling of an identifier is refused here rather than silently rewritten.
Non-goals
- A machine-readable discriminator for the four rejection classes. All four are
400 bad_requestdiffering only in message, so a route cannot log which rule an integrator broke without string-comparing. Adding one means wideningrequestError, which this package's other handlers share. - Extracting the container tag grammar into a leaf package both
managementapiandinternal/format/ociimport.internal/format/oci/ocirouteis the precedent, and the extraction needs a third file. - The spec's claim that a bulk body is validated before any database round trip.
slugMiddlewareresolves the slug before every handler, so the round trip is already spent. The code comment says what is true, and the spec sentence needs its own patch.
Related to #313 (closed)