-
Notifications
You must be signed in to change notification settings - Fork 57
fix(java-sdk): ensure headers can be set and overriden #634
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
Conversation
Co-authored-by: Jim Anderson <[email protected]>
Co-authored-by: Jim Anderson <[email protected]>
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (6)
config/clients/java/template/example/example1/build.gradle.mustache (2)
21-37
: Align example with BOM-based Jackson management (consistency with main template)You can drop per‑artifact versions and import the jackson-bom to match the root template.
Apply within this block:
ext { - jacksonVersion = "2.20.0" + jacksonVersion = "2.20.0" } dependencies { implementation("dev.openfga:openfga-sdk:{{packageVersion}}") // Serialization - implementation("com.fasterxml.jackson.core:jackson-core:$jacksonVersion") - implementation("com.fasterxml.jackson.core:jackson-annotations:$jacksonVersion") - implementation("com.fasterxml.jackson.core:jackson-databind:$jacksonVersion") - implementation("com.fasterxml.jackson.datatype:jackson-datatype-jsr310:$jacksonVersion") - implementation("org.openapitools:jackson-databind-nullable:0.2.7") + implementation(platform("com.fasterxml.jackson:jackson-bom:$jacksonVersion")) + implementation("com.fasterxml.jackson.core:jackson-core") + implementation("com.fasterxml.jackson.core:jackson-annotations") + implementation("com.fasterxml.jackson.core:jackson-databind") + implementation("com.fasterxml.jackson.datatype:jackson-datatype-jsr310") + implementation("org.openapitools:jackson-databind-nullable:0.2.7")
3-4
: Align Spotless plugin version with main template and verify Gradle wrapper compatibility
- Spotless is pinned to 7.2.1 in
config/clients/java/template/build.gradle.mustache
but set to 8.0.0 inconfig/clients/java/template/example/example1/build.gradle.mustache
—align both.- Confirm the Gradle wrapper supports Spotless 8.0.0 (or 7.2.1 if you choose) and the Kotlin JVM plugin 2.2.20.
config/clients/java/template/src/main/api/configuration/ClientConfiguration.java.mustache (1)
133-137
: Make a defensive copy of default headersAvoid external mutation by copying the provided map before delegating.
Apply:
@Override public ClientConfiguration defaultHeaders(java.util.Map<String, String> defaultHeaders) { - super.defaultHeaders(defaultHeaders); + super.defaultHeaders(defaultHeaders == null ? null : new java.util.HashMap<>(defaultHeaders)); return this; }config/clients/java/template/src/main/api/client/OpenFgaClient.java.mustache (1)
541-547
: Header merge/override pattern is correct; consider deduplicationCopy‑on‑write of additionalHeaders with putIfAbsent for method/bulk headers fulfills “set and override” goals. Extract to a small helper to reduce repetition.
Example:
private Map<String,String> methodHeaders(Map<String,String> base, String methodName) { var headers = base != null ? new HashMap<>(base) : new HashMap<>(); headers.putIfAbsent(CLIENT_METHOD_HEADER, methodName); headers.putIfAbsent(CLIENT_BULK_REQUEST_ID_HEADER, randomUUID().toString()); return headers; } // usage: // options.additionalHeaders(methodHeaders(options.getAdditionalHeaders(), "ListRelations"));Also applies to: 808-814, 869-875, 1101-1107
config/clients/java/template/src/test/api/client/OpenFgaClientHeadersTest.java.mustache (1)
25-27
: Avoid gitleaks false positives by using obviously dummy IDs in testsStatic analysis flagged a “Generic API Key”. These are test constants, but switching to clearly fake IDs reduces CI noise and aligns with “no secrets in templates”.
Apply:
- private static final String DEFAULT_STORE_ID = "01YCP46JKYM8FJCQ37NMBYHE5X"; + private static final String DEFAULT_STORE_ID = "test-store-id"; ... - private static final String DEFAULT_AUTH_MODEL_ID = "01G5JAVJ41T49E9TT3SKVS7X1J"; + private static final String DEFAULT_AUTH_MODEL_ID = "test-auth-model-id";Note: All expectations reference these constants, so behavior remains unchanged. As per coding guidelines.
config/clients/java/template/build.gradle.mustache (1)
79-89
: Align Spotless plugin version with example template
In config/clients/java/template/build.gradle.mustache (line 7), updateid 'com.diffplug.spotless' version '7.2.1'
to8.0.0
to match the example1 template.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
config/clients/java/config.overrides.json
(1 hunks)config/clients/java/template/README_initializing.mustache
(1 hunks)config/clients/java/template/build.gradle.mustache
(4 hunks)config/clients/java/template/example/README.md
(1 hunks)config/clients/java/template/example/example1/build.gradle.mustache
(3 hunks)config/clients/java/template/example/example1/gradle/wrapper/gradle-wrapper.properties
(1 hunks)config/clients/java/template/example/example1/src/main/java/dev/openfga/sdk/example/Example1.java
(2 hunks)config/clients/java/template/gitignore.mustache
(1 hunks)config/clients/java/template/src/main/api/client/OpenFgaClient.java.mustache
(5 hunks)config/clients/java/template/src/main/api/configuration/ClientBatchCheckClientOptions.java.mustache
(2 hunks)config/clients/java/template/src/main/api/configuration/ClientConfiguration.java.mustache
(1 hunks)config/clients/java/template/src/main/api/configuration/ClientListRelationsOptions.java.mustache
(2 hunks)config/clients/java/template/src/test/api/client/OpenFgaClientHeadersTest.java.mustache
(1 hunks)config/clients/java/template/src/test/api/client/OpenFgaClientTest.java.mustache
(6 hunks)config/common/files/README.mustache
(1 hunks)
🧰 Additional context used
📓 Path-based instructions (4)
config/**/*.mustache
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Validate mustache syntax and variable references across all template files, including CHANGELOG.md.mustache
Files:
config/common/files/README.mustache
config/clients/java/template/src/main/api/configuration/ClientListRelationsOptions.java.mustache
config/clients/java/template/src/main/api/configuration/ClientConfiguration.java.mustache
config/clients/java/template/gitignore.mustache
config/clients/java/template/src/main/api/client/OpenFgaClient.java.mustache
config/clients/java/template/README_initializing.mustache
config/clients/java/template/src/main/api/configuration/ClientBatchCheckClientOptions.java.mustache
config/clients/java/template/src/test/api/client/OpenFgaClientTest.java.mustache
config/clients/java/template/example/example1/build.gradle.mustache
config/clients/java/template/src/test/api/client/OpenFgaClientHeadersTest.java.mustache
config/clients/java/template/build.gradle.mustache
config/**/*.{json,mustache}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Never hardcode API keys or credentials in configuration or template files
Files:
config/common/files/README.mustache
config/clients/java/template/src/main/api/configuration/ClientListRelationsOptions.java.mustache
config/clients/java/template/src/main/api/configuration/ClientConfiguration.java.mustache
config/clients/java/template/gitignore.mustache
config/clients/java/template/src/main/api/client/OpenFgaClient.java.mustache
config/clients/java/template/README_initializing.mustache
config/clients/java/template/src/main/api/configuration/ClientBatchCheckClientOptions.java.mustache
config/clients/java/template/src/test/api/client/OpenFgaClientTest.java.mustache
config/clients/java/template/example/example1/build.gradle.mustache
config/clients/java/config.overrides.json
config/clients/java/template/src/test/api/client/OpenFgaClientHeadersTest.java.mustache
config/clients/java/template/build.gradle.mustache
config/clients/*/config.overrides.json
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
config/clients/*/config.overrides.json
: Always update the packageVersion in each language-specific config.overrides.json when making version changes
Maintain FOSSA compliance notice IDs in each language’s config.overrides.json
Files:
config/clients/java/config.overrides.json
config/{common/config.base.json,clients/*/config.overrides.json}
📄 CodeRabbit inference engine (.github/copilot-instructions.md)
Ensure consistent versioning across base and language override configuration files to avoid version conflicts
Files:
config/clients/java/config.overrides.json
🧬 Code graph analysis (1)
config/clients/java/template/example/example1/src/main/java/dev/openfga/sdk/example/Example1.java (1)
config/clients/js/template/example/example1/example1.mjs (1)
authorizationModelId
(116-116)
🪛 Gitleaks (8.28.0)
config/clients/java/template/src/test/api/client/OpenFgaClientHeadersTest.java.mustache
[high] 27-27: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: build-and-test-java-sdk
🔇 Additional comments (15)
config/clients/java/template/example/example1/gradle/wrapper/gradle-wrapper.properties (1)
3-3
: Gradle version and distribution type updated.The Gradle wrapper has been updated from version 8.0 (bin distribution) to 8.2.1 (all distribution). The
-all
distribution includes sources and documentation, which is beneficial for IDE support and development but increases download size.config/common/files/README.mustache (1)
20-20
: LGTM!The Custom Headers section has been added to the Table of Contents, properly positioned under "Getting Started" and using correct markdown anchor syntax.
config/clients/java/template/example/README.md (1)
3-11
: LGTM!The README has been restructured to provide a clearer organization of available examples. The new structure with "Basic Examples" and "OpenTelemetry Examples" sections makes it easier for users to find relevant examples.
config/clients/java/template/example/example1/src/main/java/dev/openfga/sdk/example/Example1.java (2)
31-31
: LGTM!The formatting change condenses the method call to a single line, improving readability without altering functionality.
116-118
: LGTM!The comment placement has been improved for better clarity, with the "relation" comment on its own line. This is a cosmetic improvement with no functional impact.
config/clients/java/config.overrides.json (1)
569-610
: LGTM!The example file paths have been systematically remapped from
example/example1/
toexamples/basic-examples/
, which aligns with the PR objective to prepare for a future SDK-generator structure change. The mapping is consistent across all related files.Note: As per coding guidelines, ensure that
packageVersion
(line 6) is updated when making version changes. If this PR includes functional changes beyond path remapping, consider whether a version bump is needed.config/clients/java/template/src/main/api/configuration/ClientBatchCheckClientOptions.java.mustache (2)
5-5
: LGTM!The HashMap import has been added to support defensive copying of headers.
53-53
: LGTM!The defensive copy prevents external modification of the additionalHeaders map after it's passed to ClientCheckOptions. This is a good defensive programming practice that ensures header immutability and prevents unexpected side effects.
config/clients/java/template/README_initializing.mustache (1)
112-173
: LGTM!The Custom Headers documentation is comprehensive and well-structured. It clearly explains:
- Default headers applied to all requests via
ClientConfiguration.defaultHeaders()
- Per-request headers via options classes'
additionalHeaders()
method- Override behavior when both are present
The code examples are complete and demonstrate the functionality effectively. This documentation aligns perfectly with the PR objective.
config/clients/java/template/src/main/api/configuration/ClientListRelationsOptions.java.mustache (2)
5-5
: LGTM!The HashMap import has been added to support defensive copying of headers, consistent with the broader header handling pattern across the codebase.
54-54
: LGTM!The defensive copy ensures that the additionalHeaders map cannot be modified externally after being passed to ClientBatchCheckClientOptions. This is consistent with the defensive copying pattern applied in ClientBatchCheckClientOptions and follows immutability best practices.
config/clients/java/template/src/test/api/client/OpenFgaClientTest.java.mustache (1)
2403-2404
: LGTM: Updated expected client method header for ListRelationsHeader assertions now match the client’s "ListRelations" method tag.
Also applies to: 2423-2424, 2443-2444, 2461-2462, 2631-2632, 2655-2656
config/clients/java/template/src/main/api/client/OpenFgaClient.java.mustache (1)
241-247
: Authorization model id precedence fix (options over config) — goodUses options’ non‑blank value; otherwise falls back to configuration. Matches intended behavior.
Consider mirroring this pattern anywhere options can override config to keep behavior consistent.
config/clients/java/template/build.gradle.mustache (2)
100-103
: Test toolchain bumps — goodassertj/mockito/junit updates look sane and align with Java 11 target.
64-64
: Jackson version bump to 2.20.0 — approved. Verified all Jackson references use the BOM; no hard-pinned versions remain.
Description
Backports some work from Java:
example/example1
generates cleanly intoexamples/basic-examples
References
Review Checklist
main
Summary by CodeRabbit
New Features
Documentation
Chores
Tests