Revert "Track expandable state at the sql level"#696
Closed
Conversation
This reverts commit 5c96b59.
|
No actionable comments were generated in the recent review. 🎉 WalkthroughThis PR removes grant-expansion schema/logic, consolidates grant upsert/list to use the public Writer PutGrants/ListGrants APIs, updates callers and tests to new entitlement APIs, and removes several expansion-related types, migrations, and tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Possibly related PRs
Suggested reviewers
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
pkg/sync/syncer.go (1)
1717-1785:⚠️ Potential issue | 🟠 MajorStripping
GrantExpandableduring graph load can break crash/resume unless you force a checkpoint.
loadEntitlementGraph(Line 1752–Line 1779) mutates persisted grants by removingGrantExpandable. But sync checkpointing is rate-limited (minCheckpointInterval), so a crash between “annotation stripped” and the next forced checkpoint can leave you in a state where:
- the graph/page token aren’t persisted, and
- the source data marker (
GrantExpandable) is gone,
making the resume path skip rebuilding those edges.Minimal mitigation: if you write any “stripped” grants, force a checkpoint immediately after the write completes.
Proposed mitigation (force checkpoint after grant mutations)
func (s *syncer) loadEntitlementGraph(ctx context.Context, graph *expand.EntitlementGraph) error { @@ updatedGrants := make([]*v2.Grant, 0) + wroteUpdates := false for _, grant := range resp.GetList() { err := s.processGrantForGraph(ctx, grant, graph) if err != nil { return err } @@ if !updated { continue } + wroteUpdates = true grant.SetAnnotations(newAnnos) @@ updatedGrants, err = expand.PutGrantsInChunks(ctx, s.store, updatedGrants, 10000) if err != nil { return err } } _, err = expand.PutGrantsInChunks(ctx, s.store, updatedGrants, 0) if err != nil { return err } + + // We mutated the persisted grants in a way that affects resume/idempotency. + // Ensure the state machine (graph + page token) is persisted immediately. + if wroteUpdates { + if err := s.Checkpoint(ctx, true); err != nil { + return err + } + } @@ return nil }Also applies to: 1787-1854
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sync/syncer.go` around lines 1717 - 1785, loadEntitlementGraph mutates persisted grants by stripping GrantExpandable then writes them with expand.PutGrantsInChunks, which can break crash/resume if a checkpoint isn't persisted immediately; after any successful write of updatedGrants via expand.PutGrantsInChunks inside loadEntitlementGraph (and the similar block later), immediately force the sync state to persist a checkpoint by invoking the syncer's checkpoint API (e.g., s.state.ForceCheckpoint(ctx) or the equivalent method on s.state) so the page token/graph state and the removal of GrantExpandable are atomically reflected for resume.pkg/sync/expand/expand_benchmark_test.go (1)
89-139:⚠️ Potential issue | 🟡 MinorBenchmark graph loader: don’t ignore
AddEdgeerrors (silently building an invalid graph skews results).At Line 122–Line 127 the
_ = graph.AddEdge(...)drops an important signal. Even in benchmarks, returning the error keeps failures actionable and avoids measuring “expansion on a partial graph.”Proposed fix
- _ = graph.AddEdge(ctx, + if err := graph.AddEdge(ctx, srcEntitlement.GetEntitlement().GetId(), grant.GetEntitlement().GetId(), expandable.GetShallow(), expandable.GetResourceTypeIds(), - ) + ); err != nil { + return nil, err + }Also applies to: 154-156
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sync/expand/expand_benchmark_test.go` around lines 89 - 139, In loadEntitlementGraphFromC1Z you're ignoring errors from graph.AddEdge (the `_ = graph.AddEdge(...)`), which can produce a partially built graph; instead check the error returned by graph.AddEdge and propagate it (return nil, err) so failures surface during benchmarks; apply the same change to the other AddEdge call(s) referenced (e.g., the second graph.AddEdge occurrence around the later block) so any AddEdge error aborts loading and is returned to the caller.
🧹 Nitpick comments (2)
pkg/sync/expand/graph.go (1)
3-11:AddEntitlementshould ignore nil/empty entitlement IDs (defensive, avoids corrupting graph state).Because
AddEntitlementnow takes a message (Line 183), it’s easy for call sites (especially tests/benchmarks) to pass a partially-constructed entitlement. Ifentitlement.GetId()is empty, you’ll end up with a node keyed by""inEntitlementsToNodes, which can create hard-to-diagnose edges later.Proposed defensive guard
func (g *EntitlementGraph) AddEntitlement(entitlement *v2.Entitlement) { + entitlementID := entitlement.GetId() + if entitlementID == "" { + // Invalid entitlement; ignore rather than polluting graph with an empty key. + return + } // If the entitlement is already in the graph, fail silently. - found := g.GetNode(entitlement.GetId()) + found := g.GetNode(entitlementID) if found != nil { return } g.HasNoCycles = false // Reset this since we're changing the graph. @@ node := Node{ Id: g.NextNodeID, - EntitlementIDs: []string{entitlement.GetId()}, + EntitlementIDs: []string{entitlementID}, } @@ - g.EntitlementsToNodes[entitlement.GetId()] = node.Id + g.EntitlementsToNodes[entitlementID] = node.Id }Also applies to: 182-203
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sync/expand/graph.go` around lines 3 - 11, Add a defensive guard in AddEntitlement that ignores nil entitlement messages and those whose entitlement.GetId() is empty so we never insert an empty key into EntitlementsToNodes; locate the AddEntitlement method (which accepts an entitlement message) and at its start return early if entitlement == nil or entitlement.GetId() == "" (optionally log via ctxzap/ zap) so EntitlementsToNodes and subsequent graph operations are never corrupted by an empty-string key.pkg/sync/syncer_test.go (1)
1622-1624: MockListGrantssimplification is fine; consider normalizing nil → empty slices for stability.At Line 1623 and Line 1729, a missing map entry returns a nil slice. That’s usually OK, but returning
[]*v2.Grant{}can make tests/debugging a bit more deterministic.Proposed tweak (optional)
func (mc *mockConnector) ListGrants(ctx context.Context, in *v2.GrantsServiceListGrantsRequest, opts ...grpc.CallOption) (*v2.GrantsServiceListGrantsResponse, error) { - return v2.GrantsServiceListGrantsResponse_builder{List: mc.grantDB[in.GetResource().GetId().GetResource()]}.Build(), nil + list := mc.grantDB[in.GetResource().GetId().GetResource()] + if list == nil { + list = []*v2.Grant{} + } + return v2.GrantsServiceListGrantsResponse_builder{List: list}.Build(), nil }Also applies to: 1728-1735
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/sync/syncer_test.go` around lines 1622 - 1624, The mock ListGrants currently returns mc.grantDB[in.GetResource().GetId().GetResource()] directly which can be nil when the map has no entry; change the mockConnector.ListGrants implementation to normalize a nil slice into an empty []*v2.Grant{} before building the response so tests always get a non-nil List field (adjust the behavior in mockConnector.ListGrants and mirror the same normalization used elsewhere in the mock grant map handling).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@pkg/sync/syncer.go`:
- Around line 2168-2172: processGrantsWithExternalPrincipals currently appends
the same newGrant twice (once before mutating annotations and again after), so
move the final append of newGrant to after all annotation/mutation logic
completes (refer to newGrant and the append calls around
processGrantsWithExternalPrincipals) so each grant ID is only added once;
separately, in syncGrantsForResource where etaggedGrants and resp.GetList() are
concatenated before calling s.store.PutGrants, deduplicate the combined slice by
grant ID (or explicitly document and rely on upsert semantics) so PutGrants
never receives duplicate grant IDs (refer to etaggedGrants, resp.GetList, and
PutGrants).
---
Outside diff comments:
In `@pkg/sync/expand/expand_benchmark_test.go`:
- Around line 89-139: In loadEntitlementGraphFromC1Z you're ignoring errors from
graph.AddEdge (the `_ = graph.AddEdge(...)`), which can produce a partially
built graph; instead check the error returned by graph.AddEdge and propagate it
(return nil, err) so failures surface during benchmarks; apply the same change
to the other AddEdge call(s) referenced (e.g., the second graph.AddEdge
occurrence around the later block) so any AddEdge error aborts loading and is
returned to the caller.
In `@pkg/sync/syncer.go`:
- Around line 1717-1785: loadEntitlementGraph mutates persisted grants by
stripping GrantExpandable then writes them with expand.PutGrantsInChunks, which
can break crash/resume if a checkpoint isn't persisted immediately; after any
successful write of updatedGrants via expand.PutGrantsInChunks inside
loadEntitlementGraph (and the similar block later), immediately force the sync
state to persist a checkpoint by invoking the syncer's checkpoint API (e.g.,
s.state.ForceCheckpoint(ctx) or the equivalent method on s.state) so the page
token/graph state and the removal of GrantExpandable are atomically reflected
for resume.
---
Nitpick comments:
In `@pkg/sync/expand/graph.go`:
- Around line 3-11: Add a defensive guard in AddEntitlement that ignores nil
entitlement messages and those whose entitlement.GetId() is empty so we never
insert an empty key into EntitlementsToNodes; locate the AddEntitlement method
(which accepts an entitlement message) and at its start return early if
entitlement == nil or entitlement.GetId() == "" (optionally log via ctxzap/ zap)
so EntitlementsToNodes and subsequent graph operations are never corrupted by an
empty-string key.
In `@pkg/sync/syncer_test.go`:
- Around line 1622-1624: The mock ListGrants currently returns
mc.grantDB[in.GetResource().GetId().GetResource()] directly which can be nil
when the map has no entry; change the mockConnector.ListGrants implementation to
normalize a nil slice into an empty []*v2.Grant{} before building the response
so tests always get a non-nil List field (adjust the behavior in
mockConnector.ListGrants and mirror the same normalization used elsewhere in the
mock grant map handling).
Comment on lines
2168
to
2172
|
|
||
| err = s.store.UpsertGrants(ctx, connectorstore.GrantUpsertOptions{ | ||
| Mode: connectorstore.GrantUpsertModeReplace, | ||
| }, grants...) | ||
| err = s.store.PutGrants(ctx, grants...) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find PutGrants implementation to understand duplicate handling semantics
echo "=== PutGrants Implementation ==="
rg -n --type=go 'func\s+\(.*\)\s+PutGrants\s*\(' -A 15
# Find processGrantsWithExternalPrincipals to check for double-append pattern
echo -e "\n=== Looking for processGrantsWithExternalPrincipals ==="
rg -n --type=go 'func.*processGrantsWithExternalPrincipals' -A 50 | head -100Repository: ConductorOne/baton-sdk
Length of output: 4973
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Search more broadly for the pattern of expandedGrants appends with newGrant
echo "=== All expandedGrants = append patterns with newGrant ==="
rg -n --type=go 'expandedGrants\s*=\s*append\(\s*expandedGrants\s*,\s*newGrant\s*\)' -B 3 -A 3Repository: ConductorOne/baton-sdk
Length of output: 2823
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check the file size and structure around the mentioned lines
echo "=== File structure and line counts ==="
wc -l pkg/sync/syncer.go
# Look at the specific regions mentioned in the review comment
echo -e "\n=== Lines 2168-2172 (primary concern) ==="
sed -n '2168,2172p' pkg/sync/syncer.go
echo -e "\n=== Lines 2340-2349 ==="
sed -n '2340,2349p' pkg/sync/syncer.go
echo -e "\n=== Lines 2454-2463 ==="
sed -n '2454,2463p' pkg/sync/syncer.goRepository: ConductorOne/baton-sdk
Length of output: 768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find putGrantsInternal to understand how it handles duplicates
echo "=== putGrantsInternal implementation ==="
rg -n --type=go 'func.*putGrantsInternal' -A 30
# Get more context around lines 2710-2743 to confirm double-append
echo -e "\n=== Full context around double-append at lines 2707-2745 ==="
sed -n '2707,2745p' pkg/sync/syncer.goRepository: ConductorOne/baton-sdk
Length of output: 3709
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check syncGrantsForResource merge logic around line 2169
echo "=== syncGrantsForResource context around line 2169 ==="
sed -n '2155,2185p' pkg/sync/syncer.go
# Also get function definition
echo -e "\n=== syncGrantsForResource function header ==="
rg -n --type=go 'func.*syncGrantsForResource' -A 5Repository: ConductorOne/baton-sdk
Length of output: 1435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check all the "also applies to" ranges for similar issues
echo "=== Range 2340-2349 context ==="
sed -n '2330,2355p' pkg/sync/syncer.go
echo -e "\n=== Range 2454-2463 context ==="
sed -n '2444,2470p' pkg/sync/syncer.go
echo -e "\n=== Range 2642-2750 context (beginning) ==="
sed -n '2640,2660p' pkg/sync/syncer.go
echo -e "\n=== Range 2793-2844 context ==="
sed -n '2785,2850p' pkg/sync/syncer.goRepository: ConductorOne/baton-sdk
Length of output: 4788
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Get full context of syncGrantsForResource to see how grants are built
echo "=== syncGrantsForResource full function (lines 2058-2190) ==="
sed -n '2058,2190p' pkg/sync/syncer.go | head -100
# Look for the grants construction/merge logic
echo -e "\n=== Search for grants variable in syncGrantsForResource context ==="
sed -n '2058,2180p' pkg/sync/syncer.go | grep -n 'grants' -B 2 -A 2Repository: ConductorOne/baton-sdk
Length of output: 4827
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check if there's a deduplication step before PutGrants in any of the functions
echo "=== Check for deduplication logic before PutGrants ==="
rg -n --type=go 'expandedGrants' -A 5 -B 5 | grep -A 10 -B 5 'PutGrants'
# Also check the mapset deduplication at line 2847
echo -e "\n=== Lines 2847-2855 (dedup check before PutGrants) ==="
sed -n '2847,2860p' pkg/sync/syncer.goRepository: ConductorOne/baton-sdk
Length of output: 982
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Look for what happens when a grant is appended multiple times
# Check if there's any de-duplication before the PutGrants call in processGrantsWithExternalPrincipals
echo "=== Lines 2840-2860 (full context before PutGrants in processGrantsWithExternalPrincipals) ==="
sed -n '2840,2860p' pkg/sync/syncer.go
# Get the complete line context around PutGrants call in processGrantsWithExternalPrincipals
echo -e "\n=== Lines 2850-2858 (PutGrants in processGrantsWithExternalPrincipals) ==="
sed -n '2850,2858p' pkg/sync/syncer.goRepository: ConductorOne/baton-sdk
Length of output: 765
Duplicate grants in PutGrants calls: two code paths append the same grant ID twice.
The double-append issue in processGrantsWithExternalPrincipals is real. At lines 2710 and 2743, the same newGrant object is appended twice—first on line 2710, then again on line 2743 after its annotations are mutated. This produces duplicate grant IDs in a single PutGrants call, wasting work regardless of whether the store treats them as upserts or insert-only.
The merge risk in syncGrantsForResource is also real. Lines 2090–2096 append etaggedGrants and resp.GetList() in sequence without deduplication; any overlapping grant IDs will be sent to PutGrants twice. The comment indicates the order should determine precedence ("newer data takes precedence"), but duplicates still risk failure or wasted writes.
Fix the double-append by moving the final append to after all annotation updates:
Example fix pattern for the double-append issue
- newGrant := newGrantForExternalPrincipal(grant, principal)
- expandedGrants = append(expandedGrants, newGrant)
-
- newGrantAnnos := annotations.Annotations(newGrant.GetAnnotations())
+ newGrant := newGrantForExternalPrincipal(grant, principal)
+ newGrantAnnos := annotations.Annotations(newGrant.GetAnnotations())
@@
newGrantAnnos.Update(newExpandableAnno)
newGrant.SetAnnotations(newGrantAnnos)
- expandedGrants = append(expandedGrants, newGrant)
}
+ expandedGrants = append(expandedGrants, newGrant)For syncGrantsForResource, either deduplicate by grant ID before calling PutGrants, or rely on documented upsert semantics if available.
Also applies to: 2340–2349, 2454–2463, 2642–2750, 2793–2844
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@pkg/sync/syncer.go` around lines 2168 - 2172,
processGrantsWithExternalPrincipals currently appends the same newGrant twice
(once before mutating annotations and again after), so move the final append of
newGrant to after all annotation/mutation logic completes (refer to newGrant and
the append calls around processGrantsWithExternalPrincipals) so each grant ID is
only added once; separately, in syncGrantsForResource where etaggedGrants and
resp.GetList() are concatenated before calling s.store.PutGrants, deduplicate
the combined slice by grant ID (or explicitly document and rely on upsert
semantics) so PutGrants never receives duplicate grant IDs (refer to
etaggedGrants, resp.GetList, and PutGrants).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
pkg/dotc1z/clone_sync.go (1)
46-65: Consider handling the empty columns edge case.If no common columns exist between source and destination schemas, this function returns an empty slice. This would cause
cloneTableQueryto generate malformed SQL:INSERT INTO clone.tablename () SELECT FROM tablename WHERE sync_id=?While unlikely in practice, adding a defensive check would make the code more robust.
🛡️ Suggested defensive check
func cloneCommonColumns(ctx context.Context, conn *sql.Conn, tableName string) ([]string, error) { srcColumns, err := cloneTableColumns(ctx, conn, "main", tableName) if err != nil { return nil, err } dstColumns, err := cloneTableColumns(ctx, conn, "clone", tableName) if err != nil { return nil, err } var columns []string for _, col := range dstColumns { if slices.Contains(srcColumns, col) { columns = append(columns, col) } } + if len(columns) == 0 { + return nil, fmt.Errorf("no common columns found for table %s", tableName) + } return columns, nil }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@pkg/dotc1z/clone_sync.go` around lines 46 - 65, The function cloneCommonColumns can return an empty slice which leads to malformed SQL in cloneTableQuery; update cloneCommonColumns to detect when the computed columns slice is empty and return a descriptive error (e.g., "no common columns between main and clone for table <tableName>") instead of an empty list so callers like cloneTableQuery can abort safely; ensure the error includes the tableName to aid debugging and adjust call sites if they currently assume a non-nil empty slice.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@pkg/dotc1z/clone_sync.go`:
- Around line 46-65: The function cloneCommonColumns can return an empty slice
which leads to malformed SQL in cloneTableQuery; update cloneCommonColumns to
detect when the computed columns slice is empty and return a descriptive error
(e.g., "no common columns between main and clone for table <tableName>") instead
of an empty list so callers like cloneTableQuery can abort safely; ensure the
error includes the tableName to aid debugging and adjust call sites if they
currently assume a non-nil empty slice.
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.
Reverts #666
Summary by CodeRabbit
Bug Fixes
Refactor