Add AWS Keyspaces (Cassandra) Full-Parity Support - #307
Conversation
Amazon Keyspaces is a managed, Apache Cassandra–compatible wide-column service. It is control-plane only here (CQL data operations are out of scope), so it gets a dedicated driver rather than reusing the relational/cache drivers. - Driver (services/keyspaces/driver): 18 core operations across keyspaces, tables, user-defined types, and tags, plus an AutoScaling optional capability (GetTableAutoScalingSettings) discovered by type assertion. - Provider (providers/aws/keyspaces): in-memory Mock with keyspaces (single/ multi-region replication), tables (full schema, capacity, encryption, PITR, TTL, client-side timestamps, CDC, replicas, provisioned auto-scaling), restore-from-PITR, UDTs, and tags; schema and reference validation; clone-on- read on every return path; account-default system keyspaces. - Server (server/aws/keyspaces): AWS JSON 1.0 handler on the "KeyspacesService." target prefix, with typed faults (ResourceNotFound/Conflict/Validation). Because Keyspaces models members in lowerCamelCase and its deserializer is case-sensitive, responses are emitted with lower-camel keys. Server-side pagination (MaxResults/NextToken) over the deterministic result set. Verified with real aws-sdk-go-v2/service/keyspaces round-trip + wire-error tests. - Wiring: registered in the AWS provider and server bundles; dedicated keyspaces:* cost keys; docs/services.md section and counts updated.
NitinKumar004
left a comment
There was a problem hiding this comment.
Deep end-to-end lifecycle review — AWS Keyspaces (full parity)
Reviewed the whole PR in an isolated worktree (gates, static/coverage, provider lifecycle, server/SDK/wire, cross-cutting wiring), with adversarial verification. The API surface is wired end-to-end and works through the real aws-sdk-go-v2/service/keyspaces client — the hard part (the case-sensitive lowerCamel key transform) is excellent — but the provider lifecycle has real correctness/consistency bugs.
Gates: green — build ✓ vet ✓ test ✓ -race ✓ gofmt ✓ go mod tidy ✓; lint clean. Coverage 83% provider / 70.7% server (disclosed).
Verified solid (the difficult surfaces): the lowerCamel key transform is correct field-by-field against the SDK's case-sensitive deserializer — every acronym prime-suspect (ResourceArn→resourceArn, RestoredTableARN→restoredTableARN, Ttl→ttl, CdcSpecification, KmsKeyIdentifier) matches, nested structs/lists covered, no dropped fields. Routing complete (18 core + 1 optional, all routed + SDK-tested); typed faults exact (ResourceNotFoundException/ConflictException/ValidationException); no time.Time ever reaches the wire (timestamps nil→null, SDK-safe); pagination correct; JSON-target dispatch disjoint from every AWS handler; wiring/cost/docs-counts/deps clean; determinism, clone-on-read, reference validation, and the empty-keyspace delete guard all correct.
Medium (should fix before merge — all provider-logic)
- UDT in-use delete-guard is dead code —
DeleteTypeguards onDirectReferringTables/DirectParentTypes, but those fields are never populated (only cloned);CreateTablenever records a table→UDT reference. → Create UDTaddress, a table with afrozen<address>column, thenDeleteType("app","address")succeeds, leaving a dangling reference. The guard the PR advertises does nothing. (inline) - Schema validation skips
StaticColumns, though its comment claims it validates them —validateSchemaloops PartitionKeys/ClusteringKeys only. →CreateTablewith aStaticColumnsentry not declared inAllColumnsis accepted; real Keyspaces rejects it. (inline) DeleteKeyspaceorphans UDTs, andGetTypenever checks the parent keyspace — delete only guards on tables. → Create keyspaceapp+ UDTapp/address,DeleteKeyspace("app")succeeds;GetType("app","address")still returns the orphaned type (and survives a re-created keyspace). (inline)CreateKeyspacestores the caller'sReplicationRegionsslice without cloning (write-aliasing on the multi-region path;CreateTableclones, this doesn't). → A caller mutating its ownregionsslice after create silently changes the stored keyspace. (inline)
Low
- Dual tag storage —
TagResourceupdates onlym.tags, so the embedded.Tagsgoes stale — but not observable via the SDK (Get doesn't surface tags); internal redundancy. - Weak validation:
ReplicationStrategygarbage accepted; empty system keyspaces are deletable (real Keyspaces forbids);UpdateTableappendsAddColumnswith no dup-name check;RestoreTabledoesn't validateRestoreTimestamp. - The aliasing test covers only 1 of ~8 clone paths (which is why the write-aliasing slipped through).
RestoreTablepriced0.0whileCreateTableis0.01; server nits — timestamps emitted asnull(comment says "omitted"), and a spuriousresultMetadata:{}on every response (harmless — SDK ignores unknown keys).- Coverage below the 90% pillar (disclosed).
Verdict: comment. The wire layer and wiring are genuinely strong; the four Mediums are real provider-logic bugs a user hits through normal API sequencing — and two of them (the dead UDT guard, the static-column validation comment) involve code that claims to do something it doesn't.
| return nil, cerrors.Newf(cerrors.NotFound, "type %q not found in keyspace %q", name, keyspace) | ||
| } | ||
|
|
||
| if len(u.DirectReferringTables) > 0 || len(u.DirectParentTypes) > 0 { |
There was a problem hiding this comment.
[Medium] This in-use guard is dead code. DirectReferringTables/DirectParentTypes are never assigned anywhere (grep shows only the clone at udt.go:14-15 and this read) — CreateTable/UpdateTable never register a table→UDT reference when a column type is frozen<udt>, and CreateType never records parent-type nesting. → DeleteType on a UDT a table depends on always succeeds, leaving the table with a dangling type reference — real Keyspaces rejects it, and the PR advertises this guard. Populate the reference fields on table/type create (or drop the vacuous guard and add a real one).
There was a problem hiding this comment.
Fixed in 62eb4ad. The reference fields are now populated: CreateTable/RestoreTable scan each column type for UDT mentions (typeMentions tokenizes the type, so frozen<address>/list<frozen<address>> match without false positives) and append the table to the UDT's DirectReferringTables; DeleteTable unregisters. DeleteType now blocks with FailedPrecondition while a table depends on the type. Covered by TestUDTInUseGuard.
|
|
||
| // validateSchema requires at least one partition key and rejects clustering/ | ||
| // static/partition columns that are not declared in AllColumns. | ||
| func validateSchema(s *ksdriver.SchemaDefinition) error { |
There was a problem hiding this comment.
[Medium] Schema validation skips StaticColumns despite the comment saying it validates them. validateSchema loops PartitionKeys and ClusteringKeys only; StaticColumns is cloned but never checked against AllColumns. → CreateTable with StaticColumns:[{Name:"undeclared"}] not present in AllColumns is accepted; real Keyspaces returns a ValidationException. Add a StaticColumns loop (and consider dup-column / empty-name checks).
There was a problem hiding this comment.
Fixed in 62eb4ad. validateSchema now loops StaticColumns against the declared columns, and also rejects empty and duplicate column names. Covered by TestSchemaRejectsUndeclaredStaticColumn (the declared-columns build was extracted to keep the function under the complexity limit).
| } | ||
|
|
||
| // DeleteKeyspace removes a keyspace; it must contain no tables. | ||
| func (m *Mock) DeleteKeyspace(_ context.Context, name string) error { |
There was a problem hiding this comment.
[Medium] DeleteKeyspace orphans UDTs. It guards only keyspaceHasTables, not UDTs, so a keyspace with UDTs (and no tables) deletes and leaves the UDT records dangling in m.udts. Compounding it, GetType/DeleteType (udt.go) look up m.udts directly and never verify the parent keyspace exists. → DeleteKeyspace("app") then GetType("app","address") still returns the orphaned type — even after app is re-created. Cascade-delete (or guard on) UDTs here, and have GetType/DeleteType verify the keyspace.
There was a problem hiding this comment.
Fixed in 62eb4ad. DeleteKeyspace now also guards user-defined types (FailedPrecondition if any remain) and refuses system keyspaces; GetType/DeleteType verify the parent keyspace exists first, so a type can't be looked up after its keyspace is gone. Covered by TestDeleteKeyspaceGuardsTypesAndSystem.
|
|
||
| const minMultiRegionRegions = 2 | ||
|
|
||
| regions := cfg.ReplicationRegions |
There was a problem hiding this comment.
[Medium] CreateKeyspace aliases the caller's ReplicationRegions slice (clone-on-write violation). regions := cfg.ReplicationRegions is stored verbatim for MULTI_REGION, unlike CreateTable which does append([]string(nil), cfg.ReplicaRegions...). → A caller that reuses/mutates its regions slice after CreateKeyspace silently changes the stored keyspace's replication regions. This is the #304/#305 aliasing lesson, applied on read but missed on this write path — clone it: append([]string(nil), cfg.ReplicationRegions...).
There was a problem hiding this comment.
Fixed in 62eb4ad. CreateKeyspace now clones the slice: append([]string(nil), cfg.ReplicationRegions...), matching CreateTable. Covered by TestCreateKeyspaceClonesRegions (mutating the caller's slice after create no longer reaches the store).
… timestamp Fixes from NitinKumar004's review. Medium - UDT in-use delete guard is now live: CreateTable/RestoreTable register a table→UDT reference for every column type that mentions a UDT, DeleteTable unregisters it, so DeleteType blocks (FailedPrecondition) while a table depends on the type. - validateSchema now checks StaticColumns against AllColumns (and rejects empty/duplicate column names), matching the doc comment. - DeleteKeyspace guards user-defined types (not just tables) and refuses system keyspaces; GetType/DeleteType verify the parent keyspace exists, so a stale type can't be looked up after its keyspace is gone. - CreateKeyspace clones the caller's ReplicationRegions slice (clone-on-write), matching CreateTable. Low - Reject unknown ReplicationStrategy values; UpdateTable rejects duplicate added columns; RestoreTable validates RestoreTimestamp (optional, defaults to now, rejected if in the future) — decoded from the AWS-JSON epoch number the SDK sends, which can't unmarshal into a time.Time. - keyspaces:RestoreTable priced like CreateTable (0.01). - Response transform drops null values and the empty resultMetadata envelope, so omitted timestamps truly disappear rather than serializing as null. - Tags kept solely in the ARN-keyed map (no stale struct-level copy). - Expanded clone-on-read and edge-case tests (UDT guard, static-column validation, keyspace/type guards, region aliasing, dup column, future restore).
|
Thanks for the thorough review, @NitinKumar004 — all four Mediums and the Lows are fixed in 62eb4ad. Inline replies on each Medium; the Lows:
Full local gate green before push: build / vet / Not changed: |
NitinKumar004
left a comment
There was a problem hiding this comment.
Review comments addressed — approving ✅
Verified 62eb4ad against the earlier review; every finding is resolved, correctly and with tests. Gates green (build ✓ vet ✓ test ✓ -race ✓ gofmt ✓ go mod tidy ✓; lint clean on the Keyspaces files).
Medium — fixed:
- UDT in-use delete-guard is now live —
registerTableUDTRefs/unregisterTableUDTRefspopulateDirectReferringTableson table create/restore/delete, using a robust identifier-token matcher (typeMentionssplits on non-identifier chars and compares exact tokens, sofrozen<address>/list<frozen<address>>match whileaddressbookdoes not). Deleting a UDT a table depends on is now rejected. - Schema validation now covers
StaticColumns—validateSchemaloops static columns againstAllColumns. DeleteKeyspaceno longer orphans UDTs — it now blocks onkeyspaceHasTypes(matching the existing table guard).CreateKeyspaceclonesReplicationRegions—append([]string(nil), cfg.ReplicationRegions...), closing the multi-region write-aliasing.
Low — also fixed:
wirejsonnow strips the always-emptyresultMetadataenvelope (and null values), so responses carry only meaningful fields.RestoreTableis now priced0.01(it provisions a new table), consistent withCreateTable.
Plus +132 lines of new provider tests. The full Keyspaces lifecycle (keyspaces → tables + schema validation → UDTs with live in-use guards → tags → auto-scaling → PITR restore → pagination) is implemented, wired, and round-trips through the real aws-sdk-go-v2/service/keyspaces client; the case-sensitive lowerCamel key transform is correct field-by-field. Non-blocking follow-ups: a few minor validation nits (ReplicationStrategy enum, system-keyspace deletability, UpdateTable dup-column check) and server coverage ~70% (below the 90% pillar, consistent with the codebase). LGTM.
Objective
Add full-parity support for Amazon Keyspaces (for Apache Cassandra) — the next missing managed database-server surface after MemoryDB (#305). Covers every control-plane resource, child resource, pagination, and provisioned auto-scaling, so a real
aws-sdk-go-v2/service/keyspacesclient works unchanged against a custom endpoint.What we found
services/keyspaces/driver.KeyspacesService.target prefix — disjoint from every existing handler.encoding/jsonyields PascalCase keys the client can't decode.How we fixed it
services/keyspaces/driver): 18 core operations (keyspaces, tables, user-defined types, tags) + anAutoScalingoptional capability (GetTableAutoScalingSettings), type-asserted.providers/aws/keyspaces): in-memoryMock— keyspaces (single/multi-region replication), tables (fullSchemaDefinition, capacity mode + RCU/WCU, encryption, PITR, TTL, client-side timestamps, CDC, replicas, provisioned auto-scaling), point-in-timeRestoreTable, UDTs, tags. Schema validation (partition key required, keys declared), reference validation, empty-keyspace delete guard, clone-on-read on every path, account-default system keyspaces.server/aws/keyspaces): JSON 1.0 handler; canonical errors → typed faults (ResourceNotFoundException/ConflictException/ValidationException). Responses are emitted with lower-camel keys (a recursive key transform) so the case-sensitive SDK deserializer decodes them. Server-side pagination (MaxResults/NextToken) over the deterministic result set.keyspaces:*cost keys;docs/services.mdsection + counts (Grand Total 1310 → 1328).Alternatives not taken
CreationTimestamp/LastModifiedTimestamp: omitted — AWS JSON 1.0 encodes timestamps as epoch numbersencoding/jsoncan't produce for atime.Time(same mitigation as MemoryDB). All other fields round-trip through the real SDK.Docs / Test / Playground
docs/services.md: Keyspaces section (per-resource tables incl. Auto Scaling capability + pagination note) + recomputed totals.ConflictException/ResourceNotFoundException/ValidationException). Cost rate-catalog test.Test plan
go build ./...,go vet ./...go test ./...(full module, green)golangci-lint— 0 issues on all new/changed packagesgo mod tidycleanRisk & Rollback
Additive: a new driver + provider/server packages plus opt-in bundle registration (nil driver ⇒ handler not registered). No existing behavior changes. Rollback = revert the commit.
Conclusion
Keyspaces reaches full parity — keyspaces, tables (with PITR restore), user-defined types, tags, provisioned auto-scaling, and pagination — with the established driver→provider→server layering. Follow-ups (separate PRs, missing-database-server initiative): AWS Timestream, GCP Spanner & Bigtable, Azure Managed Cassandra.