Skip to content

feat(sdk)!: switch to connect-rpc for GRPC #244

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 62 commits into from
May 9, 2025
Merged

Conversation

mkleene
Copy link
Contributor

@mkleene mkleene commented Apr 21, 2025

  • pull a TokenSource out of GRPCAuthInterceptor. This lets us keep the token code in Java while using idiomatic Kotlin stuff to modify the request

buf changes

  • upgrade buf to v2
  • remove buf generation from examples. Unless we need to call tagging or something we already have everything we need in sdk
  • move the service definitions to test so that we don't ship things that we are just using for tests

dependency updates

address normalization

  • We take the scheme (http vs https) from the value of usePlaintext
  • If a port is specified we use it. If not we default based on the value of usePlaintext (true -> 80, false -> 443)

@mkleene mkleene changed the title Feature/connect rpc feat(sdk): use connect-rpc for GRPC Apr 21, 2025
@mkleene mkleene force-pushed the feature/connect-rpc branch from ad65930 to cb472ef Compare April 21, 2025 19:33
@mkleene mkleene marked this pull request as ready for review April 21, 2025 21:46
@mkleene mkleene requested a review from a team as a code owner April 21, 2025 21:46
@mkleene mkleene requested a review from Copilot April 30, 2025 14:14
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR updates the SDK to use connect-rpc for GRPC communication while also reorganizing the codebase to use generated service definitions instead of shipping test-only code. The key changes include replacing gRPC stubs with connect-rpc clients, updating buf configuration files to version v2 with new generation rules, and aligning example modules to use the new blocking API calls.

Reviewed Changes

Copilot reviewed 30 out of 35 changed files in this pull request and generated no comments.

Show a summary per file
File Description
sdk/src/main/java/io/opentdf/platform/sdk/KASClient.java Migrates from gRPC channels to connect-rpc with updated blocking calls and error handling.
sdk/src/main/java/io/opentdf/platform/sdk/Config.java Updates import references and minor code cleanup.
sdk/src/main/java/io/opentdf/platform/sdk/Autoconfigure.java Replaces policy package imports with generated equivalents and updates service calls.
sdk/src/main/java/io/opentdf/platform/sdk/AddressNormalizer.java Introduces a new utility for normalizing addresses compatible with plaintext settings.
sdk/buf.yaml and sdk/buf.gen.yaml Upgrades configuration versions and adjusts generation options.
examples/* Updates example calls to use the new connect-rpc blocking API methods.
Files not reviewed (5)
  • examples/buf.gen.yaml: Language not supported
  • examples/buf.yaml: Language not supported
  • examples/pom.xml: Language not supported
  • pom.xml: Language not supported
  • sdk/pom.xml: Language not supported
Comments suppressed due to low confidence (2)

sdk/src/main/java/io/opentdf/platform/sdk/KASClient.java:105

  • [nitpick] Consider using a unified error handling utility; here 'RequestHelper.getOrThrow' is used while other parts of the codebase use 'ResponseMessageKt.getOrThrow'.
resp = RequestHelper.getOrThrow(req);

sdk/src/main/java/io/opentdf/platform/sdk/KASClient.java:261

  • [nitpick] Consider using consistent variable naming for request objects. The variable 'req' is renamed to 'request' in close proximity, which might be confusing.
var request = getStub(keyAccess.url).rewrapBlocking(req, Collections.emptyMap()).execute();

@mkleene mkleene changed the title feat(sdk): use connect-rpc for GRPC feat!(sdk): use connect-rpc for GRPC May 7, 2025
@mkleene mkleene changed the title feat!(sdk): use connect-rpc for GRPC feat(sdk)!: use connect-rpc for GRPC May 7, 2025
@mkleene mkleene changed the title feat(sdk)!: use connect-rpc for GRPC feat(sdk)!: switch to connect-rpc for GRPC May 7, 2025
@mkleene mkleene requested a review from Copilot May 8, 2025 19:28
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR migrates the GRPC client implementation to use connect‑rpc along with several dependency and configuration updates. Key changes include:

  • Replacing gRPC channel and stub creation with OkHttpClient and ProtocolClient.
  • Upgrading buf configuration to v2 and updating code generation settings.
  • Adjusting address normalization logic and updating example usages to call new blocking APIs.

Reviewed Changes

Copilot reviewed 32 out of 37 changed files in this pull request and generated no comments.

Show a summary per file
File Description
sdk/src/main/java/io/opentdf/platform/sdk/KASClient.java Updated KASClient constructor, stub retrieval, and close logic to use connect‑rpc.
sdk/src/main/java/io/opentdf/platform/sdk/Config.java Cleaned up redundant imports.
sdk/src/main/java/io/opentdf/platform/sdk/Autoconfigure.java Updated attribute grants retrieval to use blocking client APIs.
sdk/src/main/java/io/opentdf/platform/sdk/AddressNormalizer.java Added new address normalization logic using URI with updated scheme handling.
sdk/buf.yaml and sdk/buf.gen.yaml Upgraded buf version and reconfigured code generation plugins.
examples/… Updated examples to use new blocking methods and connect‑rpc ResponseMessageKt utilities.
Files not reviewed (5)
  • examples/buf.gen.yaml: Language not supported
  • examples/buf.yaml: Language not supported
  • examples/pom.xml: Language not supported
  • pom.xml: Language not supported
  • sdk/pom.xml: Language not supported
Comments suppressed due to low confidence (2)

sdk/src/main/java/io/opentdf/platform/sdk/KASClient.java:252

  • [nitpick] The use of 'req' followed by 'request' in the same method can be confusing; consider using a consistent or more descriptive naming scheme to improve clarity.
var req = RewrapRequest.newBuilder()

sdk/src/main/java/io/opentdf/platform/sdk/KASClient.java:44

  • Changing KASClient visibility from public to package-private may limit external accessibility. Confirm if this change is intentional or consider reverting to public.
class KASClient implements SDK.KAS {

Co-authored-by: Dave Mihalcik <dmihalcik@virtru.com>
Copy link

sonarqubecloud bot commented May 8, 2025

Quality Gate Failed Quality Gate failed

Failed conditions
0.0% Coverage on New Code (required ≥ 80%)

See analysis details on SonarQube Cloud

@mkleene mkleene merged commit ff36a1d into main May 9, 2025
10 of 11 checks passed
@mkleene mkleene deleted the feature/connect-rpc branch May 9, 2025 13:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants