feat: non-transaction write fix, retry improvements, telemetry export #206
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughAdds per-tuple write result modeling and two-mode (transactional/non-transactional) write handling in the client. Introduces retry/backoff utilities and enhances HTTP retry telemetry and header parsing. Updates metrics to use GlobalOpenTelemetry. Refreshes examples, integration tests, and Gradle wrapper/settings. Expands ClientWriteResponse with detailed write/delete results. Changes
Sequence Diagram(s)sequenceDiagram
participant App
participant OpenFgaClient
participant FGA_API
Note over OpenFgaClient: Transactional mode
App->>OpenFgaClient: writeTuples(writes, deletes)
OpenFgaClient->>FGA_API: POST /write (all tuples)
FGA_API-->>OpenFgaClient: 2xx
OpenFgaClient-->>App: ClientWriteResponse (all inputs marked SUCCESS)
Note over OpenFgaClient: Non-Transactional mode
App->>OpenFgaClient: writeTuples(writes, deletes, nonTx opts)
loop chunks
OpenFgaClient->>FGA_API: POST /write (chunk)
alt 2xx
OpenFgaClient-->>OpenFgaClient: mark chunk tuples SUCCESS
else Auth error
OpenFgaClient-->>App: throw FgaApiAuthenticationError
else Other error
OpenFgaClient-->>OpenFgaClient: mark chunk tuples FAILURE with error
end
end
OpenFgaClient-->>App: Aggregated per-tuple statuses
sequenceDiagram
participant OpenFgaClient
participant HttpRequestAttempt
participant RetryStrategy
participant FGA_API
OpenFgaClient->>HttpRequestAttempt: send(request)
HttpRequestAttempt->>FGA_API: HTTP request
FGA_API-->>HttpRequestAttempt: response/error
alt retryable error/status
HttpRequestAttempt->>RetryStrategy: shouldRetry(response/error)
RetryStrategy-->>HttpRequestAttempt: decision, delay (uses RetryAfterHeaderParser / ExponentialBackoff)
HttpRequestAttempt-->>HttpRequestAttempt: record resend-count telemetry
HttpRequestAttempt->>FGA_API: retry after delay
end
HttpRequestAttempt-->>OpenFgaClient: final response/error
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested reviewers
✨ Finishing Touches🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (21)
gradle/wrapper/gradle-wrapper.properties (1)
3-3: Pin the wrapper withdistributionSha256Sumfor supply-chain hardening.Add the checksum to verify the downloaded Gradle distribution. Optional: keep distribution type consistent across the repo (root uses
-all, example uses-bin).Apply this change (replace placeholder with the actual checksum):
distributionUrl=https\://services.gradle.org/distributions/gradle-8.14.3-all.zip +distributionSha256Sum=<replace-with-gradle-8.14.3-all.zip-sha256>You can use the script from my other comment to resolve and inject the correct SHA.
settings.gradle (1)
1-6: Optional: also centralize dependency repositories via dependencyResolutionManagement.If you want to prevent subprojects from declaring their own repositories ad hoc and ensure consistent artifact resolution, add a top-level
dependencyResolutionManagementblock.You could add:
dependencyResolutionManagement { repositoriesMode.set(RepositoriesMode.PREFER_PROJECT) // or FAIL_ON_PROJECT_REPOS repositories { mavenCentral() // add any others you need (e.g., mavenLocal(), company mirrors) } }src/main/java/dev/openfga/sdk/telemetry/Metrics.java (2)
38-38: Prefer meterBuilder and set instrumentation version (optional, forward-looking).Using
meterBuilderallows setting an instrumentation version and schema, which improves observability and attribution in backends. This also avoids reliance on the deprecatedget(name)style in newer OTel APIs.Apply:
- this.meter = GlobalOpenTelemetry.get().getMeterProvider().get("openfga-sdk"); + this.meter = GlobalOpenTelemetry.get() + .getMeterProvider() + .meterBuilder("openfga-sdk") + .setInstrumentationVersion( + Metrics.class.getPackage() != null ? Metrics.class.getPackage().getImplementationVersion() : null) + .build();Note:
getImplementationVersion()reads from your JAR manifest’sImplementation-Version. If unavailable, it gracefully passesnull(allowed).
38-38: Thread-safety of metric caches (optional).
countersandhistogramsareHashMaps accessed without synchronization; ifMetricsis used across threads, considerConcurrentHashMapwithcomputeIfAbsent.Outside this line range, consider:
private final Map<String, LongCounter> counters = new ConcurrentHashMap<>(); private final Map<String, DoubleHistogram> histograms = new ConcurrentHashMap<>(); // Usage: counters.computeIfAbsent(counter.getName(), name -> meter.counterBuilder(name).setDescription(counter.getDescription()).build());src/main/java/dev/openfga/sdk/util/ExponentialBackoff.java (2)
64-69: Input validation for baseDelay is correct; consider updating Javadoc.The new checks for negative and zero durations are good and align with tests. Consider documenting the thrown exceptions and the behavior when
retryCount < 0(currently returnsDuration.ZERO).You might update the Javadoc above
calculateDelay(int, Duration)to include:
- Throws IllegalArgumentException when baseDelay is null, zero, or negative.
- Returns Duration.ZERO when retryCount is negative.
73-79: Avoid overflow when computing exponential delay (optional).
(long) Math.pow(2, retryCount) * baseDelayMscan overflow before the cap is applied, yielding incorrect negative values for extreme inputs. A saturating doubling loop avoids this.Apply:
- long exponentialDelayMs = (long) Math.pow(2, retryCount) * baseDelayMs; - - // Cap at maximum delay - long maxDelayMs = MAX_DELAY_SECONDS * 1000L; - if (exponentialDelayMs > maxDelayMs) { - exponentialDelayMs = maxDelayMs; - } + long maxDelayMs = MAX_DELAY_SECONDS * 1000L; + long exponentialDelayMs = baseDelayMs; + for (int i = 0; i < retryCount && exponentialDelayMs < maxDelayMs; i++) { + if (exponentialDelayMs > maxDelayMs / 2) { // prevent overflow + exponentialDelayMs = maxDelayMs; + break; + } + exponentialDelayMs *= 2; + } + if (exponentialDelayMs > maxDelayMs) { + exponentialDelayMs = maxDelayMs; + }This keeps semantics while ensuring no overflow even with large retry counts.
src/test/java/dev/openfga/sdk/util/ExponentialBackoffTest.java (1)
198-234: Input validation tests for baseDelay are correct and aligned with implementation
- Negative base delay (two-arg) and (three-arg with Random) correctly assert the exact messages.
- Zero base delay case is also covered with the expected message.
To reduce repetition and improve consistency:
- Consider consolidating these three tests into a parameterized test.
- Optionally unify on a single assertion style (either AssertJ’s assertThatThrownBy or JUnit’s assertThrows) across the class for consistency.
src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java (1)
185-197: Simplify header parsing and trim value to avoid false negativesMinor cleanup: avoid calling firstValue twice and trim the header before parsing to reduce NumberFormatException on values with incidental whitespace. Consider optional debug logging on malformed values.
Apply this diff:
- if (response.headers().firstValue("fga-query-duration-ms").isPresent()) { - String queryDuration = - response.headers().firstValue("fga-query-duration-ms").orElse(null); - - if (!isNullOrWhitespace(queryDuration)) { - try { - double queryDurationDouble = Double.parseDouble(queryDuration); - telemetry.metrics().queryDuration(queryDurationDouble, this.getTelemetryAttributes()); - } catch (NumberFormatException e) { - // Ignore malformed fga-query-duration-ms header values to prevent exceptions - // on otherwise valid responses. The telemetry metric will simply not be recorded. - } - } - } + response.headers() + .firstValue("fga-query-duration-ms") + .ifPresent(queryDuration -> { + if (!isNullOrWhitespace(queryDuration)) { + try { + double queryDurationDouble = Double.parseDouble(queryDuration.trim()); + telemetry.metrics().queryDuration(queryDurationDouble, this.getTelemetryAttributes()); + } catch (NumberFormatException e) { + // Ignore malformed fga-query-duration-ms header values to prevent exceptions + // on otherwise valid responses. The telemetry metric will simply not be recorded. + // (Optional) add debug log if you want visibility when this occurs. + } + } + });example/example1/src/main/java/dev/openfga/sdk/example/Example1.java (2)
21-23: Environment-based credential wiring looks good; consider validating both ID and secretReading issuer, clientId, and clientSecret from env is great. Small robustness nit: the guard currently checks only FGA_CLIENT_ID; consider requiring both ID and SECRET (and optionally issuer) to avoid constructing credentials with a missing secret.
Example adjustment (outside the shown range):
if (System.getenv("FGA_CLIENT_ID") != null && System.getenv("FGA_CLIENT_SECRET") != null) { ... }
105-118: Nice example payload to exercise non-transactional writes; consider printing per-tuple resultsIncluding a duplicate and a different relation is a good way to showcase the new per-tuple write semantics. To highlight the new API surface, consider capturing the ClientWriteResponse and printing each tuple’s status and any message for developer visibility.
Example (outside the shown range):
var writeResponse = fgaClient.write(...).get(); // Pseudocode – adjust getters to actual model names: writeResponse.getWriteResults().forEach(r -> System.out.printf("tuple=%s status=%s msg=%s%n", r.getTupleKey(), r.getStatus(), r.getMessage()));Also consider extracting the document ID to a local variable to avoid repeating the long literal.
src/test-integration/java/dev/openfga/sdk/example/Example1.java (2)
21-23: Env-driven credentials mirrored in integration example; add a second null-checkSame suggestion as the example app: ensure both FGA_CLIENT_ID and FGA_CLIENT_SECRET are present before constructing credentials to avoid partially configured auth in CI.
104-118: Good end-to-end payload for non-transaction mode; consider asserting per-tuple outcomesSince this runs in integration tests, it could validate the new per-tuple behavior by asserting expected statuses (e.g., duplicate returns failure and owner relation succeeds) when disableTransactions(true) is set.
If assertions are out of scope here, at least log the per-tuple statuses to aid debugging flakiness in CI environments.
src/main/java/dev/openfga/sdk/api/client/model/ClientWriteResponse.java (2)
24-33: Defensive-copy and expose unmodifiable result lists to avoid external mutationThe new
writesanddeletesfields hold references to caller-provided lists. Since the fields are returned directly via getters, callers can mutate the response afterward. Prefer copying and returning unmodifiable lists to preserve immutability guarantees.Apply this diff to defensively copy and expose unmodifiable lists:
public ClientWriteResponse(ApiResponse<Object> apiResponse) { this.statusCode = apiResponse.getStatusCode(); this.headers = apiResponse.getHeaders(); this.rawResponse = apiResponse.getRawResponse(); - this.writes = Collections.emptyList(); - this.deletes = Collections.emptyList(); + this.writes = Collections.emptyList(); + this.deletes = Collections.emptyList(); } public ClientWriteResponse(List<ClientWriteSingleResponse> writes, List<ClientWriteSingleResponse> deletes) { this.statusCode = 200; this.headers = Collections.emptyMap(); this.rawResponse = ""; - this.writes = writes != null ? writes : Collections.emptyList(); - this.deletes = deletes != null ? deletes : Collections.emptyList(); + this.writes = writes != null ? Collections.unmodifiableList(List.copyOf(writes)) : Collections.emptyList(); + this.deletes = deletes != null ? Collections.unmodifiableList(List.copyOf(deletes)) : Collections.emptyList(); }
35-41: Consider preserving HTTP metadata (status, headers, raw) when synthesizing per-tuple responsesThis constructor hardcodes
statusCode=200, empty headers, and empty raw response. In transactional/non-transactional write paths you already have the API response available; discarding headers (e.g., request IDs, server timing) may reduce diagnosability/telemetry.Recommend adding an additional constructor to carry HTTP metadata plus the per-tuple results:
// New overload (in addition to the existing constructors) public ClientWriteResponse( int statusCode, Map<String, List<String>> headers, String rawResponse, List<ClientWriteSingleResponse> writes, List<ClientWriteSingleResponse> deletes ) { this.statusCode = statusCode; this.headers = headers != null ? headers : Collections.emptyMap(); this.rawResponse = rawResponse != null ? rawResponse : ""; this.writes = writes != null ? Collections.unmodifiableList(List.copyOf(writes)) : Collections.emptyList(); this.deletes = deletes != null ? Collections.unmodifiableList(List.copyOf(deletes)) : Collections.emptyList(); }With that in place, callers (e.g., OpenFgaClient.writeTransactions/writeTuples/deleteTuples) can propagate metadata instead of dropping it. Can you confirm if any consumers rely on these headers today?
src/main/java/dev/openfga/sdk/api/client/model/ClientWriteSingleResponse.java (2)
17-17: Make the response holder class finalThis is an immutable data holder (final fields, only getters). Declaring the class
finalprevents unintended subclassing and aligns with its immutable intent.-public class ClientWriteSingleResponse { +public final class ClientWriteSingleResponse {
15-16: Add null-safety for required fields (tupleKey, status)To fail fast and make misuse obvious, guard required fields in the constructor. This prevents obscure NPEs later.
import dev.openfga.sdk.api.model.TupleKey; +import java.util.Objects; public final class ClientWriteSingleResponse { @@ public ClientWriteSingleResponse(TupleKey tupleKey, ClientWriteStatus status, Exception error) { - this.tupleKey = tupleKey; - this.status = status; + this.tupleKey = Objects.requireNonNull(tupleKey, "tupleKey"); + this.status = Objects.requireNonNull(status, "status"); this.error = error; }Also applies to: 26-30
.openapi-generator/FILES (1)
320-322: Duplicate file entry detected
src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.javaappears twice. This can confuse tooling relying on this manifest.Apply this diff to remove the duplicate:
src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.java -src/test/java/dev/openfga/sdk/api/OpenFgaApiTest.javasrc/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java (1)
1269-1344: Solid coverage for non-transactional partial failures; consider asserting error typeGreat end-to-end verification: request sequencing, headers, and per-tuple statuses in the response. To further harden this test, add an assertion that the failure’s error is of type
FgaApiValidationErrorand hasstatusCode=400to ensure error typing is preserved.Example enhancement:
assertTrue(writes.get(1).getError() instanceof FgaApiValidationError); assertEquals(400, ((FgaApiValidationError) writes.get(1).getError()).getStatusCode());src/main/java/dev/openfga/sdk/api/client/OpenFgaClient.java (3)
498-519: Preserve HTTP metadata when returning synthesized per-tuple resultsHere the API response is ignored and a synthetic
ClientWriteResponseis returned. Consider preservingstatusCode, headers, and raw response for observability and debugging.If you add the constructor suggested in ClientWriteResponse, this block can be adjusted to:
return call(() -> api.write(storeId, body, overrides)).thenApply(apiResponse -> { // build writeResponses/deleteResponses as you already do... return new ClientWriteResponse( apiResponse.getStatusCode(), apiResponse.getHeaders(), apiResponse.getRawResponse(), writeResponses, deleteResponses ); });I can follow up with a PR-ready patch across both files if you’d like.
714-720: writeTuples: also consider propagating HTTP metadataSimilar to
writeTransactions, this path discards headers/raw response. If you adopt the new constructor, you can carry metadata through while still returning per-tuple results.Suggested change after adding the new constructor:
return call(() -> api.write(storeId, body, overrides)).thenApply(apiResponse -> { List<ClientWriteSingleResponse> writeResponses = tupleKeys.stream() .map(tuple -> new ClientWriteSingleResponse(tuple.asTupleKey(), ClientWriteStatus.SUCCESS)) .collect(Collectors.toList()); return new ClientWriteResponse( apiResponse.getStatusCode(), apiResponse.getHeaders(), apiResponse.getRawResponse(), writeResponses, List.of() ); });
754-765: deleteTuples: also consider propagating HTTP metadataSame rationale as
writeTransactions/writeTuples: keep status/headers/raw if possible.Suggested change after adding the new constructor:
return call(() -> api.write(storeId, body, overrides)).thenApply(apiResponse -> { List<ClientWriteSingleResponse> deleteResponses = tupleKeys.stream() .map(tuple -> new ClientWriteSingleResponse( new TupleKey().user(tuple.getUser()).relation(tuple.getRelation())._object(tuple.getObject()), ClientWriteStatus.SUCCESS)) .collect(Collectors.toList()); return new ClientWriteResponse( apiResponse.getStatusCode(), apiResponse.getHeaders(), apiResponse.getRawResponse(), List.of(), deleteResponses ); });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
💡 Knowledge Base configuration:
- MCP integration is disabled by default for public repositories
- Jira integration is disabled by default for public repositories
- Linear integration is disabled by default for public repositories
You can enable these sources in your CodeRabbit configuration.
📒 Files selected for processing (15)
.openapi-generator/FILES(4 hunks)example/example1/gradle/wrapper/gradle-wrapper.properties(1 hunks)example/example1/src/main/java/dev/openfga/sdk/example/Example1.java(2 hunks)gradle/wrapper/gradle-wrapper.properties(1 hunks)settings.gradle(1 hunks)src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java(2 hunks)src/main/java/dev/openfga/sdk/api/client/OpenFgaClient.java(6 hunks)src/main/java/dev/openfga/sdk/api/client/model/ClientWriteResponse.java(2 hunks)src/main/java/dev/openfga/sdk/api/client/model/ClientWriteSingleResponse.java(1 hunks)src/main/java/dev/openfga/sdk/api/client/model/ClientWriteStatus.java(1 hunks)src/main/java/dev/openfga/sdk/telemetry/Metrics.java(2 hunks)src/main/java/dev/openfga/sdk/util/ExponentialBackoff.java(3 hunks)src/test-integration/java/dev/openfga/sdk/example/Example1.java(2 hunks)src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java(6 hunks)src/test/java/dev/openfga/sdk/util/ExponentialBackoffTest.java(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java (1)
src/main/java/dev/openfga/sdk/telemetry/Attributes.java (1)
Attributes(31-216)
src/test/java/dev/openfga/sdk/util/ExponentialBackoffTest.java (1)
src/main/java/dev/openfga/sdk/util/ExponentialBackoff.java (1)
ExponentialBackoff(27-91)
src/main/java/dev/openfga/sdk/api/client/OpenFgaClient.java (1)
src/main/java/dev/openfga/sdk/api/client/model/ClientWriteRequest.java (1)
ClientWriteRequest(17-46)
🔇 Additional comments (12)
settings.gradle (1)
1-6: LGTM: pluginManagement repositories configured.Defining
gradlePluginPortal()andmavenCentral()at the settings level is a solid move for reproducible plugin resolution.src/main/java/dev/openfga/sdk/telemetry/Metrics.java (1)
17-17: Switch to GlobalOpenTelemetry is appropriate.Using
GlobalOpenTelemetryaligns with standard OTel initialization patterns and preserves no-op behavior when not configured.src/main/java/dev/openfga/sdk/util/ExponentialBackoff.java (2)
17-17: Good improvement: use ThreadLocalRandom for jitter.This avoids shared RNG contention and is safer in concurrent contexts.
43-44: LGTM: route public calculateDelay through ThreadLocalRandom.Keeps default behavior fast and thread-safe while retaining the testable overload.
src/test/java/dev/openfga/sdk/util/ExponentialBackoffTest.java (1)
17-17: Good addition: JUnit 5 assertThrows import is correctly usedThe static import aligns with the new exception tests and keeps the code concise.
src/main/java/dev/openfga/sdk/api/client/model/ClientWriteStatus.java (1)
15-18: Enum introduction looks goodThe SUCCESS/FAILURE statuses are a clear and minimal API for per-tuple write results and match the new response model.
src/main/java/dev/openfga/sdk/api/client/HttpRequestAttempt.java (1)
128-130: Retry telemetry attribute correctly recorded for HTTP errorsAdding HTTP_REQUEST_RESEND_COUNT on HTTP retry paths makes retry telemetry consistent with network error retries and with the attribute added on successful responses.
src/test/java/dev/openfga/sdk/api/client/OpenFgaClientTest.java (2)
1162-1167: LGTM: Good asserts on per-tuple results in transactional pathThe added checks validate the new
ClientWriteResponsestructure and tuple-level SUCCESS status as expected for transactional writes.
2009-2009: LGTM: Clarified stubbed response to include structured error payloadThe updated WireMock stub improves realism for the error case.
src/main/java/dev/openfga/sdk/api/client/OpenFgaClient.java (3)
361-405: Javadoc clearly documents the new transactional vs non-transactional semanticsThe mode-specific behavior, error handling, and caller responsibilities are well explained.
415-461: Javadoc duplication: consistent and helpful on the overloadThe duplicated documentation on the overloaded
writeis consistent and improves usability.
521-547: Javadoc for non-transactional write is thoroughClear articulation of chunking, error isolation, and ordering guarantees.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #206 +/- ##
============================================
+ Coverage 34.58% 35.19% +0.60%
- Complexity 1043 1071 +28
============================================
Files 185 187 +2
Lines 6980 7087 +107
Branches 790 803 +13
============================================
+ Hits 2414 2494 +80
- Misses 4463 4483 +20
- Partials 103 110 +7 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
sync changes from geneator:
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Chores