Conversation
|
Caution Review failedAn error occurred during the review process. Please try again later. WalkthroughAdds persistent grant expansion support (new Changes
Sequence Diagram(s)sequenceDiagram
participant Syncer
participant DB as Database
participant Graph as EntitlementGraph
participant Src as SourceEntitlements
Syncer->>DB: ListGrants(expandable_only=..., needs_expansion_only=...)
DB-->>Syncer: rows (id, data, expansion, needs_expansion, ...)
loop per grant
Syncer->>Syncer: extractAndStripExpansion(grant → expansionBytes)
alt expansion present
Syncer->>Src: Fetch source entitlement(s) for each EntitlementId
Src-->>Syncer: source entitlement / not found / error
alt source found & type matches
Syncer->>Graph: AddEntitlementID(destination)
Syncer->>Graph: AddEntitlementID(source)
Syncer->>Graph: AddEdge(source → destination)
else skip
end
end
end
alt starting fresh (no edges/page)
Syncer->>DB: SetSupportsDiff(syncID)
DB-->>Syncer: ack
else resuming
Syncer->>Syncer: skip SetSupportsDiff
end
Syncer->>Graph: Mark graph Loaded
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/dotc1z/grants_expandable_query.go`:
- Around line 158-162: annos.Pick may return (false, nil) when the
GrantExpandable annotation is missing; update the block around annos.Pick(ge) to
treat a false "found" as an error or at least log a warning and fail fast
instead of continuing with an empty v2.GrantExpandable. Specifically, check the
boolean return from annos.Pick(ge) (the first value), and if it's false log a
warning including externalID and either return an error (so you don't construct
an ExpandableGrantDef with nil SrcEntitlementIDs) or explicitly set safe
defaults before proceeding; make this change wherever annos.Pick and
v2.GrantExpandable are used so downstream code (e.g., ExpandableGrantDef
creation) cannot receive an empty annotation unexpectedly.
🧹 Nitpick comments (1)
pkg/dotc1z/grants.go (1)
251-328: Rows with invalidGrantExpandabletext in blob will be re-scanned on every migration.The query selects rows where
is_expandable=0 AND data LIKE '%GrantExpandable%', but rows that match the LIKE pattern but failgrantExpandableColumnsvalidation (e.g., the text "GrantExpandable" appears in a different context or with empty entitlement IDs) are skipped without update. These rows will be re-scanned on subsequent migrations.Consider marking these rows as "checked" to avoid repeated scans, or accept this as a minor inefficiency since the LIKE filter should only match a small subset of rows.
💡 Potential approach to avoid re-scanning
One option is to set
is_expandable = -1(or another sentinel value) for rows that were checked but deemed not expandable, then exclude those from future scans:WHERE is_expandable=0 AND data LIKE '%%GrantExpandable%%' +-- or use: WHERE is_expandable >= 0 to skip -1 sentinelHowever, this adds complexity and may not be worth it if the number of false-positive rows is small.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Fix all issues with AI agents
In `@pkg/sync/syncer.go`:
- Around line 1757-1765: The pagination state update and graph completion flag
are being applied before the current page's defs are processed, which risks
skipping pages on retry or marking graph.Loaded prematurely; move the call to
s.state.NextPage(ctx, nextPageToken) and the assignment graph.Loaded = true to
occur only after the code that processes defs (the block that iterates over defs
and calls AddEdge/etc.) completes successfully so that pagination is advanced
and graph.Loaded is set only on successful processing of the page.
- Around line 1737-1746: The code currently hard-fails when s.store doesn't
implement expandableGrantLister; change this to the same soft type-assertion
pattern used for ExpansionStore: attempt the type assertion for
expandableGrantLister (the interface that exposes ListExpandableGrants), and if
ok is false simply fall back (e.g., skip the fast-path and proceed with the full
grant listing or return empty results) instead of returning an error; update the
block around the expandableGrantLister/ListExpandableGrants usage so callers
continue normally when the method is absent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/sync/syncer.go`:
- Around line 1737-1747: The code currently silently returns nil when s.store
doesn't implement expandableGrantLister, which causes graph.Loaded to remain
false and disables expansion; instead hard-fail: detect the missing capability
on s.store (the expandableGrantLister interface / ListExpandableGrants) and
return an explicit error (and log it) from the enclosing function so the caller
surfaces the failure rather than looping; update the branch that checks lister,
ok := s.store.(expandableGrantLister) to log a clear error and return
fmt.Errorf(...) (or the function's error return) indicating ListExpandableGrants
is required.
pkg/dotc1z/grants.go
Outdated
| } | ||
|
|
||
| // Backfill from stored grant bytes for rows that haven't been classified yet. | ||
| return backfillGrantExpandableColumns(ctx, db, r.Name()) |
There was a problem hiding this comment.
We should figure out a way to skip this if we're running migrations on a db that has already had its columns backfilled (or was run with a newer version of baton-sdk).
pkg/dotc1z/grants.go
Outdated
|
|
||
| err := f(ctx, c, grants.Name(), | ||
| func(grant *v2.Grant) (goqu.Record, error) { | ||
| expandable := isGrantExpandable(grant) |
There was a problem hiding this comment.
I'm pretty sure the GrantExpandable annotation isn't used by anything except connectors, so we could remove the expandable annotation from the grant before saving it. That way the data is in one place and can never get out of sync.
| s.handleInitialActionForStep(ctx, *s.state.Current()) | ||
| } | ||
|
|
||
| resp, err := s.store.ListGrants(ctx, v2.GrantsServiceListGrantsRequest_builder{PageToken: pageToken}.Build()) |
There was a problem hiding this comment.
To avoid breaking the interface here, we should use ListGrants but add a WithExpandable flag (or something similar) to the GrantsServiceListGrantsRequest.
pkg/sync/syncer.go
Outdated
| // Mark the sync as having reached the expansion phase. This is set unconditionally | ||
| // so that diffs can detect syncs created with older code that dropped annotations. | ||
| if expansionStore, ok := s.store.(connectorstore.ExpansionStore); ok { | ||
| if err := expansionStore.SetExpansionStarted(ctx, s.syncID); err != nil { |
There was a problem hiding this comment.
Maybe this should be called a little later? Not sure.
kans
force-pushed
the
kans/grant-expandable-columns
branch
from
bdd5572 to
1cecec8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/sync/syncer.go`:
- Around line 1742-1747: The ListGrants call is constructing a
GrantsServiceListGrantsRequest (via GrantsServiceListGrantsRequest_builder)
without a Resource which will cause generated validation or builder code that
calls request.GetResource().GetId() to panic; update the store and request
building so Resource can be nil when using ExpandableOnly/NeedsExpansionOnly
filters: either change the proto to make Resource optional for these filters, or
modify any code paths that access Resource (notably code that calls
request.GetResource().GetId()) in implementations like the builder and SQL store
to null-check request.GetResource() and branch to the filter-only logic when
Resource == nil, ensuring ListGrants, GrantsServiceListGrantsRequest_builder,
and downstream store implementations uniformly handle resource-less requests.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@pkg/dotc1z/grants.go`:
- Around line 427-513: In backfillGrantExpansionColumn, when
extractAndStripExpansion(g) returns expansionBytes == nil you must still persist
the stripped proto and mark the row processed so the loop can make progress:
after re-serializing g (newData), call stmt.ExecContext(ctx, nil, 0, newData,
r.id) (or equivalent) instead of continuing, ensuring expansion is NULL and
needs_expansion is false; do this before moving to the next batch so
invalid/empty GrantExpandable rows are updated and the backfill can terminate.
|
|
||
| message GrantsServiceListGrantsRequest { | ||
| c1.connector.v2.Resource resource = 1 [(validate.rules).message = {required: true}]; | ||
| c1.connector.v2.Resource resource = 1; |
There was a problem hiding this comment.
Validation was added to the call sites that need it.
| } | ||
| } | ||
| require.Len(t, allGrants, expectedGrantCount, "should have %d grants but got %d", expectedGrantCount, len(allGrants)) | ||
| for _, grant := range allGrants { |
There was a problem hiding this comment.
We just remove these ahead of time now, unconditionally, because they are only used at the SDK application layer.
kans
force-pushed
the
kans/grant-expandable-columns
branch
from
75d4dfe to
5d369aa
Compare
2 participants
... which is useful for graph expansion without visiting the world, and latter, for partial expansion of partial syncs.
Summary by CodeRabbit
New Features
Bug Fixes
Tests