Repository navigation
feat: serve rest-json from the generic CRUD engine - #139
Merged
Merged
Conversation
The matcher lived only inside router.go.tmpl, so it was emitted into 148 generated packages and could not be imported. The CRUD engine needs the same answer from the same data to resolve a rest-json operation, and writing it a 149th time is how the two would drift. PathParams and OperationRoute become type aliases, so the four hand-written callers (cloudfront, efs and their tests) compile and pass unchanged — that is the behavioural proof this is a refactor. RED: go test ./internal/shared/httproute/ failed to build — match_test.go:12:11: undefined: Params match_test.go:85:15: undefined: MatchURI match_test.go:163:14: undefined: Route FAIL ... [build failed] (a standalone RED commit is impossible here: the repo's pre-commit go-vet hook refuses a non-compiling tree, so the evidence is recorded here.) GREEN: go test ./internal/shared/httproute/ -v — 20/20 subtests PASS. Full suite: CGO_ENABLED=0 go test ./... all pass. gen_router_test.go swaps its "type OperationRoute struct" assertion for one on the delegation, which is what stops the algorithm being re-inlined. Net -16,728 lines across 136 regenerated routers.
rest-json carries no X-Amz-Target header, so the engine had nothing to classify and 59 registered services served nothing. The operation was always recoverable: every rest-json operation is bound to a method and URI template in the model, and the generated per-service routers have matched on that pair since day one. The gateway just threw the answer away. crud.OpMeta now carries the REST binding, crud.Handle takes a Call and resolves the operation through internal/shared/httproute, and the gateway buffers rest-json bodies so a Create sees its parameters. rest-xml stays unbuffered: S3 bodies are large binary uploads and the engine refuses the protocol anyway. Fleet effect, before a single new service is added: services serving >=1 op 117 -> 145 registered-only 31 -> 3 auto-crud operations 1,415 -> 2,200 unimplemented 3,119 -> 2,334 The 3 that remain are not a protocol problem: forecastquery and the two SageMaker Runtime variants have no CRUD-shaped operation at all. RED (engine): go test ./internal/shared/crud/ — undefined: Call; unknown field Method/URI in struct literal of type OpMeta. FAIL [build]. RED (codegen): go test ./internal/codegen/ -run TestServiceCRUDData — crudOpData has no field or method Method/URI. FAIL [build]. RED (gateway): TestServiceRouter_RESTJSONBodyReachesEngine — "request body did not reach the engine": expected "g1", actual <nil>. The other two rest-json gateway cases already passed, which is what showed routing was live and only buffering was missing. GREEN: go test ./... all pass; make test-compat 790 passed, 2 skipped. Also: the fidelity manifest now publishes EngineWired per service. TestFidelityManifestCoversCRUDRegistry assumed registry membership implied the engine serves an operation; that broke honestly on apigatewayv2 and xray, whose hand-written providers never return ErrUnhandledOp and so are never routed to the engine. The manifest was right and the test was inferring — it now reads the fact instead. polly, qbusiness and codeguru-reviewer move to ENGINE_SERVED_SERVICES; personalize-runtime leaves REGISTERED_ONLY_SERVICES (GetRecommendations classifies as a Get) but has no parameterless read to smoke-test with, so the manifest is its record.
5 tasks done
5 tasks done
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
The CRUD engine could only read the
X-Amz-TargetJSON protocols, so 59 registeredrest-jsonservices were routed but served nothing. The operation was always recoverable — everyrest-jsonoperation is bound to a method and URI template in the model, and the generated per-service routers have matched on that pair since day one. The gateway just threw the answer away.This PR adds no services. It changes what the engine can read, and 28 already-registered services stop being registered-only as a result.
Related Issue
Refs
.claude/prds/aws-service-coverage-100.prd.md— answers Open Question 1 ("Does the claim include non-JSON-protocol services?"). Yes forrest-json;queryandrest-xmlstay outside and are Milestone 5.Fixes #
Changes
internal/shared/httproute(new) — the URI matcher lifted out ofrouter.go.tmpl, where it was emitted into 148 generated packages and could not be imported.PathParamsandOperationRoutebecame type aliases, socloudfront,efsand their tests compile and pass unchanged. Net −16,728 lines across 136 regenerated routers.internal/shared/crud—OpMetacarries the REST binding;Handletakes aCalland resolves the operation throughhttproute; parameters are read from query, body, then path labels, with the path label authoritative because the URI is what addresses the resource.internal/codegen—isJSONProtocolbecomesengineServableand admitsrest-json; the CRUD registry carries each operation's method and URI.internal/gateway— body buffering moves fromcrud.JSONProtocoltocrud.Servable.rest-xmlstays unbuffered: S3 bodies are large binary uploads and the engine refuses the protocol anyway.Fidelity manifest — now publishes
EngineWiredper service.TestFidelityManifestCoversCRUDRegistryassumed CRUD-registry membership implies the engine serves an operation; that broke honestly onapigatewayv2andxray, whose hand-written providers never returnErrUnhandledOpand so are never routed to the engine. The manifest was right and the test was inferring — it now reads the fact.Fleet effect, before a single new service is added
auto-crudoperationsunimplementedoperationsThe 3 that remain are not a protocol problem:
forecastqueryand the two SageMaker Runtime variants have no CRUD-shaped operation at all.Test Plan
CGO_ENABLED=0 go test ./...— all passmake test-compat— 790 passed, 2 skippedrm -rf internal/generated && make codegen— no drift (matches the CIcodegen-driftjob)RED evidence (a standalone RED commit is impossible here: the repo's pre-commit
go vetrefuses a non-compiling tree, so it is recorded in the commit bodies):go test ./internal/shared/crud/:undefined: Call;unknown field Method in struct literal of type OpMeta;FAIL [build failed]go test ./internal/codegen/ -run TestServiceCRUDData:crudOpData has no field or method Method/URI;FAIL [build failed]TestServiceRouter_RESTJSONBodyReachesEngine:"request body did not reach the engine": expected "g1", actual <nil>. The other two rest-json gateway cases already passed, which is what showed routing was live and only buffering was missing.End-to-end:
polly,qbusinessandcodeguru-reviewerfailedtest_registered_only_service_declines_cleanlywith "returned 200 but nothing serves this service" — the harness correctly detecting that they now serve. They moved toENGINE_SERVED_SERVICESin the same commit.New tests worth reading:
TestEngineRESTJSONUnmatchedPathIsUnclassifiedandTestServiceRouter_RESTJSONUnmatchedPathDeclinesCleanly— a path the route table does not know must decline withInvalidAction, never a fabricated success. That guarantee is what the whole coverage claim rests on.Checklist
golangci-lint run)docs/coverage.md