Repository navigation
Conversation
Allow setting the expiry buffer and jitter while preserving existing defaults. Include refresh settings in the OAuth client cache key. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Keep ClientCredentials limited to token request parameters. Preserve refresh policy through request overrides. Co-Authored-By: Codex GPT-5 <noreply@openai.com>
Co-Authored-By: Codex GPT-5 <noreply@openai.com>
|
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (8)
Included review availability: Your plan provides up to 1 included review per hour; 0 remain after this review. WalkthroughThe change adds configurable token-expiry buffer and jitter settings. OAuth2 token validation uses these settings, configuration overrides preserve them, and OAuth2 client cache keys include them. Tests and documentation cover the new behavior. ChangesOAuth2 refresh timing
Estimated code review effort: 3 (Moderate) | ~20 minutes Severity of issue fixed: Medium Sequence Diagram(s)sequenceDiagram
participant ApiClient
participant OAuth2Client
participant AccessToken
ApiClient->>OAuth2Client: Request access token
OAuth2Client->>AccessToken: Validate with buffer and jitter
AccessToken-->>OAuth2Client: Return validity result
OAuth2Client-->>ApiClient: Reuse or refresh token
Suggested reviewers: Merge Risk: ⚪ Minimal · up to The SDK now supports configurable client-credentials token refresh timing while preserving defaults. Configuration overrides, cache separation, and refresh behavior are covered, with no concrete merge-blocking risk evident. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 28.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 25 functions across 7 files. (1 skipped: 1 unsupported.) ✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 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 |
|
should i remove codex for cla to be accepted? |
|
@dpkass yes, you'll need to remove codex |
Codecov Report❌ Patch coverage is ❌ Your project status has failed because the head coverage (39.42%) is below the target coverage (80.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #389 +/- ##
============================================
+ Coverage 39.34% 39.42% +0.08%
- Complexity 1336 1341 +5
============================================
Files 202 202
Lines 7791 7815 +24
Branches 912 913 +1
============================================
+ Hits 3065 3081 +16
- Misses 4579 4585 +6
- Partials 147 149 +2 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
|
Hi @dpkass |
There was a problem hiding this comment.
Thanks for this, @dpkass, and for raising #388 alongside it. A hard-coded 300s buffer plus up-to-300s jitter means short-lived tokens get treated as stale almost immediately, so making the timing configurable is a sensible and backward-compatible fix. The direction is good. I've left inline comments on the Files changed tab; collecting the main points here so they're in one place.
1. Design
- buffer and jitter config (client level vs request level vs under Credentials object) - here
- removing buffer and jitter from the token cache key - here
2. Cross-SDK consistency
These SDKs are meant to behave consistently, so in parallel I'll check whether the other OpenFGA SDKs have the same hard-coded buffer/jitter and whether they expose configurable timing. If we settle on a shape here, it's worth tracking matching changes in the others (ideally coordinated through sdk-generator where it applies) so the behavior stays aligned across languages. This PR effectively sets the pattern the others would follow, which is another reason to get the design right here first.
3. Suggested sequencing
I'd suggest picking the comments related to buffer/jitter config and the cache-key design first so the other comments naturally follow.
4. Tests and coverage
Added inline comments where needed
Please let me know if there are any questions or if you need any clarifications.
Thank you again for the contribution.
| result.tokenExpiryBufferSeconds(tokenExpiryBufferSeconds); | ||
| result.tokenExpiryJitterSeconds(tokenExpiryJitterSeconds); |
There was a problem hiding this comment.
These two lines copy the client-level buffer/jitter as is, unlike every other field in override(). And buffer/jitter are the only two Configuration fields not mirrored in ConfigurationOverride.
Here are my thoughts.
I think the current approach is fine. I don't think these should be per-request overridable. The access token is shared per identity, so "use buffer=X for this one call" has no well-defined meaning on a shared token: whichever request happens to trigger the refresh wins, and it affects every other caller using that token. That's unlike credentials (overriding them yields a genuinely different token) or timeouts/headers (which apply only to your own request).
There is a legitimate case where these values could differ: two credentials whose tokens have very different lifetimes (say 1h vs 5m), where a single buffer is wrong for one of them. But that is per-credential, not per-request, so the natural home would be on the Credentials object (the policy travels with the identity). I'd treat that as a follow-up, not part of this PR.
So my suggestion: keep these as client-level only (don't add them to ConfigurationOverride), and add a short comment here noting that's intentional.
@curfew-marathon, @SoulPancake - Does this make sense or is there something I'm missing? What do you suggest?
| return this; | ||
| } | ||
|
|
||
| public int getTokenExpiryBufferSeconds() { |
There was a problem hiding this comment.
Housekeeping nit: Could you add a short Javadoc with @return? Thank you.
| return this; | ||
| } | ||
|
|
||
| public int getTokenExpiryJitterSeconds() { |
There was a problem hiding this comment.
Housekeeping nit: Could you add a short Javadoc with @return? Thank you.
| private int maxRetries; | ||
| private Duration minimumRetryDelay; | ||
| private int tokenExpiryBufferSeconds = FgaConstants.TOKEN_EXPIRY_THRESHOLD_BUFFER_IN_SEC; | ||
| private int tokenExpiryJitterSeconds = FgaConstants.TOKEN_EXPIRY_JITTER_IN_SEC; |
There was a problem hiding this comment.
Minor consistency: Assign the defaults in the constructor - rather than inline initializers.
| // Reuse the token only while it is outside the configured refresh window. | ||
| HttpRequest.Builder secondBuilder = HttpRequest.newBuilder().uri(URI.create(FgaConstants.TEST_API_URL)); | ||
| apiClient.applyAuthHeader(secondBuilder, configuration); | ||
| apiClient.applyAuthHeader(secondBuilder, configuration.override(new ConfigurationOverride())); |
There was a problem hiding this comment.
Any particular reason an override is used here? An empty one merges to the same config, so it reuses the same cache entry either way.
|
|
||
| @ParameterizedTest | ||
| @CsvSource({"31,10", "30,11"}) | ||
| void clientCredentials_differentRefreshSettings_useSeparateCaches(int buffer, int jitter) throws Exception { |
There was a problem hiding this comment.
This comment should be picked once https://github.com/openfga/java-sdk/pull/389/changes#r4156168685 is resolved.
This test exercises the behavior I flagged over on ApiClient.java (buffer/jitter in the cache key) here: same credentials with different timing producing separate token exchanges. It's tied to that discussion, so once we settle it there and take timing out of the key, this test should be deleted.
Once we remove the above, it'd be worth replacing it with the inverse, a guard that same credentials share one cached client regardless of refresh timing, which is exactly the contract we'd be committing to. Sharing a skeleton below:
@Test
void clientCredentials_differentRefreshSettings_shareOneCache() throws Exception {
HttpClientMock mockHttpClient = new HttpClientMock();
mockHttpClient
.onPost(FgaConstants.TEST_ISSUER_URL + "/oauth/token")
.doReturn(200, "{\"access_token\":\"token\",\"expires_in\":3600}");
ApiClient apiClient = new ApiClient(mockHttpClientBuilder(mockHttpClient));
ClientCredentials credentials = new ClientCredentials()
.clientId("client")
.clientSecret("secret")
.apiTokenIssuer(FgaConstants.TEST_ISSUER_URL);
Configuration first = new Configuration()
.credentials(new Credentials(credentials))
.tokenExpiryBufferSeconds(30)
.tokenExpiryJitterSeconds(10);
Configuration second = new Configuration()
.credentials(new Credentials(credentials))
.tokenExpiryBufferSeconds(300)
.tokenExpiryJitterSeconds(0);
apiClient.applyAuthHeader(HttpRequest.newBuilder(), first);
apiClient.applyAuthHeader(HttpRequest.newBuilder(), second);
// Same credentials => one cached client/token, regardless of refresh timing.
mockHttpClient.verify().post(FgaConstants.TEST_ISSUER_URL + "/oauth/token").called(1);
}
| * Defaults to 300. This is a client-level setting, preserved by request overrides. | ||
| * @throws IllegalArgumentException if seconds is negative. | ||
| */ | ||
| public Configuration tokenExpiryBufferSeconds(int seconds) { |
There was a problem hiding this comment.
The new buffer/jitter setters and getters don't have unit coverage in ConfigurationTest. Could you please add the relevant tests in ConfigurationTest file?
For each of buffer and jitter: a valid set/read-back (with the assertSame chaining check), the zero case (0 is meaningful here, it disables the buffer / disables jitter), the negative-value IllegalArgumentException (like minimumRetryDelay_negativeDuration_throwsException), and a _hasDefaultValue test asserting 300. And others that I might have missed.
You could refer to the minimumRetryDelay_* cluster and other test cases in the file for reference.
| Credentials overrideCredentials = configurationOverride.getCredentials(); | ||
| result.credentials(overrideCredentials != null ? overrideCredentials : credentials); | ||
| result.tokenExpiryBufferSeconds(tokenExpiryBufferSeconds); | ||
| result.tokenExpiryJitterSeconds(tokenExpiryJitterSeconds); |
There was a problem hiding this comment.
This comment should be picked up after https://github.com/openfga/java-sdk/pull/389/changes#r4154934650 is resolved.
Without these two lines, override() would reset buffer/jitter to the 300 defaults on every request. Nothing in the test file covers that currently. Worth a small direct unit test in ConfigurationTest, in the override_* style, something like:
@Test
void override_preservesTokenExpirySettings() {
// Given
Configuration original = new Configuration().tokenExpiryBufferSeconds(42).tokenExpiryJitterSeconds(7);
// When
Configuration result = original.override(new ConfigurationOverride());
// Then
assertEquals(42, result.getTokenExpiryBufferSeconds());
assertEquals(7, result.getTokenExpiryJitterSeconds());
}
That matches the existing override_apiUrl / override_userAgent tests. Feel free to also assert the original is unmodified if you want to mirror them fully.
| } | ||
| ``` | ||
|
|
||
| #### Token refresh timing |
There was a problem hiding this comment.
@SoulPancake - checking placement: README.md is autogenerated (it's in .openapi-generator/FILES, and CONTRIBUTING notes changes to those files go through sdk-generator), so as-is this edit will be overwritten on the next generation, right? Could you confirm whether these changes should go to sdk-generator instead?
There was a problem hiding this comment.
Yes it should be nice to have the changes in the generator to avoid drift later but it's okay for the maintainers to do that chore instead of the contributors.
So, non-blocking for the PR context.
Description
What problem is being solved?
Five-minute access tokens are immediately considered stale by the fixed 300-second refresh buffer plus jitter, causing repeated token exchanges.
How is it being solved?
Make the refresh buffer and jitter configurable at the client level while preserving existing defaults.
What changes are made to solve it?
tokenExpiryBufferSecondsandtokenExpiryJitterSecondssettings toConfigurationand fluentClientConfiguration. Zero jitter disables jitter.Validation:
./gradlew build test-integrationpassed on Java 21. Formatting and focused tests also passed after removing a redundant setter-validation test.References
Closes #388
Review Checklist
main).Summary by CodeRabbit