Skip to content

Conversation

elizabethhealy
Copy link
Member

Proposed Changes

  • move the connect rpc wrapper code gen out of internal so it can be re-used in sdk extension

Checklist

  • I have added or updated unit tests
  • I have added or updated integration tests (if appropriate)
  • I have added or updated documentation

Testing Instructions

@github-actions github-actions bot added comp:sdk A software development kit, including library, for client applications and inter-service communicati size/m labels May 28, 2025
Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Hello @elizabethhealy, 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!

Summary of Changes

As gemini-code-assist, I've reviewed this pull request. The primary goal of this PR is to refactor the ConnectRPC wrapper code generation tool within the SDK. Previously located in an internal directory, the tool and its logic are being moved to a more accessible location (sdk/codegen) to facilitate reusability, particularly for SDK extensions. This involves restructuring the code generation entry point and making the core generation function more flexible by passing configuration parameters to it.

Highlights

  • Enhanced Reusability: The core generation logic in sdk/codegen/runner/generate.go has been refactored to accept the list of clients and the output directory as arguments, making it reusable for external SDK extensions.
  • Updated Build and Documentation: The Makefile targets (proto-generate, connect-wrapper-generate) and the sdk/codegen/README.md documentation have been updated to reflect the new location and usage of the codegen tool.

Changelog

Click here to see the changelog
  • Makefile
    • Updated go run commands in proto-generate and connect-wrapper-generate targets to point to the new ./sdk/codegen path.
  • sdk/codegen/README.md
    • Renamed the title from 'SDK Internal Codegen' to 'SDK Codegen'.
    • Updated the overview description to reflect the focus on ConnectRPC components.
    • Corrected the reference to the clientsToGenerateList variable location from runner/generate.go to main.go.
    • Updated the example command for running code generation to use the new ./sdk/codegen path.
  • sdk/codegen/main.go
    • New file added.
    • Contains the main function which serves as the entry point for the codegen tool.
    • Defines the clientsToGenerateList variable specifying which gRPC interfaces to generate ConnectRPC wrappers for.
    • Determines the output directory (sdkconnect) and calls the refactored runner.Generate function.
  • sdk/codegen/runner/generate.go
    • Removed the hardcoded clientsToGenerateList variable.
    • Refactored the Generate function to accept clientsToGenerateList and outputDir as parameters.
    • Renamed the clientsToGenerate struct to ClientsToGenerate and exported its fields.
    • Updated internal references to use the exported field names (e.g., client.GrpcClientInterface).
    • Removed the getCurrentFileDir helper function as the output directory is now passed in.
    • Updated the output path calculation to use the provided outputDir.
  • sdk/internal/codegen/main.go
    • Deleted the old main entry point file for the internal codegen tool.
Using Gemini Code Assist

The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.

Feature Command Description
Code Review /gemini review Performs a code review for the current pull request in its current state.
Pull Request Summary /gemini summary Provides a summary of the current pull request in its current state.
Comment @gemini-code-assist Responds in comments when explicitly tagged, both in issue comments and review comments.
Help /gemini help Displays a list of available commands.

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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.

Limitations & Feedback

Gemini Code Assist is currently in preview and 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 to provide feedback.

You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.


Code moves, path is new,
Reusable now for all,
Build scripts updated.

Footnotes

  1. Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution.

Copy link
Contributor

@gemini-code-assist gemini-code-assist bot left a comment

Choose a reason for hiding this comment

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

Code Review

This pull request effectively refactors the ConnectRPC wrapper codegen logic by moving it from an internal package to a new sdk/codegen package. This change successfully achieves the goal of making the codegen reusable for SDK extensions. The restructuring is clean, and the necessary path and visibility adjustments have been made correctly.

Overall, the changes are well-executed and improve the codebase structure. I have one minor suggestion for an error message to enhance clarity.

Summary of Findings

  • Incorrect Error Message Context: In sdk/codegen/main.go, the getCurrentFileDir function contains an error message that incorrectly refers to generate.go instead of main.go. This could cause minor confusion during debugging.

Merge Readiness

