Repository navigation
feat: serve query and rest-xml from the generic CRUD engine - #143
Merged
Merged
Conversation
github-actions
Bot
added
documentation
RED: TestEncodeXMLEnvelopes / TestEncodeXMLValues (9 subtests) / TestEncodeXMLIsDeterministic / TestEncodeXMLDeclaresItsHeader all fail against an unimplemented encodeXML — got "", want the envelope. The engine's only serializer is okJSON, so query and rest-xml have no success path at all, which is why both are registered-only. The stub keeps the tree compiling (pre-commit runs go vet) so the RED is a real assertion diff rather than a build error. Decisions pinned here rather than discovered later: query gets the <OpResponse><OpResult> envelope botocore's query parser looks for, rest-xml the bare <OpResult>; lists wrap in <member>; an empty list is an empty element, not an omitted key; keys are sorted for byte stability; dotted query keys are dropped; values are escaped.
GREEN: all 13 encodeXML tests pass (4 top-level, 9 value subtests); go test ./... fully green. One encoder, two dialects. query gets <OpResponse><OpResult> plus a ResponseMetadata/RequestId, because botocore's query parser reads the result out of that nesting and returns an empty one without it; rest-xml gets the bare <OpResult>, because its parser maps the root's children onto the output shape and ignores the root's name. No xmlns: botocore strips namespaces before matching element names, so declaring one is bytes nothing reads. If that changes it belongs on OpMeta from the model's trait, not guessed at in the encoder.
…edicate
RED: Servable("rest-xml") = false, want true; NeedsBody("query") =
false, want true; TestEngineRESTXMLRoundTrip and
TestEngineRESTXMLContentType fail.
Replaces TestEngineRESTXMLStillDeclines, which pinned the old
boundary. rest-xml is not on the wrong side of it: all 97 of
s3-control's operations are bound to a method and URI template, so the
route table that classifies rest-json classifies it too. What survives
is TestEngineQueryStillDeclines, until Task 4.
The new predicate is the point of this task. "Can the engine serve
this protocol" and "must the gateway buffer the body" are different
questions, and rest-xml is where they come apart: S3 speaks it, and S3
bodies are large binary uploads that must keep streaming.
GREEN: go test ./... fully green; make build clean; codegen idempotent (identical hashes across two runs); imports.go unchanged. rest-xml was excluded on the assumption that it had no modelled path. It has one — all 97 of s3-control's operations carry a method and URI template — so the route table that classifies rest-json classifies it too. 94 operations move unimplemented -> auto-crud. Blast radius is exactly one service, measured rather than assumed. s3, cloudfront and route53 gain registry entries but not a single manifest change, because BuildFidelityData gates auto-crud on EngineWired and none of their providers returns ErrUnhandledOp. The gate written for this case did its job. Servable and NeedsBody are now separate questions and the router asks the second one. The engine serves rest-xml without the body, which is what keeps S3's large binary uploads streaming; a test double that fails on Read proves the gateway never touches them.
RED: Servable("query") = false, want true; TestEngineQueryRoundTrip,
TestEngineQueryDropsProtocolFields, TestEngineQueryBodyIsNotJSON and
TestEngineQueryContentType all fail.
query is the last protocol and the odd one: its operation is neither a
header nor a path but the Action field of a form body. Two traps are
specified rather than left to be hit — Action and Version describe the
request, not the resource, so they must not be stored and echoed back
as members; and Handle's JSON body parse must not treat a form-encoded
body as a malformed JSON one, which would decline every query call
with a SerializationException.
GREEN: go test ./... fully green; make build clean; codegen idempotent. query is the last protocol and the odd one out. Its operation is neither a header nor a path but the Action field of a form body, which is why it outlived rest-json and rest-xml as a gap — and why it registers no route: a query operation has no method or URI at all. 18 operations move unimplemented -> auto-crud, all in elasticloadbalancing. Thirteen other query services gain registry entries and not one manifest change, including elbv2 sitting directly beside it, because EngineWired gates the label and none of their hand-written providers returns ErrUnhandledOp. Two traps closed by construction rather than by luck: Action and Version are dropped before storage, so a created resource does not echo <Action> back as a member; and the JSON body parse is now an else-branch, because reaching it with a form-encoded body would have declined every query call with a SerializationException.
GREEN: 854 passed, 2 skipped (was 846/3); go test ./... green. Both leave REGISTERED_ONLY_SERVICES. elb joins the generic smoke list; s3control cannot, and that is a finding rather than a test to loosen — all 97 of its operations take a required AccountId, so the harness has no parameterless read to pick. It is proved in test_s3control.py instead, the budgets precedent. These tests assert on parsed members, not status codes, because the failure mode they exist to catch is silent: botocore's query parser given the wrong envelope returns a result with nothing in it rather than an error, so a 200 whose body it discards is indistinguishable from a working service on the Go side of the wire. Both files also pin the edge of the claim. ConfigureHealthCheck is one of ELB's 11 unclassifiable operations and a Terraform aws_elb resource calls it; it must decline with a parseable code, not an empty success.
205 registered / 201 serving >=1 / 4 registered-only, and 4,496 hand-verified / 4,970 auto-crud / 2,941 unimplemented. Every number is read out of internal/generated/fidelity/manifest_gen.go, not out of the plan; go test ./cmd/devcloud/ asserts the manifest against the live registry. The interesting edit is a deletion. coverage.md listed two reasons a registered service can serve nothing; one of them is gone. Protocol is no longer a reason, so the four that remain are all the same case — no CRUD-shaped operation anywhere in their API — and the page says so rather than leaving a category with nothing in it. crud-engine.md documents the limits each new protocol brings, because they are real: rest-xml request bodies are never read, query keeps flat form keys only and drops Action and Version, lists are always <member>-wrapped, no xmlns is emitted. The PRD retires the non-JSON risk as closed rather than as excluded.
errcheck flagged both xml.EscapeText calls. The error is the Writer's and a strings.Builder never fails a write, so the discard is explicit with the reason attached rather than implicit. Found by running golangci-lint locally: every CI workflow in this repo is gated on `pull_request: branches: [main]`, so a PR stacked on another branch gets no CI at all beyond the labeler.
skyoo2003
force-pushed
the
feat/non-json-crud-engine
branch
from
September 5, 2026 17:18
64dc8d1 to
9804d7b
Compare
5 tasks done
skyoo2003
added a commit
that referenced
this pull request
Sep 5, 2026
…ions GREEN: 126 passed, 17 skipped, 2 xfailed. The six failures this change targets are gone; the two that remain are different defects, marked xfail(strict) with what was observed rather than a guessed cause. The default branch of each provider returned 200 with an empty <ActionResponse/> for any action it did not implement. Two consequences, both bad: the caller was told an operation succeeded when nothing ran, and the body carried no <ActionResult> wrapper, so botocore's query parser raised KeyError instead of returning a value. neptune.describe_db_cluster_endpoints() crashed the SDK outright. Each now returns plugin.ErrUnhandledOp, which is what every other provider in the fleet does and what the CRUD engine has been reachable through since query was admitted in #143. 223 operations move from unimplemented to auto-crud — they are served from the real store now — and the rest decline with InvalidAction. docs/coverage.md's tier table moves with them: auto-crud 4,970 -> 5,193, unimplemented 2,941 -> 2,718, total unchanged. That edit was not optional; the gate added in #144 failed until the published figures matched the manifest, naming both numbers.
5 tasks done
skyoo2003
added a commit
that referenced
this pull request
Sep 5, 2026
…ions (#145) * test: add reproducer for fabricated successes on unserved operations RED: 8 failed, 120 passed, 17 skipped. FAILED [autoscaling.DescribeAdjustmentTypes] FAILED [cloudformation.DescribeChangeSetHooks] FAILED [elasticache.DescribeCacheEngineVersions] FAILED [elasticloadbalancingv2.DescribeCapacityReservation] FAILED [rds.DescribeBlueGreenDeployments] FAILED [redshift.DescribeAccountAttributes] FAILED [resourcegroups.Tag] FAILED [s3.ListBucketAnalyticsConfigurations] Each returned HTTP 200 for an operation the fidelity manifest does not list as served. docs/coverage.md calls that the one thing DevCloud must never do. test_service_smoke.py already asserted this for services that serve nothing at all. The gap was the other case: a service that serves plenty and answers an operation it does not implement with an empty success anyway. * fix: eight query providers fabricated a success for unimplemented actions GREEN: 126 passed, 17 skipped, 2 xfailed. The six failures this change targets are gone; the two that remain are different defects, marked xfail(strict) with what was observed rather than a guessed cause. The default branch of each provider returned 200 with an empty <ActionResponse/> for any action it did not implement. Two consequences, both bad: the caller was told an operation succeeded when nothing ran, and the body carried no <ActionResult> wrapper, so botocore's query parser raised KeyError instead of returning a value. neptune.describe_db_cluster_endpoints() crashed the SDK outright. Each now returns plugin.ErrUnhandledOp, which is what every other provider in the fleet does and what the CRUD engine has been reachable through since query was admitted in #143. 223 operations move from unimplemented to auto-crud — they are served from the real store now — and the rest decline with InvalidAction. docs/coverage.md's tier table moves with them: auto-crud 4,970 -> 5,193, unimplemented 2,941 -> 2,718, total unchanged. That edit was not optional; the gate added in #144 failed until the published figures matched the manifest, naming both numbers.
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
1 participant
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 generic CRUD engine now serves every protocol DevCloud registers.
rest-xmlandquerywere the last two, and they were the whole of PRD Milestone 5 — "Query / EC2 Query / REST-XML services within the demand set either meet a stated floor or are excluded from the published claim."They are served, not excluded. The milestone permitted either; the measurement decided it.
Related Issue
Refs #
auto-crudoperationsunimplementedoperationsThe 112 moved operations are 94 in
s3-controland 18 inelastic-load-balancing. Protocol is no longer a reason any registered service serves nothing — the four that remain have no CRUD-shaped operation anywhere in their API.Changes
rest-xmlwas excluded on a false premise. It was assumed to have no modelled path. It has one: all 97 ofs3-control's operations carry a method and URI template, so the same route table that classifiesrest-jsonclassifies it.querygenuinely is different — its operation is theActionfield of a form body, not a header or a path — so it registers no route and the engine matches it from the body instead.internal/shared/crud/xml.go— the one genuinely new thing: amap[string]any→ AWS XML encoder. Both protocols needed it; neither needed anything else.crud.Servable/crud.NeedsBodyare now separate predicates, and the router asks the second one. See below.internal/codegen/gen_crud_meta.go—engineServableadmits both. This gate andcrud.Servableanswer the same question and a test now holds them together.The blast radius is exactly two services, measured not assumed
Of the 20 in-tree
awsQuery/ec2Query/restXmlmodels, onlyelasticloadbalancingands3controlreturnplugin.ErrUnhandledOp. Every other one —s3,cloudfront,route-53,iam,sts,rds,ec2,elbv2, … — answers its own unknown operations and never enters the engine.They gained CRUD registry entries and produced zero manifest changes, because
BuildFidelityDatagatesauto-crudonEngineWired. The gate written for exactly this case did its job, and the fleet-wide flip that dominated the rest-json change (#139) did not happen here.rest-xmlbodies are still never bufferedS3 speaks
rest-xmland its bodies are large binary uploads. The engine servesrest-xmlwithout the body — every CRUD-shaped S3 Control operation addresses its resource with a path label or query term — so what used to be a side effect of the protocol being unservable is now an explicit rule,crud.NeedsBody, with a test double that fails if the gateway reads it.Test Plan
Every change went RED first. Two RED runs worth quoting:
That is the regression the predicate split exists to prevent, caught before it shipped.
GREEN:
Coverage on the changed packages:
httproute96.8%,crud86.5%,gateway84.8%,codegen83.8%.Manual, against
./dist/devcloud:The boto3 tests assert on parsed members, not status codes, because the failure they exist to catch is silent: botocore's query parser given the wrong envelope returns a result with nothing in it rather than an error, so a 200 whose body it discards looks identical to a working service from Go's side of the wire.
What this does not claim
Breadth, not depth — unchanged from the PRD's stated scope.
ConfigureHealthCheckandRegisterInstancesWithLoadBalancer, which a Terraformaws_elbresource does call. Tested to decline with a parseable code, not to work.s3controlis absent fromENGINE_SERVED_SERVICESand proved by its own file instead: all 97 of its operations require anAccountId, so the generic harness has no parameterless read to pick. Thebudgetsprecedent.ec2-queryis still not served, deliberately. Only EC2 speaks it and its provider is hand-written.Checklist
docs/coverage.md,docs/crud-engine.md)