feat: generic Smithy-driven CRUD fallback engine for all-op coverage#98
Merged
Conversation
Contributor
Reviewer's GuideIntroduces a generic, Smithy-model-driven CRUD fallback engine for JSON-protocol services, wires selected providers into it via a new ErrUnhandledOp sentinel, and adds codegen, routing, tests, and documentation to support auto-served CRUD-shaped operations while preserving honest errors for unclassifiable ops. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
github-actions
Bot
added
documentation
skyoo2003
added a commit
that referenced
this pull request
Jul 18, 2026
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 5 issues, and left some high level feedback:
- The gateway’s JSON body buffering in
ServiceRouter.ServeHTTPcurrently ignoresio.ReadAllerrors and has no size guard; consider enforcing a reasonable max body size and returning a clear AWS-style error if the read fails or the payload is too large to avoid potential DoS or odd fallback behavior. - The CRUD engine’s in-memory
storeis keyed only byservice|resource, so data is effectively global across all callers; if you expect to simulate multi-account/region isolation, it may be worth threading through and incorporating account/region (or some other tenancy key) into the store key now before more services depend on this behavior.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The gateway’s JSON body buffering in `ServiceRouter.ServeHTTP` currently ignores `io.ReadAll` errors and has no size guard; consider enforcing a reasonable max body size and returning a clear AWS-style error if the read fails or the payload is too large to avoid potential DoS or odd fallback behavior.
- The CRUD engine’s in-memory `store` is keyed only by `service|resource`, so data is effectively global across all callers; if you expect to simulate multi-account/region isolation, it may be worth threading through and incorporating account/region (or some other tenancy key) into the store key now before more services depend on this behavior.
## Individual Comments
### Comment 1
<location path="internal/gateway/router.go" line_range="46-48" />
<code_context>
+ // Buffer the body for JSON protocols so the CRUD fallback engine can re-read
+ // it if the provider does not handle the operation. Cheap for JSON payloads;
+ // skipped for REST-XML (S3) where bodies may be large binary uploads.
+ var body []byte
+ if crud.JSONProtocol(protocol) {
+ body, _ = io.ReadAll(r.Body)
+ r.Body = io.NopCloser(bytes.NewReader(body))
+ }
</code_context>
<issue_to_address>
**issue (bug_risk):** The error from io.ReadAll is ignored, which can hide real I/O problems and still call the CRUD engine with an invalid body.
If io.ReadAll fails, we still continue with a zero-value body and no indication of an error, making a failed read indistinguishable from a valid empty payload and potentially confusing the CRUD engine. Please handle this error explicitly (e.g., return an appropriate 4xx/5xx and skip the plugin/CRUD call) or at least log it so transport-layer issues are visible.
</issue_to_address>
### Comment 2
<location path="internal/shared/crud/crud.go" line_range="61-64" />
<code_context>
+}
+
+// RegisteredOps returns the operation metadata registered for a service.
+func RegisteredOps(service string) map[string]OpMeta {
+ mu.RLock()
+ defer mu.RUnlock()
+ return registry[service]
+}
+
</code_context>
<issue_to_address>
**suggestion (bug_risk):** RegisteredOps returns the internal registry map by reference, allowing external mutation and possible data races.
Because this returns the underlying map, callers can mutate shared state without holding the mutex, creating potential data races if any goroutine writes while others read. To keep the concurrency model safe, consider returning a shallow copy of the map and documenting the result as read-only so callers cannot modify internal state directly.
Suggested implementation:
```golang
// RegisteredOps returns a read-only snapshot of operation metadata registered for a service.
// The returned map is a shallow copy and must not be modified by callers.
```
```golang
func RegisteredOps(service string) map[string]OpMeta {
mu.RLock()
defer mu.RUnlock()
ops, ok := registry[service]
if !ok || ops == nil {
return nil
}
// Return a shallow copy so callers cannot mutate shared state.
copied := make(map[string]OpMeta, len(ops))
for name, meta := range ops {
copied[name] = meta
}
return copied
}
```
</issue_to_address>
### Comment 3
<location path="internal/shared/crud/crud.go" line_range="249-251" />
<code_context>
+ }
+}
+
+func randHex(n int) string {
+ b := make([]byte, n)
+ if _, err := rand.Read(b); err != nil {
+ return "0000000000"
+ }
</code_context>
<issue_to_address>
**suggestion (bug_risk):** randHex silently falls back to a fixed value on crypto/rand failure, which can cause ID collisions and hide entropy problems.
If crypto/rand fails, returning a constant string causes all generated IDs to be identical and masks the underlying entropy issue. Even in a dev/emulation context, it would be safer to either fall back to a non-cryptographic but varying source (e.g., math/rand) or surface the error (log/propagate and fail the operation) rather than silently returning a fixed value.
Suggested implementation:
```golang
import (
"crypto/rand"
"log"
mrand "math/rand"
```
```golang
func randHex(n int) string {
b := make([]byte, n)
if _, err := rand.Read(b); err != nil {
// crypto/rand failing is serious; log it and fall back to a non-cryptographic source
log.Printf("crud: crypto/rand failed in randHex: %v; falling back to math/rand", err)
for i := range b {
b[i] = byte(mrand.Intn(256))
}
}
```
- Ensure `math/rand` is seeded somewhere in your process startup (e.g., in `main()` with `mrand.Seed(time.Now().UnixNano())`) so that the fallback values vary between runs.
- If the existing import block in `crud.go` already contains other imports or aliases, merge the added `log` and `mrand "math/rand"` entries into that block rather than replacing it verbatim.
</issue_to_address>
### Comment 4
<location path="tests/compatibility/test_unimplemented_ops.py" line_range="12-15" />
<code_context>
-
-
-def test_sagemaker_unimplemented_op_errors(sagemaker_client):
+def test_unwired_service_errors_on_unimplemented(codebuild_client):
+ # codebuild is not opted into the CRUD engine.
with pytest.raises(ClientError):
- sagemaker_client.list_auto_ml_jobs()
+ codebuild_client.list_build_batches()
</code_context>
<issue_to_address>
**suggestion (testing):** The tests only assert that a ClientError is raised; consider also checking error code/message to guard against regressions to fabricated success.
Since this test is our guardrail against "fabricated success", it would be stronger to assert on the specific error details, e.g. `e.response["Error"]["Code"]` (such as `InvalidAction`) and, if stable enough, part of the message. That way a future change that returns a different error or a silent 200 would be caught by this test rather than slipping through as a generic `ClientError` check.
</issue_to_address>
### Comment 5
<location path="internal/shared/crud/crud_test.go" line_range="81" />
<code_context>
+ }
+}
+
+func TestEngineUnclassified(t *testing.T) {
+ const svc = "othersvc"
+ Register(svc, map[string]OpMeta{"CreateWidget": {Verb: "Create", Resource: "Widget"}})
+
+ if _, err := handleJSON(t, svc, "SomeCustomOp", nil); err != ErrUnclassified {
+ t.Fatalf("unknown op: want ErrUnclassified, got %v", err)
+ }
+ if _, err := Handle(svc, "CreateWidget", "query", []byte("{}")); err != ErrUnclassified {
+ t.Fatalf("non-json protocol: want ErrUnclassified, got %v", err)
+ }
+ if _, err := handleJSON(t, "unregistered", "CreateThing", nil); err != ErrUnclassified {
+ t.Fatalf("unregistered service: want ErrUnclassified, got %v", err)
+ }
</code_context>
<issue_to_address>
**suggestion (testing):** It could be useful to add a test that verifies isolation between services/resources in the in-memory store.
Since the store is keyed by `service|resource`, please add a test that creates the same resource name in two different services (e.g. `CreateThing` under `svcA` and `svcB`) and verifies their entries don’t collide when listing/getting. This will help catch bugs in key construction and guard against future refactors that might leak state across services.
```suggestion
func TestEngineUnclassified(t *testing.T) {
const svc = "othersvc"
Register(svc, map[string]OpMeta{"CreateWidget": {Verb: "Create", Resource: "Widget"}})
if _, err := handleJSON(t, svc, "SomeCustomOp", nil); err != ErrUnclassified {
t.Fatalf("unknown op: want ErrUnclassified, got %v", err)
}
if _, err := Handle(svc, "CreateWidget", "query", []byte("{}")); err != ErrUnclassified {
t.Fatalf("non-json protocol: want ErrUnclassified, got %v", err)
}
if _, err := handleJSON(t, "unregistered", "CreateThing", nil); err != ErrUnclassified {
t.Fatalf("unregistered service: want ErrUnclassified, got %v", err)
}
}
func TestEngineServiceIsolation(t *testing.T) {
const (
svcA = "svcA"
svcB = "svcB"
)
ops := map[string]OpMeta{
"CreateThing": {Verb: "Create", Resource: "Thing", OutputItemKey: "Thing"},
"GetThing": {Verb: "Get", Resource: "Thing", OutputItemKey: "Thing"},
"ListThings": {Verb: "List", Resource: "Thing", OutputListKey: "Things"},
}
Register(svcA, ops)
Register(svcB, ops)
// Create distinct Things in each service
if r, _ := handleJSON(t, svcA, "CreateThing", map[string]any{"ThingName": "a"}); r.Status != 200 {
t.Fatalf("CreateThing svcA status=%d", r.Status)
}
if r, _ := handleJSON(t, svcB, "CreateThing", map[string]any{"ThingName": "b"}); r.Status != 200 {
t.Fatalf("CreateThing svcB status=%d", r.Status)
}
// svcA should only see Thing "a"
if r, _ := handleJSON(t, svcA, "ListThings", nil); r.Status != 200 {
t.Fatalf("ListThings svcA status=%d", r.Status)
} else {
body := decode(t, r)
rawThings, ok := body["Things"]
if !ok {
t.Fatalf("svcA response missing Things key: %#v", body)
}
things, ok := rawThings.([]any)
if !ok {
t.Fatalf("svcA Things has unexpected type %T", rawThings)
}
if len(things) != 1 {
t.Fatalf("svcA expected 1 Thing, got %d", len(things))
}
thing, ok := things[0].(map[string]any)
if !ok {
t.Fatalf("svcA Thing has unexpected type %T", things[0])
}
if got := thing["ThingName"]; got != "a" {
t.Fatalf("svcA expected ThingName a, got %#v", got)
}
}
// svcB should only see Thing "b"
if r, _ := handleJSON(t, svcB, "ListThings", nil); r.Status != 200 {
t.Fatalf("ListThings svcB status=%d", r.Status)
} else {
body := decode(t, r)
rawThings, ok := body["Things"]
if !ok {
t.Fatalf("svcB response missing Things key: %#v", body)
}
things, ok := rawThings.([]any)
if !ok {
t.Fatalf("svcB Things has unexpected type %T", rawThings)
}
if len(things) != 1 {
t.Fatalf("svcB expected 1 Thing, got %d", len(things))
}
thing, ok := things[0].(map[string]any)
if !ok {
t.Fatalf("svcB Thing has unexpected type %T", things[0])
}
if got := thing["ThingName"]; got != "b" {
t.Fatalf("svcB expected ThingName b, got %#v", got)
}
}
// Ensure GetThing is also isolated
if r, _ := handleJSON(t, svcA, "GetThing", map[string]any{"ThingName": "a"}); r.Status != 200 {
t.Fatalf("GetThing svcA status=%d", r.Status)
} else {
body := decode(t, r)
thing, ok := body["Thing"].(map[string]any)
if !ok {
t.Fatalf("svcA GetThing Thing has unexpected type %T", body["Thing"])
}
if got := thing["ThingName"]; got != "a" {
t.Fatalf("svcA GetThing expected ThingName a, got %#v", got)
}
}
if r, _ := handleJSON(t, svcB, "GetThing", map[string]any{"ThingName": "b"}); r.Status != 200 {
t.Fatalf("GetThing svcB status=%d", r.Status)
} else {
body := decode(t, r)
thing, ok := body["Thing"].(map[string]any)
if !ok {
t.Fatalf("svcB GetThing Thing has unexpected type %T", body["Thing"])
}
if got := thing["ThingName"]; got != "b" {
t.Fatalf("svcB GetThing expected ThingName b, got %#v", got)
}
}
}
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
skyoo2003
added a commit
that referenced
this pull request
Jul 18, 2026
skyoo2003
force-pushed
the
feat/phase1-op-depth
branch
from
July 19, 2026 13:57
0da5241 to
5330d51
Compare
skyoo2003
changed the title
skyoo2003
added a commit
that referenced
this pull request
Jul 19, 2026
skyoo2003
added a commit
that referenced
this pull request
Jul 19, 2026
skyoo2003
force-pushed
the
feat/phase1-crud-engine
branch
from
July 19, 2026 21:15
783525f to
b674cb0
Compare
skyoo2003
added a commit
that referenced
this pull request
Jul 19, 2026
Address correctness findings in the generic CRUD fallback engine: - Fix a fatal data race: get()/list() aliased the stored map, so a concurrent Update (in-place write) and Get/List (marshal) triggered `concurrent map read and map write`, crashing the whole gateway. Both now return cloneMap copies. Covered by TestEngineConcurrentAccess. - List-returning Describe*/Get* ops (no item wrapper) now return an empty collection (200) instead of ResourceNotFoundException (400). - Reject malformed JSON bodies with SerializationException instead of fabricating a 200 from empty params. - Stamp the protocol-appropriate amz-json content-type so json-1.0 services are not served application/x-amz-json-1.1. - Router surfaces io.ReadAll body-read errors instead of swallowing them. - Restore lost regression coverage: repurpose the now-dead UNIMPLEMENTED_OPS list into a live served-or-honest-error test.
…erage Auto-serve standard CRUD-shaped operations that hand-written providers do not implement, so the long tail of ~2,200 operations across 46 JSON-protocol services responds to SDK calls without hand-coding each one. Fidelity is deliberately "plausible, not faithful": responses are store-backed and echo input plus synthesized ids/ARNs (create→get→list→delete round-trips), with no validation or business logic. Clearly documented and opt-in per service. - Engine: internal/shared/crud — in-memory resource store + verb dispatch (Create/Get/List/Delete/Update/Tag/…), with ErrUnclassified fallthrough. - Codegen: classify ops by name + output shape (internal/codegen/gen_crud_meta.go) and emit one aggregate registry internal/generated/crudregistry/registry_gen.go (single init(), so registration does not depend on per-service package imports). - Gateway: plugin.ErrUnhandledOp sentinel; router buffers the JSON body and falls back to the engine, returning an honest InvalidAction error when the op is not CRUD-classifiable — never a fabricated success. - Wired (pilot): glue, sagemaker, ssm, cloudwatchlogs, emr, transcribe. Other registered services keep returning InvalidAction until opted in (rollout backlog in roadmap.md). - Docs: docs/crud-engine.md (fidelity tiers, scope, limits); services-matrix and roadmap updated. Verified: engine unit tests, codegen reproducibility, and boto3 compat smoke tests. Suite: 740 -> 741.
…gine Complete the rollout: every registered JSON-protocol service now returns plugin.ErrUnhandledOp from its dispatch default, so unimplemented CRUD-shaped operations are served by the engine (previously only 6 pilot services). - 40 additional providers wired (default: -> plugin.ErrUnhandledOp), including core services (dynamodb, sqs, kms, ecs, ...); hand-implemented ops always win since the engine is only reached on fallthrough. Removed now-unused fmt imports. - Non-CRUD / unclassifiable ops still return an honest InvalidAction error. - Updated codebuild/codedeploy unit tests (unimplemented op now yields the sentinel) and the compat suite (formerly-erroring ops are now engine-served; unclassifiable ops still error). Docs updated: all JSON services wired. Verified: go build/vet/test ./... green; make test-compat 741 passed.
Address correctness findings in the generic CRUD fallback engine: - Fix a fatal data race: get()/list() aliased the stored map, so a concurrent Update (in-place write) and Get/List (marshal) triggered `concurrent map read and map write`, crashing the whole gateway. Both now return cloneMap copies. Covered by TestEngineConcurrentAccess. - List-returning Describe*/Get* ops (no item wrapper) now return an empty collection (200) instead of ResourceNotFoundException (400). - Reject malformed JSON bodies with SerializationException instead of fabricating a 200 from empty params. - Stamp the protocol-appropriate amz-json content-type so json-1.0 services are not served application/x-amz-json-1.1. - Router surfaces io.ReadAll body-read errors instead of swallowing them. - Restore lost regression coverage: repurpose the now-dead UNIMPLEMENTED_OPS list into a live served-or-honest-error test.
skyoo2003
force-pushed
the
feat/phase1-crud-engine
branch
from
July 24, 2026 14:44
3db44c5 to
2c62aa6
Compare
Address PR #98 review: replace hand-rolled cloneMap with maps.Clone (Go 1.26) at all call sites, and make randHex return a consistent 2*n-char id on the near-impossible rand failure instead of a fixed 10-char literal.
… return - 4 REST-JSON services (apigatewayv2, backup, iot, iotwireless) returned 501 for unimplemented ops; the CRUD engine can only classify awsJson (X-Amz-Target) protocols, so REST-JSON falls through. Return a 4xx UnsupportedOperation (matching the efs convention) so the response is an honest <500 AWS error, satisfying test_unimplemented_ops. - crud_test.go: check handleJSON return value (errcheck).
…ontract apigatewayv2/backup/iot/iotwireless default-branch now returns 400 UnsupportedOperation instead of 501 NotImplemented; update the provider unit tests that pinned the old 501/NotImplemented contract.
skyoo2003
changed the title
skyoo2003
changed the title
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
Adds a generic, Smithy-model-driven CRUD engine (
internal/shared/crud) that auto-serves standard CRUD-shaped operations a provider hasn't hand-implemented, and wires all 46 JSON-protocol services into it. ~2,200 previously-unimplemented operations now return plausible, store-backed responses (not faithful behavior — no validation, cross-resource integrity, or business logic); hand-implemented ops always take precedence, and ops the engine can't classify still return an honest AWS error — never a fabricated success.Related Issue
No dedicated tracking issue — part of Phase 1 (AWS Depth & Stabilization), tracked in docs/roadmap.md.
Fixes #
Changes
internal/shared/crud): in-memory resource store + verb dispatch (Create/Get/List/Delete/Update/Tag/…); returnsErrUnclassifiedfor anything it can't classify so the caller falls back to a normal AWS error. Concurrency-safe (copies on get/list,-racetested).internal/codegen/gen_crud_meta.go,templates/crud_registry.go.tmpl): classifies ops by name prefix + output shape and emits one aggregate registryinternal/generated/crudregistry/registry_gen.gowith a singleinit(), so registration doesn't depend on per-service package imports. Reproducible.internal/gateway/router.go,plugin.ErrUnhandledOp): a provider opts in by returning the sentinel from its dispatchdefault:; the router buffers the JSON body and falls back to the engine, otherwise returns an honestInvalidAction. The engine is only reached on fallthrough, so it can never shadow a hand-implemented op.default:branch now returnsplugin.ErrUnhandledOpto opt into the engine.default:branch now returns an honest400 UnsupportedOperationinstead of a server-side501 NotImplemented.docs/crud-engine.md(fidelity tiers, scope, limits);docs/services-matrix.mdanddocs/roadmap.mdupdated.Addedfragment.Test Plan
crud_test.go): verb dispatch,ErrUnclassifiedfallthrough, and concurrent Get/List/Update under-race.test_crud_engine.py,test_unimplemented_ops.py): create/get/delete round-trip, hand-implemented-wins, formerly-unimplemented-now-served, wired-but-unclassifiable-errors, and every listed unimplemented op returns<500(served or honest error).go test ./...→ 110 packages green; boto3 compat suite → 764 passing.Scope / non-goals
json-1.0/json-1.1, op inX-Amz-Target). Query / REST-XML (EC2/S3/Route53/IAM/RDS/CFN) are hand-written and out of scope; REST-JSON is not engine-servable and returns an honest error.List*of string-name members may not populate in SDKs.Checklist
golangci-lint run)changie new, see docs/release.md) — or N/A (docs/tests/chore only)