The pull request is well-structured and achieves its goal of making the codegen reusable. There is one medium-severity issue regarding an error message that should be addressed for clarity. Once that's updated, the PR should be in good shape for merging. As a reviewer, I am not authorized to approve pull requests, so please ensure other necessary reviews and approvals are obtained before merging.

Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 467.19591ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 288.709792ms

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 348.327058ms
Throughput 287.09 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 53.606081031s
Average Latency 533.832558ms
Throughput 93.27 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 4963
Failed Requests 37
Concurrent Requests 50
Total Time 44.656236497s
Average Latency 442.84225ms
Throughput 111.14 requests/second

Error Summary:

Error Message Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error
rpc error: code = Internal desc = could not perform access
37 occurrences

Standard Benchmark Metrics Skipped or Failed

Copy link
Contributor

Benchmark results, click to expand

Benchmark authorization.GetDecisions Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 484.146921ms

Benchmark authorization.v2.GetMultiResourceDecision Results:

Metric Value
Approved Decision Requests 5000
Denied Decision Requests 0
Total Time 280.282929ms

Bulk Benchmark Results

Metric Value
Total Decrypts 100
Successful Decrypts 100
Failed Decrypts 0
Total Time 343.855717ms
Throughput 290.82 requests/second

TDF3 Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 5000
Failed Requests 0
Concurrent Requests 50
Total Time 56.114483668s
Average Latency 557.877048ms
Throughput 89.10 requests/second

NANOTDF Benchmark Results:

Metric Value
Total Requests 5000
Successful Requests 4948
Failed Requests 52
Concurrent Requests 50
Total Time 44.531492766s
Average Latency 440.061501ms
Throughput 111.11 requests/second

Error Summary:

Error Message Occurrences
ReadNanoTDF error: getNanoRewrapKey: rewrapError: internal: internal error
rpc error: code = Internal desc = could not perform access
52 occurrences

Standard Benchmark Metrics Skipped or Failed

@elizabethhealy elizabethhealy marked this pull request as ready for review May 29, 2025 03:02
@elizabethhealy elizabethhealy requested review from a team as code owners May 29, 2025 03:02
@elizabethhealy elizabethhealy added this pull request to the merge queue May 29, 2025
Merged via the queue into main with commit 8b29392 May 29, 2025
29 checks passed
@elizabethhealy elizabethhealy deleted the dspx-528-move-codegen-connect branch May 29, 2025 03:18
github-merge-queue bot pushed a commit that referenced this pull request May 29, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.4.7](sdk/v0.4.6...sdk/v0.4.7)
(2025-05-29)


### Features

