Skip to content

[CXP-1251] - Promote v0.2.6-test to main: paginate group member grants - #85

Closed
agustin-conductor wants to merge 1 commit into
mainfrom
bugfix/promote-paginate-group-member-grants
Closed

[CXP-1251] - Promote v0.2.6-test to main: paginate group member grants#85
agustin-conductor wants to merge 1 commit into
mainfrom
bugfix/promote-paginate-group-member-grants

Conversation

@agustin-conductor

@agustin-conductor agustin-conductor commented Oct 29, 2026

Copy link
Copy Markdown
Contributor

Promotes the change validated in the v0.2.6-test release to main.

This contains only #81 (6384cfa1), cherry-picked onto the current main. staging also has later changes (Unity Catalog metastore sync #70 and incremental sync #56) that weren't part of the validated release, so this PR doesn't merge staging as a whole.

main has received dependency bumps since the tag was cut, so the commit was cherry-picked rather than taken from the tag directly. It applied with no conflicts, and pkg/ and cmd/ are identical to v0.2.6-test.

Change (from #81)

Group and workspace member grants are now paged locally, 2000 per page, using a key cursor, so a single Grants response stays under the 6MB Lambda payload limit. Previously, large groups failed with HTTP 413 in sync-grants-for-resource.

Checks

  • go build ./... and go test ./... pass on the cherry-picked branch.
  • golangci-lint run ./... reports 0 issues.

🤖 Generated with Claude Code

@agustin-conductor @claude
[CXP-1251] - Paginate group member grants to stay under the Lambda 6M…
f792fb9 
…B response limit (#81)

* Paginate group and workspace member grants to stay under Lambda payload limit

Databricks returns every group member in a single Get group response and has
no member pagination. Grants() emitted one grant per member in one response,
so a group with ~11k+ members (e.g. an all-users account group) exceeded the
6MB Lambda response limit and failed with HTTP 413 on every retry.

Page members locally (GrantsPageSize = 2000, ~1.1MB per page) using the page
token as a member offset, and emit role rule-set grants on the first page
only. Databricks returns members in a different order on every call, so they
are sorted before slicing so pages line up across re-fetches in separate
Lambda invocations. Apply the same paging to workspace assignment grants.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

* Page member grants by last emitted key instead of offset

An offset page token into a list that is re-fetched for every page drifts
when members are added or removed between page invocations: a removal skips
the first member of the next page (C1 reads that grant as revoked) and an
addition grants one member twice. Store the key of the last emitted item
(member Ref for groups, principal ID for workspaces) and start the next page
after it, so membership churn no longer shifts page boundaries.

Add churn tests for both paths, a workspace multi-page test, and mark the
test-only shuffle as not security sensitive for gosec.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 <noreply@anthropic.com>
@agustin-conductor
agustin-conductor requested a review from a team October 29, 2026 14:45
@linear-code

linear-code Bot commented Oct 29, 2026

Copy link
Copy Markdown

CXP-1251

@github-actions

github-actions Bot commented Oct 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor
Superseded — see the current review report for commit f792fb95103d

Connector PR Review: [CXP-1251] - Promote v0.2.6-test to main: paginate group member grants

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base c0a476de267d.
Review mode: full
View review run

Review Summary

This PR changes groupBuilder.Grants and workspaceBuilder.Grants so they no longer return every member or assignment in one response.

  • How paging works: each call fetches the full list from the API, sorts it by a stable key (member Ref for groups, the principal ID as a string for workspaces), and returns up to GrantsPageSize (2000) entries after the cursor. The cursor is the last key emitted and is stored in a pagination.Bag (pkg/connector/pagination.go:68-110).
  • Role grants: group role (rule-set) grants are emitted only on the first page.
  • Workspace results: the workspace builder now also returns its rate-limit annotations and a NextPageToken.

I scanned the full PR diff for security and correctness and checked these behaviors against the code:

  • Paging logic: pageAfter works whether or not the cursor key is still in the list. When the cursor sorts after every key, it returns an empty last page.
  • End of paging: bag.NextToken("") pops the page state and returns an empty token, so paging terminates.
  • Errors: errors from the token parse, the group fetch and ListRuleSets are all returned, never swallowed. That satisfies R7 and S1/S5: no empty-result-on-error path is added.

I applied the repo-local criteria:

  • S (data loss on empty results): no new empty-on-error returns.
  • L (log levels): the new logging is Debug only.
  • F2: local chunking of a list that is already in memory is explicitly allowed by C8.
  • B (breaking changes): no resource type, entitlement slug or ID changes.
  • Not relevant here: E/T/J/PR, because there are no spans, no provisioning changes, no new response structs and no new endpoints. B10 does not apply either.

go.mod and go.sum are unchanged.

New tests cover multi-page grants, members added or removed between pages, and the single-page case for small groups.

Coverage gap: I did not verify the description's claim that pkg/ and cmd/ are byte-identical to v0.2.6-test. The review is based only on the code in this diff.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

@github-actions

github-actions Bot commented Oct 29, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Connector PR Review: [CXP-1251] - Promote v0.2.6-test to main: paginate group member grants

Blocking Issues: 0 | Suggestions: 0 | Threads Resolved: 0
Criteria: Criteria status: loaded .claude/skills/ci-review.md from trusted base c0a476de267d.
Review mode: full
View review run

Review Summary

This PR changes groupBuilder.Grants and workspaceBuilder.Grants so they no longer return every member or assignment in one response.

  • How paging works: each call fetches the full list from the API, sorts it by a stable key (member Ref for groups, the principal ID as a string for workspaces), and returns up to GrantsPageSize (2000) entries after the cursor. The cursor is the last key emitted and is stored in a pagination.Bag (pkg/connector/pagination.go:68-110).
  • Role grants: group role (rule-set) grants are emitted only on the first page.
  • Workspace results: the workspace builder now also returns its rate-limit annotations and a NextPageToken.

I scanned the full PR diff for security and correctness and checked these behaviors against the code:

  • Paging logic: pageAfter works whether or not the cursor key is still in the list. When the cursor sorts after every key, it returns an empty last page.
  • End of paging: bag.NextToken("") pops the page state and returns an empty token, so paging terminates.
  • Errors: errors from the token parse, the group fetch and ListRuleSets are all returned, never swallowed. That satisfies R7 and S1/S5: no empty-result-on-error path is added.

I applied the repo-local criteria:

  • S (data loss on empty results): no new empty-on-error returns.
  • L (log levels): the new logging is Debug only.
  • F2: local chunking of a list that is already in memory is explicitly allowed by C8.
  • B (breaking changes): no resource type, entitlement slug or ID changes.
  • Not relevant here: E/T/J/PR, because there are no spans, no provisioning changes, no new response structs and no new endpoints. B10 does not apply either.

go.mod and go.sum are unchanged.

New tests cover multi-page grants, members added or removed between pages, and the single-page case for small groups.

Coverage gap: I did not verify the description's claim that pkg/ and cmd/ are byte-identical to v0.2.6-test. The review is based only on the code in this diff.

Security Issues

None found.

Correctness Issues

None found.

Suggestions

None.

Reviewed commit: f792fb95103d

github-actions[bot]
github-actions Bot reviewed Oct 29, 2026
View reviewed changes

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No blocking issues found — see the full review report

@agustin-conductor

agustin-conductor commented Oct 29, 2026

Copy link
Copy Markdown
Contributor Author

Closing in favor of a promotion PR from a branch at the v0.2.6-test commit, so the PR head is exactly the validated commit.

@agustin-conductor agustin-conductor mentioned this pull request Oct 29, 2026
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

1 participant

@agustin-conductor