Rather than separate fields, these could live in this.config (already this client's config holder). See my notes on the builder and the isValid calls here.

Rather than separate fields, these could live in this.config (already this client's config holder). See my notes on the builder and the isValid calls here.

Refer to my notes on this here.

Refer to my notes on this here.

My suggestion is to take the take buffer and jitter out of the cache key. All the changes in this file are just to add buffer/jitter to the cache key. So can we revet this file to main?

The cache key identifies the token: same credentials -> same token. Buffer and jitter don't have any role in the token the IdP returns, they only affect when we refresh it. So putting them in the key just creates a second cache entry, and a second token exchange, for the same credentials whenever the timing differs. The key should only hold what determines the token.

Nothing's lost (in terms of the functionality added in this PR) by removing them form the cache key: OAuth2Client reads the timing straight from the config when it's built, and ensureOAuth2Client already passes the config through.

Minor consistency: Assign the defaults in the constructor - rather than inline initializers.

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?

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.

Small consistency nit: The setter has @throws but is missing @param and @return. Could you round add those to match other setters? Thank you.

Also added a note to the reader so they know the behavior at the edges.

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.

Housekeeping nit: Could you add a short Javadoc with @return? Thank you.

Small consistency nit: The setter has @throws but is missing @param and @return. Could you round add those to match other setters? Thank you.

Also added a note to the reader so they know the behavior at the edges.

Housekeeping nit: Could you add a short Javadoc with @return? Thank you.

Could we move buffer and jitter into the @MethodSource args here instead of reading them off a default Configuration? Right now every case runs at 300/300, so isValid is only ever checked at the old default: the test would still pass even if it ignored the buffer/jitter arguments. With them as inputs we can add cases that prove it honors non-default values, e.g. a token expiring in 5 minutes being invalid at buffer=400 but valid at buffer=30.

We don't need to use Configuration here either, this is a unit test of isValid, which just takes two ints, so coupling it to the config defaults isn't really relevant here.

Nit: could the CsvSource get a header or a short comment on what each column drives (lifetime/buffer/jitter → expected exchanges)? Takes a second to work out why each row is 1 or 2 otherwise. Thank you.

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.

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);
}