* **sdk:** Expose connectrpc wrapper codegen for re-use
([#2322](#2322))
([8b29392](8b29392))

---
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>
strantalis pushed a commit to strantalis/platform that referenced this pull request May 29, 2025
### Proposed Changes

* move the connect rpc wrapper code gen out of internal so it can be
re-used in sdk extension

### Checklist

- [ ] I have added or updated unit tests
- [ ] I have added or updated integration tests (if appropriate)
- [ ] I have added or updated documentation

### Testing Instructions
strantalis pushed a commit to strantalis/platform that referenced this pull request May 29, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.4.7](opentdf/platform@sdk/v0.4.6...sdk/v0.4.7)
(2025-05-29)


### Features

* **sdk:** Expose connectrpc wrapper codegen for re-use
([opentdf#2322](opentdf#2322))
([8b29392](opentdf@8b29392))

---
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>
github-merge-queue bot pushed a commit that referenced this pull request Aug 26, 2025
🤖 I have created a release *beep* *boop*
---


##
[0.7.0](sdk/v0.6.1...sdk/v0.7.0)
(2025-08-25)


### ⚠ BREAKING CHANGES

* **core:** Require go 1.23+
([#1979](#1979))

### Features

* add system metadata assertions to TDFConfig
([#2446](#2446))
([4eb9fff](4eb9fff))
* **authz:** authz v2 versioning implementation
([#2173](#2173))
([557fc21](557fc21))
* **core:** adds bulk rewrap to sdk and service
([#1835](#1835))
([11698ae](11698ae))
* **core:** Adds EC withSalt options
([#2126](#2126))
([67b6fb8](67b6fb8))
* **core:** Adds ErrInvalidPerSchema
([#1860](#1860))
([456639e](456639e))
* **core:** DSPX-608 - Deprecate public_client_id
([#2185](#2185))
([0f58efa](0f58efa))
* **core:** EXPERIMENTAL: EC-wrapped key support
([#1902](#1902))
([652266f](652266f))
* **core:** Expose version info
([#1841](#1841))
([92a9f5e](92a9f5e))
* **core:** Require go 1.23+
([#1979](#1979))
([164c922](164c922))
* **core:** v2 ERS with proto updates
([#2210](#2210))
([a161ef8](a161ef8))
* **policy:** actions service RPCs should actually hit storage layer
CRUD ([#2063](#2063))
([da4faf5](da4faf5))
* **policy:** Add list key mappings rpc.
([#2533](#2533))
([fbc2724](fbc2724))
* **policy:** adds new public keys table
([#1836](#1836))
([cad5048](cad5048))
* **policy:** Allow the deletion of a key.
([#2575](#2575))
([82b96f0](82b96f0))
* **policy:** Default Platform Keys
([#2254](#2254))
([d7447fe](d7447fe))
* **policy:** DSPX-902 NDR service crud implementation (2/2)
([#2066](#2066))
([030ad33](030ad33))
* **policy:** key management crud
([#2110](#2110))
([4c3d53d](4c3d53d))
* **sdk:** Add a KAS allowlist
([#2085](#2085))
([d7cfdf3](d7cfdf3))
* **sdk:** add nanotdf plaintext policy
([#2182](#2182))
([e5c56db](e5c56db))
* **sdk:** adds seeker interface to TDF Reader
([#2385](#2385))
([63ccd9a](63ccd9a))
* **sdk:** Allow key splits with same algo
([#2454](#2454))
([7422b15](7422b15))
* **sdk:** Allow schema validation during TDF decrypt
([#1870](#1870))
([b7e6fb2](b7e6fb2))
* **sdk:** autoconfig kaos with kids
([#2438](#2438))
([c272016](c272016))
* **sdk:** bump protocol/go v0.6.0
([#2536](#2536))
([23e4c2b](23e4c2b))
* **sdk:** CreateTDF option to run with specific target schema version
([#2045](#2045))
([0976b15](0976b15))
* **sdk:** Enable base key support.
([#2425](#2425))
([9ff3806](9ff3806))
* **sdk:** Expose connectrpc wrapper codegen for re-use
([#2322](#2322))
([8b29392](8b29392))
* **sdk:** MIC-1436: User can decrypt TDF files created with
FileWatcher2.0.8 and older.
([#1833](#1833))
([f77d110](f77d110))
* **sdk:** remove hex encoding for segment hash
([#1805](#1805))
([d7179c2](d7179c2))
* **sdk:** sdk.New should validate platform connectivity and provide
precise error ([#1937](#1937))
([aa3696d](aa3696d))
* **sdk:** Use ConnectRPC in the go client
([#2200](#2200))
([fc34ee6](fc34ee6))


### Bug Fixes

* Allow parsing IPs as hostnames
([#1999](#1999))
([d54b550](d54b550))
* **ci:** Fix intermittent failures from auth tests
([#2345](#2345))
([395988a](395988a))
* **ci:** Update expired ca and certs in oauth unit tests
([#2113](#2113))
([5440fcc](5440fcc))
* **core:** Autobump sdk
([#1863](#1863))
([855cb2b](855cb2b))
* **core:** Autobump sdk
([#1873](#1873))
([085ac7a](085ac7a))
* **core:** Autobump sdk
([#1894](#1894))
([201244e](201244e))
* **core:** Autobump sdk
([#1917](#1917))
([edeeb74](edeeb74))
* **core:** Autobump sdk
([#1941](#1941))
([0a5a948](0a5a948))
* **core:** Autobump sdk
([#1948](#1948))
([4dfb457](4dfb457))
* **core:** Autobump sdk
([#1968](#1968))
([7084061](7084061))
* **core:** Autobump sdk
([#1972](#1972))
([7258f5d](7258f5d))
* **core:** Autobump sdk
([#2102](#2102))
([0315635](0315635))
* **core:** Fixes protoJSON parse bug on ec rewrap
([#1943](#1943))
([9bebfd0](9bebfd0))
* **core:** Improves errors when under heavy load
([#2132](#2132))
([4490a14](4490a14))
* **core:** Update fixtures and flattening in sdk and service
([#1827](#1827))
([d6d6a7a](d6d6a7a))
* **core:** Updates ec-wrapped to newer salt
([#1961](#1961))
([0e17968](0e17968))
* **deps:** bump github.com/docker/docker from 28.2.2+incompatible to
28.3.3+incompatible in /sdk
([#2597](#2597))
([a68d00d](a68d00d))
* **deps:** bump github.com/opentdf/platform/lib/ocrypto from 0.2.0 to
0.3.0 in /sdk ([#2502](#2502))
([3ec8b35](3ec8b35))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.3.6 to
0.4.0 in /sdk ([#2397](#2397))
([99e3aa4](99e3aa4))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.4.0 to
0.5.0 in /sdk ([#2471](#2471))
([e8f97e0](e8f97e0))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.5.0 to
0.5.1 in /sdk ([#2505](#2505))
([4edab72](4edab72))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.6.0 to
0.6.2 in /sdk ([#2586](#2586))
([4ed9856](4ed9856))
* **deps:** bump github.com/opentdf/platform/protocol/go from 0.6.2 to
0.7.0 in /sdk ([#2627](#2627))
([e775e14](e775e14))
* **deps:** bump golang.org/x/oauth2 from 0.26.0 to 0.30.0 in /sdk
([#2252](#2252))
([9b775a2](9b775a2))
* **deps:** bump google.golang.org/grpc from 1.71.0 to 1.72.1 in /sdk
([#2244](#2244))
([49484e0](49484e0))
* **deps:** bump the external group across 1 directory with 5 updates
([#2400](#2400))
([0b7ea79](0b7ea79))
* **deps:** bump toolchain in /lib/fixtures and /examples to resolve CVE
GO-2025-3563 ([#2061](#2061))
([9c16843](9c16843))
* Improve http.Client usage for security and performance
([#1910](#1910))
([e6a53a3](e6a53a3))
* **sdk:** adds connection options to getPlatformConfiguration
([#2286](#2286))
([a3af31e](a3af31e))
* **sdk:** Allow reuse of session key
([#2016](#2016))
([d48c11e](d48c11e))
* **sdk:** bump lib/ocrypto to 0.1.8
([#1938](#1938))
([53fa8ab](53fa8ab))
* **sdk:** bump protocol/go module dependencies
([#2078](#2078))
([e027f43](e027f43))
* **sdk:** Display proper error on kas rewrap failure
([#2081](#2081))
([508cbcd](508cbcd))
* **sdk:** everything is `mixedSplits` now
([#1861](#1861))
([ba78f14](ba78f14))
* **sdk:** Fix compatibility between bulk and non-bulk rewrap
([#1914](#1914))
([74abbb6](74abbb6))
* **sdk:** Fixed token expiration time
([#1854](#1854))
([c3cda1b](c3cda1b))
* **sdk:** perfsprint lint issues
([#2208](#2208))
([d36a078](d36a078))
* **sdk:** Prefer KID and Algorithm selection from key maps
([#2475](#2475))
([98fd392](98fd392))
* **sdk:** Removes unnecessary down-cast of `int`
([#1869](#1869))
([66f0c14](66f0c14))
* **sdk:** Version config fix
([#1847](#1847))
([be5d817](be5d817))
* Service utilize `httputil.SafeHttpClient`
([#1926](#1926))
([af32700](af32700))
* set consistent system metadata id and schema
([#2451](#2451))
([5db3cf2](5db3cf2))

---
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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
comp:sdk A software development kit, including library, for client applications and inter-service communicati size/m
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants