feat(reader): add c1.reader.v3 grants reader service (un-nerfed records)#1031
feat(reader): add c1.reader.v3 grants reader service (un-nerfed records)#1031manojacs wants to merge 1 commit into
Conversation
The reader_v2 GrantsReaderService returns c1.connector.v2.Grant, which drops fields the storage record holds — notably discovered_at — when it downshifts each v3.GrantRecord via V3GrantToV2. c1's uplift needs the per-grant discovered_at (the connector's actual observation time) to decide overlay deletion safely, so the read must stop losing it. Add a parallel c1.reader.v3 GrantsReaderService that returns the rich c1.storage.v3.GrantRecord directly — nothing dropped on read. - proto: new proto/c1/reader/v3/grant.proto — same 5 RPCs and request shapes as reader_v2 (GetGrant, ListGrantsForEntitlement, ListGrantsForResourceType, ListGrantsForEntitlements, ListGrantsForPrincipal); responses return c1.storage.v3.GrantRecord. - pebble: engineV3Grants implements the generated server, reusing the exact sync-resolution, fetch, pagination, cursor and filter helpers as the v2 methods and skipping only the V3GrantToV2 conversion. v2 behavior is byte-for-byte unchanged. - connectorstore: optional V3GrantReader capability + V3GrantReaderProvider discovery interface (type-asserted, Pebble-only; callers fall back to the v2 reader). Not part of the required Reader interface, so it never forces the other engines to implement it. The provider indirection is required because the v3 RPC names collide with the v2 ones on the store and Go has no method overloading. - tests: all 5 v3 RPCs covered — records returned with their STORED discovered_at (never time.Now()), principal/resource-type filters honored, and a pagination round-trip proving cursor + records survive page boundaries. Additive and backward-compatible; no proto renumbering; grants-only (other record types follow the same pattern as needed). Co-authored-by: c1-squire-dev[bot] <c1-squire-dev[bot]@users.noreply.github.com>
General PR Review: feat(reader): add c1.reader.v3 grants reader service (un-nerfed records)Blocking Issues: 0 | Suggestions: 1 | Threads Resolved: 0 Review SummaryScanned the full PR diff for security and correctness. This is an additive change: a new Security IssuesNone found. Correctness IssuesNone found. Suggestions
Prompt for AI agents |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
Leaning toward seperation of concerns here as the co-existence of v2 and v3 readers are for a limited time and planning to deprecate v2. |
| ctx context.Context, | ||
| req *reader_v3.GrantsReaderServiceGetGrantRequest, | ||
| ) (*reader_v3.GrantsReaderServiceGetGrantResponse, error) { | ||
| syncID := r.e.CurrentSyncID() |
There was a problem hiding this comment.
Why not use syncID, err := e.resolveActiveSyncForReader(ctx, req.GetAnnotations()) here like in the others? Ostensibly, we could drop the sync check entirely at some point.
edit, oh pkg/dotc1z/engine/pebble/adapter.go:922 does this too. Thats probably why this method is written this way. Would you fix that one, too?
edit edit: It looks like we don't preserve the sync id through a EndFreshSync? In which case, this API, and the v2 maybe won't work (in prod)?
| cursorFor := func(rec *v3.GrantRecord) string { | ||
| id, err := grantIdentityFromRecord(rec) | ||
| if err != nil { | ||
| return "" |
There was a problem hiding this comment.
^ swallowing this error is probably not correct
2 participants
What & why
The v2 reader (
c1.reader.v2GrantsReaderService) returnsc1.connector.v2.Grant, which drops fields the storage record holds — notablydiscovered_at— when it downshifts eachv3.GrantRecordthroughV3GrantToV2. c1's uplift needs the per-grantdiscovered_at(the connector's actual observation time) to decide overlay deletion safely, so the read has to stop losing it.This adds a parallel
c1.reader.v3GrantsReaderServicethat returns the richc1.storage.v3.GrantRecorddirectly — nothing dropped on read.Shape
proto/c1/reader/v3/grant.proto: same 5 RPCs and request shapes asreader_v2(GetGrant,ListGrantsForEntitlement,ListGrantsForResourceType,ListGrantsForEntitlements,ListGrantsForPrincipal); only the response record type changes toc1.storage.v3.GrantRecord.engineV3Grantsimplements the generated server, reusing the exact same sync-resolution, fetch, pagination, cursor and filter helpers as the v2 methods and skipping only theV3GrantToV2conversion. v2 behavior is byte-for-byte unchanged.V3GrantReadercapability +V3GrantReaderProviderdiscovery interface: type-asserted, Pebble-only, callers fall back to the v2 reader. Deliberately not part of the requiredReaderinterface, so it never forces SQLite/gRPC readers to implement it.Note on the provider indirection
The v3 RPC method names (
GetGrant, …) are identical to thereader_v2methods*Enginealready carries, and Go has no method overloading — one type can't declare both. So the v3 methods live on a wrapper and*Engineexposes them viaV3GrantReader(); discover the capability by asserting the store forV3GrantReaderProvider, notV3GrantReaderdirectly.Compatibility
Additive and backward-compatible: existing
c1/connector/v2wire contract untouched, no proto renumbering, nogo.mod/go.sum/vendor changes, no default-behavior change. The value returned is the storeddiscovered_at, never a freshtime.Now().Tests
All 5 v3 RPCs covered in
adapter_reader_v3_test.go: records returned with their storeddiscovered_at, principal/resource-type filters honored (decoys excluded), and a pagination round-trip proving the cursor + records survivePageSize:1page boundaries.go build ./...,go test ./pkg/connectorstore/... ./pkg/dotc1z/..., andgolangci-linton changed files all pass.Scope / follow-ups
Grants-only (the must-have that unblocks the uplift/discovered_at work). The other record types (resources, entitlements, resource types, syncs) can follow the same
reader_v3pattern as needed. No gRPC server registration is wired to a network endpoint yet — this is the service definition + in-process engine implementation.🤖 Generated with Claude Code