feat(sdk): source-file codegen for EntityIdentifier helpers #3232
feat(sdk): source-file codegen for EntityIdentifier helpers #3232marythought merged 15 commits intomainfrom
Conversation
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 5 minutes and 10 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (3)
📝 WalkthroughWalkthroughAdds a new standalone codegen tool in Changes
Sequence DiagramsequenceDiagram
participant Src as Internal Source\n(protocol/go/internal/*)
participant CG as Codegen Tool\n(protocol/codegen)
participant FS as Filesystem\n(protocol/go/*)
Src->>CG: Read mapped .go source files
CG->>CG: Rewrite imports & strip alias qualifiers
CG->>FS: Remove stale `*.gen.go` files
CG->>FS: Write new `*.gen.go` outputs
FS-->>FS: `entity_identifier.gen.go` created/updated
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request introduces a set of ergonomic constructor functions within the SDK to streamline the instantiation of authorization entity identifiers. By abstracting the underlying protocol buffer structure, these changes make the developer experience more intuitive and reduce the verbosity required for common authorization tasks. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. With builders clean and code so bright, / The nested structs now take to flight. / No more deep chains to make us weep, / Just simple calls for us to keep. Footnotes
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protocol/go/authorization/v2/entity_identifier_test.go`:
- Around line 34-98: The three tests TestForClientID, TestForEmail, and
TestForUserName duplicate the same assertions; refactor them into a single
table-driven test (e.g., TestEntityChainConstructors) that iterates over cases
with fields: name, constructor (ForClientID/ForEmail/ForUserName), input string,
a small checker to extract the expected value from the entity (using type
assertions to *entity.Entity_ClientId, *entity.Entity_EmailAddress,
*entity.Entity_UserName), and expected value; inside each subtest call the
constructor, use extractEntityChain and chain.GetEntities(), assert len==1, run
the checker and compare the returned value to expected, and assert
e.GetCategory() == entity.Entity_CATEGORY_SUBJECT to replace the three existing
tests.
- Around line 1-107: Add edge-case unit tests that exercise empty-string inputs
and ensure constructors behave as intended: create tests like
TestForClientID_EmptyString, TestForEmail_EmptyString,
TestForUserName_EmptyString and TestForToken_EmptyString that call
ForClientID(""), ForEmail(""), ForUserName(""), ForToken("") respectively, use
extractEntityChain(t, eid) (or GetIdentifier for Token) to retrieve the created
entity, assert the chain/entities length is still correct (e.g., 1) and that the
specific field (ClientId, EmailAddress, UserName, Token.Jwt) equals the empty
string (or whatever the defined behavior should be), and for WithRequestToken
consider a test asserting that WithRequestToken still returns true/false as
expected; make sure to follow existing test patterns (extractEntityChain, type
assertions like (*entity.Entity_ClientId), and category checks) to keep
consistency.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 77800d08-e88c-4e00-81e7-569b52d2fa87
📒 Files selected for processing (1)
protocol/go/authorization/v2/entity_identifier_test.go
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Pull request was converted to draft
|
Implementation approach is pending decision on ADR: Proto helper code generation. The current PR places helpers in the |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Move protocol/go/codegen/ to protocol/codegen/ with its own go.mod, per review feedback from dmihalcik-virtru. This removes the codegen directory from the protocol/go/ tree, simplifying the Makefile find cleanup (one exclusion instead of two) and scoping CI triggers. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com>
7a155ef
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
Add a single-entry go.work so the codegen module resolves itself without needing GOWORK=off in the Makefile. More self-documenting than an env var override. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com>
X-Test Failure Reportcukes-report |
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@protocol/codegen/go.mod`:
- Around line 1-3: Update the Go toolchain version in the module directive to a
patched release to silence stdlib OSV warnings; in the go.mod for module
github.com/opentdf/platform/protocol/codegen replace the "go 1.25.0" directive
with the patched version used in the workspace (e.g., "go 1.25.8") so the
codegen tool aligns with the workspace toolchain and addresses the flagged
stdlib advisories.
In `@protocol/go/authorization/v2/entity_identifier.gen.go`:
- Around line 10-64: The new helper constructors (ForToken, WithRequestToken,
ForClientID, ForEmail, ForUserName, entityIdentifierFromEntity) are available
and callers should use them instead of manually constructing EntityIdentifier
structs; update locations such as accessPdp.go to replace inline builds of
EntityIdentifier/EntityChain/Entity with calls to
ForClientID/ForEmail/ForUserName (or ForToken/WithRequestToken as appropriate)
so the code is simpler and consistent across the codebase.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 920c0c2c-c700-40a8-bbc8-d815ba7eb616
📒 Files selected for processing (8)
.github/workflows/checks.yamlMakefileprotocol/codegen/go.modprotocol/codegen/main.goprotocol/codegen/main_test.goprotocol/go/authorization/v2/entity_identifier.gen.goprotocol/go/internal/authorization/v2/entity_identifier.goprotocol/go/internal/authorization/v2/entity_identifier_test.go
- Add copyHelpers integration test covering file filtering, _test.go exclusion, .gen.go naming, header prepending, and stale file cleanup - Strip empty import blocks from generated output - Replace Jira reference with public PR link in codegen doc comment - Add explanatory comment to protocol/codegen/go.work - Bump go version to 1.25.5 to match root workspace Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Signed-off-by: Mary Dickson <mary.dickson@virtru.com>
Benchmark results, click to expandBenchmark authorization.GetDecisions Results:
Benchmark authorization.v2.GetMultiResourceDecision Results:
Benchmark Statistics
Bulk Benchmark Results
TDF3 Benchmark Results:
|
|
🤖 I have created a release *beep* *boop* --- ## [0.23.0](protocol/go/v0.22.0...protocol/go/v0.23.0) (2026-04-07) ### Features * **policy:** add sort support to ListAttributes API ([#3223](#3223)) ([ec3312f](ec3312f)) * **sdk:** source-file codegen for EntityIdentifier helpers ([#3232](#3232)) ([ee8177c](ee8177c)) --- This PR was generated with [Release Please](https://github.com/googleapis/release-please). See [documentation](https://github.com/googleapis/release-please#release-please). Co-authored-by: opentdf-automation[bot] <149537512+opentdf-automation[bot]@users.noreply.github.com>
Summary
ForToken,WithRequestToken,ForClientID,ForEmail,ForUserName) that live in theauthorizationv2proto package — no extra import neededprotocol/go/internal/authorization/v2/have full IDE support and standard unit tests; Go'sinternalpackage rule prevents external consumers from importing them directlyprotocol/codegen(separate Go module, stdlib only) copies source files into proto packages at build time, stripping the self-referencing import and type qualifiers so the output compiles in-packagemake proto-generateafterbuf generateand beforesdk/codegen.protofiles)Before / After
Before
Constructing an
EntityIdentifierrequires 4 levels of proto nesting:After
One-line convenience constructors in the same proto package:
All five constructors:
How it works
Source files import the proto package explicitly for compilation/IDE support. The codegen tool strips the self-referencing import and qualifier prefix when copying, producing clean in-package code. All source files are read and transformed before any existing
.gen.gofiles are removed, so a failed read won't leave the target directory empty.The codegen tool lives in
protocol/codegen/with its owngo.mod(stdlib only), outside theprotocol/go/tree. This means the Makefilefindcleanup only needs to excludeinternal/— no complicated exclusion patterns.Test plan
cd protocol/codegen && GOWORK=off go test ./...— import rewriting and stale file cleanup tests passcd protocol/codegen && GOWORK=off go run .— generates.gen.gowith correct import rewritinggo test ./protocol/go/internal/authorization/v2/...— source tests passgo build ./protocol/go/authorization/v2/...— proto package builds with generated helpersgo test ./sdk/...— no breakage from removing old sdk helper filesgolangci-lint run— 0 issues on all new files🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Chores
Tests