feat: Implement REST API retry mechanism #1102
Conversation
|
🚀 Thanks for opening this pull request! |
|
Warning Rate limit exceeded@kirkmorrow has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 11 minutes and 21 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📒 Files selected for processing (1)
Note Other AI code review bot(s) detectedCodeRabbit has detected other AI code review bot(s) in this pull request and will avoid duplicating their findings in the review comments. This may lead to a less comprehensive review. 📝 WalkthroughWalkthroughAdds a centralized executeWithRetry retry mechanism, surfaces configurable REST retry intervals (separate read/write) via Parse.initialize → ParseCoreData, applies retry wrappers to HTTP clients (Dio/Http), standardizes error JSON handling, improves non‑JSON error parsing, and adds related unit and integration tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant App as Client
participant SDK as Parse HTTP Client
participant Retry as executeWithRetry
participant Server as Parse Server
App->>SDK: perform HTTP operation (get/post/put/delete)
SDK->>Retry: executeWithRetry(operation: () => actual HTTP call, isWriteOperation?)
Retry->>SDK: invoke operation attempt `#1`
SDK->>Server: send HTTP request
alt network failure / HTML-like / status -1
Server-->>SDK: error / HTML / no-response
SDK-->>Retry: return retryable ParseNetworkResponse (status -1 / otherCause)
Retry-->>Retry: wait interval (from ParseCoreData().restRetryIntervals or restRetryIntervalsForWrites)
Retry->>SDK: invoke operation attempt `#2` (repeat per intervals)
else success (200/201) or non-retriable Parse error
Server-->>SDK: success or non-retriable error
SDK-->>Retry: return final ParseNetworkResponse
end
Retry-->>App: final ParseNetworkResponse (success or error)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Potential attention areas:
Suggested reviewers
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
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 |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #1102 +/- ##
==========================================
+ Coverage 44.26% 44.37% +0.10%
==========================================
Files 61 62 +1
Lines 3637 3725 +88
==========================================
+ Hits 1610 1653 +43
- Misses 2027 2072 +45 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
This PR implements an automatic retry mechanism for REST API network operations to handle transient failures from network issues, reverse proxies, and load balancers. The retry logic operates at the HTTP client layer with configurable exponential backoff.
Key Changes:
- Added
executeWithRetry()function that wraps HTTP operations with automatic retry capability for status code -1 (network errors) and HTML error responses - Enhanced error response handling to gracefully manage non-JSON responses from proxies/load balancers
- Introduced
restRetryIntervalsconfiguration parameter with sensible defaults[0, 250, 500, 1000, 2000]
Reviewed changes
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
packages/dart/lib/src/network/parse_network_retry.dart |
Core retry logic implementing exponential backoff with configurable intervals and HTML error detection |
packages/dart/lib/src/network/parse_http_client.dart |
Wraps all HTTP methods (get, post, put, delete, head, patch) with executeWithRetry() and standardizes error formatting |
packages/dart/lib/src/network/parse_dio_client.dart |
Wraps all Dio HTTP methods with executeWithRetry() and updates _fallbackErrorData to include error code |
packages/dart/lib/src/objects/response/parse_error_response.dart |
Adds FormatException handling to gracefully process HTML responses with helpful error messages |
packages/dart/lib/src/data/parse_core_data.dart |
Adds restRetryIntervals field with default configuration |
packages/dart/lib/parse_server_sdk.dart |
Exposes restRetryIntervals parameter in Parse.initialize() and registers retry module |
packages/dart/test/src/network/parse_network_retry_test.dart |
Comprehensive unit tests (26 tests) covering all retry scenarios, edge cases, and configuration validation |
packages/dart/test/src/network/parse_client_retry_integration_test.dart |
Integration tests (7 tests) documenting MockClient behavior and architectural design decisions |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![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 (6)
packages/dart/test/src/network/parse_client_retry_integration_test.dart (1)
147-154: Verify the fetch test assertion logic.The test expects
fetchedObject.objectIdto be'abc123'after a network error response. This is checking that the original object'sobjectIdis preserved, not that the fetch succeeded. Consider adding an assertion onresponse.successor the error to make the test intent clearer.final object = ParseObject('TestObject', client: client) ..objectId = 'abc123'; - final fetchedObject = await object.fetch(); + final response = await object.fetch(); expect(callCount, 1); // No retry via mock - expect(fetchedObject.objectId, 'abc123'); + expect(response.success, false); + expect(object.objectId, 'abc123'); // Original objectId preservedpackages/dart/test/src/network/parse_network_retry_test.dart (2)
147-173: Potential race condition in exception test.The
expect(..., throwsA(...))returns once the exception is thrown, but thecallCountis checked after aFuture.delayed(50ms). Since the retry intervals are[0, 10], the total delay should be ~10ms, but if the test execution is slow, this could be flaky.Consider restructuring to await all retries before checking:
test('should handle exceptions and retry', () async { int callCount = 0; final oldIntervals = ParseCoreData().restRetryIntervals; ParseCoreData().restRetryIntervals = [0, 10]; - expect( - () async => await executeWithRetry( + await expectLater( + executeWithRetry( operation: () async { callCount++; throw Exception('Network timeout'); }, ), throwsA( isA<Exception>().having( (e) => e.toString(), 'message', contains('Network timeout'), ), ), ); // Should retry on exceptions: initial + 2 retries = 3 times - await Future.delayed(Duration(milliseconds: 50)); // Wait for retries expect(callCount, 3); ParseCoreData().restRetryIntervals = oldIntervals; });
522-528: Test doesn't verify actual implementation behavior.This test only validates a static string format rather than testing that both HTTP clients produce the same error format. Consider removing it or replacing with a more meaningful test that actually exercises both clients.
packages/dart/lib/src/network/parse_network_retry.dart (3)
67-74: Clarify the max retries error message.The error message states "cannot exceed $maxRetries retries" but the actual number of attempts is
retryIntervals.length + 1(one initial attempt plus retries). This could confuse users about the limit. Consider rewording for clarity:if (retryIntervals.length > maxRetries) { throw ArgumentError( - 'restRetryIntervals cannot exceed $maxRetries retries. ' + 'restRetryIntervals cannot exceed $maxRetries retry attempts. ' 'Current length: ${retryIntervals.length}', ); }Or more precisely:
if (retryIntervals.length > maxRetries) { throw ArgumentError( - 'restRetryIntervals cannot exceed $maxRetries retries. ' + 'restRetryIntervals cannot exceed $maxRetries elements (which allows up to ${maxRetries + 1} total attempts). ' 'Current length: ${retryIntervals.length}', ); }
139-141: Consider throwing instead of using non-null assertion.While this code should be unreachable, the non-null assertion operator
!could cause issues if the compiler cannot provelastResponseis non-null. Consider throwing an error for clarity and safety:- // Should never reach here, but return last response as fallback - return lastResponse!; + // This should never be reached due to the loop logic above + throw StateError( + 'Retry loop completed without returning or rethrowing. This indicates a logic error.', + );This makes the "impossible" case explicit and easier to debug if it somehow occurs.
157-175: HTML detection optimization is optional; ParseError.otherCause value is correct.The check for
ParseError.otherCause == -1is correct. However, the suggested optimization to substring the response body has tradeoffs worth considering:The current code calls
trimLeft()andtoLowerCase()on the full response. If you want to optimize for large responses, you could limit the check to the first ~100 characters:- final String trimmedData = response.data.trimLeft().toLowerCase(); + // Only check first ~100 chars for HTML markers (proxy errors typically at the start) + final String prefix = response.data + .substring(0, response.data.length < 100 ? response.data.length : 100) + .trimLeft() + .toLowerCase(); // Check for common HTML patterns that indicate proxy/load balancer errors // More robust than just checking for '<' which could be in valid JSON strings - if (trimmedData.startsWith('<!doctype') || - trimmedData.startsWith('<html') || - trimmedData.startsWith('<head') || - trimmedData.startsWith('<body')) { + if (prefix.startsWith('<!doctype') || + prefix.startsWith('<html') || + prefix.startsWith('<head') || + prefix.startsWith('<body')) { // HTML response indicates proxy/load balancer error return true; }Note:
substring()always allocates a new string in Dart, whiletrimLeft()andtoLowerCase()only allocate if changes are needed. For typical proxy error pages (small, starting with whitespace or HTML), the optimization may have negligible impact. Apply only if profiling shows large response bodies are common in your retry scenarios.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (8)
packages/dart/lib/parse_server_sdk.dart(3 hunks)packages/dart/lib/src/data/parse_core_data.dart(3 hunks)packages/dart/lib/src/network/parse_dio_client.dart(7 hunks)packages/dart/lib/src/network/parse_http_client.dart(7 hunks)packages/dart/lib/src/network/parse_network_retry.dart(1 hunks)packages/dart/lib/src/objects/response/parse_error_response.dart(1 hunks)packages/dart/test/src/network/parse_client_retry_integration_test.dart(1 hunks)packages/dart/test/src/network/parse_network_retry_test.dart(1 hunks)
⏰ 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). (11)
- GitHub Check: Test Dart 3.10, Ubuntu
- GitHub Check: Test Dart 3.10, Windows
- GitHub Check: Test Flutter 3.38, Ubuntu, WASM
- GitHub Check: Test Dart beta
- GitHub Check: Test Dart 3.10, macOS
- GitHub Check: Test Flutter 3.38, Windows
- GitHub Check: Test Flutter 3.38, Ubuntu
- GitHub Check: Test Flutter beta
- GitHub Check: Test Flutter 3.38, macOS
- GitHub Check: Auto-Release Dry Run
- GitHub Check: Agent
🔇 Additional comments (12)
packages/dart/lib/src/objects/response/parse_error_response.dart (1)
8-30: Good defensive handling for non-JSON responses.The try-catch structure properly handles HTML responses from reverse proxies/load balancers by creating a meaningful error message with a preview. The exception is correctly attached to the ParseError for debugging.
One minor consideration: if
apiResponse.datais empty, the preview will just be an empty string, which is fine but produces a slightly awkward message like"Invalid response format (expected JSON): ". This is acceptable behavior but could be enhanced later.packages/dart/lib/src/network/parse_http_client.dart (1)
50-69: Good retry wrapper integration.The
executeWithRetrywrapper is correctly applied to all HTTP methods with consistent error handling patterns. The structure properly separates the HTTP operation logic from the retry mechanism.packages/dart/test/src/network/parse_client_retry_integration_test.dart (1)
9-34: Excellent documentation of testing architecture.The documentation clearly explains why mock tests don't exercise retry logic and how the testing strategy is split between unit tests (for retry logic) and integration tests (for ParseClient interface behavior). This will help future maintainers understand the coverage implications.
packages/dart/test/src/network/parse_network_retry_test.dart (1)
11-283: Comprehensive retry logic coverage.The test suite thoroughly covers:
- Immediate success scenarios (200, 201)
- Non-retriable Parse errors (101)
- Retry on status -1 and HTML responses
- Exception handling and recovery
- Delay timing verification
- Configuration validation (max 100 retries)
- Both response types (ParseNetworkResponse, ParseNetworkByteResponse)
The save/restore pattern for
restRetryIntervalsproperly isolates tests.packages/dart/lib/src/data/parse_core_data.dart (1)
35-36: Clean integration of retry configuration.The new
restRetryIntervalsparameter follows the existing pattern forliveListRetryIntervals. The default[0, 250, 500, 1000, 2000]provides sensible exponential backoff (5 total attempts including the initial call).The mutable field design allows runtime configuration changes, which is useful for testing but also means the configuration can be altered during app execution. This is consistent with the existing
liveListRetryIntervalsbehavior.Also applies to: 56-57, 95-95
packages/dart/lib/src/network/parse_dio_client.dart (2)
39-58: Consistent retry wrapper integration across all HTTP methods.The
executeWithRetrywrapper is correctly applied to all Dio HTTP operations. The error handling properly distinguishes between responses with and without error data, falling back to_fallbackErrorDatawhen needed.Also applies to: 109-129, 138-158, 212-231
234-235: Clean fallback error data.The
_fallbackErrorDatagetter provides a well-formed JSON string for network errors. This is correctly used as a fallback whenDioExceptiondoesn't have response data.packages/dart/lib/src/network/parse_network_retry.dart (2)
76-137: LGTM: Retry loop logic is sound.The retry loop correctly handles:
- Initial attempt plus configurable retries based on
retryIntervals.length- Proper bounds checking before accessing
retryIntervals[i]- Early return on success or non-retryable errors
- Exception handling with rethrow on final attempt
- Helpful debug logging for troubleshooting
The logic ensures that with
retryIntervalsof length N, the operation is attempted N+1 times total (1 initial + N retries), which aligns with the expected behavior.
25-37: The retry mechanism for POST/PUT operations is documented with appropriate risk mitigations.The documentation correctly identifies that POST/PUT retries carry a duplication risk if the response is lost. The stated mitigations—optimistic locking, idempotent-by-design operations, and network-level-only retry triggers—are standard Parse Server safeguards. The documentation sufficiently informs developers of the tradeoff. No additional verification or changes needed unless there are specific use cases in your application where this retry behavior is inappropriate (in which case, that should be handled at the application integration layer, not in the SDK).
packages/dart/lib/parse_server_sdk.dart (3)
39-39: LGTM: Part directive correctly added.The new part directive for
parse_network_retry.dartis properly placed among other network-related part files.
131-149: Review comment cannot be verified due to unavailable repository access.The repository could not be cloned to verify the following claims in the original review comment:
- That
ParseCoreData.init()accepts therestRetryIntervalsparameter- That the default value
[0, 250, 500, 1000, 2000]is properly set whennullis provided- That the parameter is correctly passed through to
ParseCoreData.init()Manual verification of the ParseCoreData.init() method signature and implementation is required to confirm these details.
104-122: Add documentation for therestRetryIntervalsparameter in the method's doc comment.The
restRetryIntervalsparameter (line 122) is correctly typed and positioned, but it must be documented in theFuture<Parse> initialize()method's doc comment (lines 92-103) to maintain consistency with other parameters.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
packages/dart/lib/src/network/parse_network_retry.dart (2)
139-140: Unreachable code assertion is correct but could be more explicit.The control flow ensures this line is never reached: all paths either return early or rethrow. Consider using an assertion or more explicit comment to document this invariant for maintainability.
- // Should never reach here, but return last response as fallback - return lastResponse!; + // Unreachable: all paths return or rethrow in the loop + assert(false, 'Unreachable code - loop should always return or rethrow'); + return lastResponse!;
157-179: HTML detection logic is redundant within the current control flow.Lines 165-171 check for HTML patterns and return
true, but line 174 also returnstrueunconditionally for all-1status codes. The HTML check never changes the outcome.If the intent is simply to retry all
-1errors (which is reasonable), simplify:bool _shouldRetryResponse(ParseNetworkResponse response) { - // Retry on status code -1 (network/parse errors) - if (response.statusCode == ParseError.otherCause) { - // Additional check: is it HTML instead of JSON? - final String trimmedData = response.data.trimLeft().toLowerCase(); - - // Check for common HTML patterns that indicate proxy/load balancer errors - // More robust than just checking for '<' which could be in valid JSON strings - if (trimmedData.startsWith('<!doctype') || - trimmedData.startsWith('<html') || - trimmedData.startsWith('<head') || - trimmedData.startsWith('<body')) { - // HTML response indicates proxy/load balancer error - return true; - } - - // Other -1 errors (network issues, parse failures) - return true; - } - - // Don't retry successful responses or valid Parse API errors - return false; + // Retry on status code -1 (network/parse errors, including HTML from proxies) + return response.statusCode == ParseError.otherCause; }Alternatively, if HTML detection should influence different behavior (e.g., logging or future logic), keep it but make the distinction meaningful.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/dart/lib/src/network/parse_network_retry.dart(1 hunks)
⏰ 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). (6)
- GitHub Check: Test Dart 3.10, Windows
- GitHub Check: Test Flutter 3.38, Ubuntu, WASM
- GitHub Check: Test Flutter 3.38, Windows
- GitHub Check: Test Flutter beta
- GitHub Check: Test Flutter 3.38, Ubuntu
- GitHub Check: Test Flutter 3.38, macOS
🔇 Additional comments (4)
packages/dart/lib/src/network/parse_network_retry.dart (4)
1-59: Well-documented retry mechanism with clear idempotency warnings.The documentation comprehensively covers retry conditions, edge cases, and importantly warns about non-idempotent operations (POST/PUT). The explanation of why this is acceptable in Parse Server scenarios is helpful.
60-74: Good defensive programming with retry limit enforcement.The 100-retry cap prevents configuration mistakes from causing excessive network requests. The generic constraint
T extends ParseNetworkResponseensures type safety while supporting bothParseNetworkResponseandParseNetworkByteResponse.
76-115: Retry loop implementation is correct and well-structured.The loop correctly handles:
- Initial attempt plus configured retry count
- Early return on success or non-retryable errors
- Proper delay timing using the intervals array
- Debug logging with accurate attempt counts
The exponential backoff pattern with configurable intervals is a good practice for handling transient failures.
161-161: Verifyresponse.datais guaranteed to be a non-null String.Calling
response.data.trimLeft()will throw ifdatais null or not a String. EnsureParseNetworkResponse.datais always a non-nullable String whenstatusCode == -1.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/dart/lib/src/network/parse_dio_client.dart (1)
162-199: Stream data cannot be safely retried - potential data loss.The
postBytesmethod accepts aStream<List<int>>for the request body. Streams can only be consumed once. If a transient failure triggers a retry, the stream will be exhausted, and the retry will send an empty or incomplete body.This is a significant issue for file uploads or other stream-based operations that rely on retry for reliability.
Consider one of these approaches:
- Buffer the stream to a
List<int>before entering the retry loop- Accept a factory function
Stream<List<int>> Function()that creates fresh streams- Document that
postBytesdoes not support retry and exclude it fromexecuteWithRetry@override Future<ParseNetworkResponse> postBytes( String path, { Stream<List<int>>? data, ParseNetworkOptions? options, ProgressCallback? onSendProgress, dynamic cancelToken, }) async { + // Note: Stream-based requests cannot be reliably retried since streams + // can only be consumed once. Consider buffering if retry is needed. return executeWithRetry(
🧹 Nitpick comments (5)
packages/dart/test/src/network/parse_client_retry_integration_test.dart (1)
147-153: Consider addingresponse.successassertion for consistency.Other tests (lines 72, 100, 124, 176, 204) verify
response.success, but this fetch test only checksobjectIdpreservation. Whilefetch()returns the object directly, adding a success check would maintain consistency across tests.final fetchedObject = await object.fetch(); expect(callCount, 1); // No retry via mock expect(fetchedObject.objectId, 'abc123'); // Original objectId preserved + // Note: fetch() returns ParseObject, not ParseResponse - success check not applicable },packages/dart/test/src/network/parse_network_retry_test.dart (3)
67-92: Test isolation pattern is correct but verbose - consider a helper.The save/restore pattern for
restRetryIntervalsis repeated across most tests. While correct for test isolation, consider extracting a test utility or usingsetUp/tearDownwith a stored value to reduce boilerplate.Example helper approach:
T withRetryIntervals<T>(List<int> intervals, T Function() testBody) { final old = ParseCoreData().restRetryIntervals; try { ParseCoreData().restRetryIntervals = intervals; return testBody(); } finally { ParseCoreData().restRetryIntervals = old; } }
198-218: Timing-sensitive test may be flaky in CI environments.The test expects at least 250ms for a 300ms total delay (100+200). While there's a 50ms tolerance, CI environments with resource contention may occasionally fail. Consider increasing the tolerance or using a fake clock for more deterministic testing.
// Should have at least 300ms delay (100 + 200) // Allow some variance for test execution - expect(duration.inMilliseconds, greaterThan(250)); + expect(duration.inMilliseconds, greaterThan(200)); // Allow more variance for CI
285-294: This test provides no behavioral validation.As the comment notes, this test cannot directly test the private function. The assertion only verifies the response object's properties, not the retry behavior. Consider removing this test since the behavior is already validated by other tests in this file, or convert it to a documentation comment.
packages/dart/lib/src/network/parse_network_retry.dart (1)
147-167: Documentation mentions HTML detection but implementation only checks status code.The docstring for
_shouldRetryResponsementions "HTML responses from proxies/load balancers" as a retry trigger, but the implementation only checksstatusCode == ParseError.otherCause.If HTML responses are being converted to status code -1 elsewhere (e.g., in the HTTP client or error parsing), the documentation is misleading about what this specific function does. Consider clarifying that HTML detection happens at the response parsing layer, not in this function.
/// Determines if a network response should be retried. /// /// Returns `true` if the response indicates a transient network error /// that might succeed on retry, `false` otherwise. /// /// Retry Triggers: /// -/// - Status code `-1` (network/parsing errors) -/// - HTML responses from proxies/load balancers (502, 503, 504 errors) -/// - Socket exceptions, timeouts, DNS failures -/// - JSON parse errors from malformed responses +/// - Status code `-1` (network/parsing errors from the HTTP client layer) +/// Note: HTML responses, socket exceptions, timeouts, and parse errors +/// are converted to status -1 by the HTTP client before reaching here.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
packages/dart/lib/src/network/parse_dio_client.dart(6 hunks)packages/dart/lib/src/network/parse_http_client.dart(7 hunks)packages/dart/lib/src/network/parse_network_retry.dart(1 hunks)packages/dart/test/src/network/parse_client_retry_integration_test.dart(1 hunks)packages/dart/test/src/network/parse_network_retry_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/dart/lib/src/network/parse_http_client.dart
⏰ 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). (5)
- GitHub Check: Test Flutter 3.38, Ubuntu, WASM
- GitHub Check: Test Flutter 3.38, Ubuntu
- GitHub Check: Test Flutter 3.38, macOS
- GitHub Check: Test Flutter 3.38, Windows
- GitHub Check: Test Flutter beta
🔇 Additional comments (5)
packages/dart/test/src/network/parse_client_retry_integration_test.dart (1)
1-38: Well-documented integration test setup with clear architectural rationale.The documentation clearly explains why these tests intentionally do not exercise retry logic and how they complement the unit tests. The test architecture properly separates concerns between HTTP-layer retry testing and ParseClient interface behavior.
packages/dart/test/src/network/parse_network_retry_test.dart (1)
1-10: Comprehensive unit test suite for retry mechanism.The test file provides thorough coverage of the
executeWithRetryfunction including success paths, retry scenarios, exception handling, timing, and configuration validation. The 100-retry limit validation and edge cases (empty intervals, byte responses) are well-tested.packages/dart/lib/src/network/parse_dio_client.dart (2)
208-218: Good fix for the JSON escaping issue.The
_buildErrorJsonhelper properly usesjsonEncodeto escape error messages, addressing the past review concern about malformed JSON when error messages contain quotes or special characters.
41-61: Retry wrapper correctly applied to GET method.The
executeWithRetrywrapper properly encapsulates the entire HTTP operation including error handling. TheDioExceptioncatch block is correctly placed inside the operation closure, ensuring retry logic can re-execute the full operation.packages/dart/lib/src/network/parse_network_retry.dart (1)
60-145: Core retry logic is well-implemented with proper bounds checking.The retry loop correctly handles:
- Initial attempt plus configured retries
- Early return on success or non-retryable errors
- Exception handling with retry support
- Delay application between attempts
- Maximum retry limit enforcement
The defensive
StateErrorat line 141 is appropriate for catching potential logic errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/dart/lib/src/network/parse_network_retry.dart (1)
154-174: Consider clarifying the upstream conversion mechanism.The documentation mentions HTML responses from proxies trigger retries, but the implementation only checks for
statusCode == -1. This works correctly becauseParseErrorResponse(upstream) converts HTML error pages to status -1, but this dependency isn't immediately obvious from the documentation.Consider adding a brief note like: "Note: HTML responses are converted to status -1 by ParseErrorResponse before reaching this check."
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/dart/lib/src/network/parse_network_retry.dart(1 hunks)
⏰ 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). (6)
- GitHub Check: Test Dart 3.10, Windows
- GitHub Check: Test Flutter beta
- GitHub Check: Test Flutter 3.38, Windows
- GitHub Check: Test Flutter 3.38, Ubuntu, WASM
- GitHub Check: Test Flutter 3.38, macOS
- GitHub Check: Test Flutter 3.38, Ubuntu
🔇 Additional comments (3)
packages/dart/lib/src/network/parse_network_retry.dart (3)
25-44: Idempotency warnings are well-documented.The documentation properly warns about retry risks for non-idempotent POST/PUT operations and provides clear mitigation strategies. This addresses the previous review concerns about misleading idempotency claims.
67-102: Retry loop logic is correct.The loop correctly handles:
- Initial attempt plus configurable retries based on
retryIntervals.length- Empty intervals list results in single attempt with no retries
- Early exit on success or non-retryable responses
124-151: Good defensive programming with the StateError.The exception handling correctly mirrors the success path logic, and the
StateErrorat the end serves as a safety net to catch any future logic errors, even though it should be unreachable with the current implementation.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 8 changed files in this pull request and generated 1 comment.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/dart/lib/parse_server_sdk.dart (1)
122-123: Consider adding parameter documentation.The parameters are properly added and forwarded, maintaining backward compatibility. However, the doc comment for the
initializemethod (lines 92-103) doesn't mention the new retry configuration parameters. Consider adding brief documentation explaining these parameters and their defaults.Example addition to the doc comment:
/// * [restRetryIntervals] - Optional list of retry delay intervals (in milliseconds) /// for read operations. Defaults to [0, 250, 500, 1000, 2000]. /// * [restRetryIntervalsForWrites] - Optional list of retry delay intervals for /// write operations. Defaults to [] (no retries) to prevent duplicate data.Also applies to: 150-151
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
packages/dart/lib/parse_server_sdk.dart(3 hunks)packages/dart/lib/src/data/parse_core_data.dart(3 hunks)packages/dart/lib/src/network/parse_dio_client.dart(6 hunks)packages/dart/lib/src/network/parse_http_client.dart(7 hunks)packages/dart/lib/src/network/parse_network_retry.dart(1 hunks)packages/dart/test/src/network/parse_client_retry_integration_test.dart(1 hunks)packages/dart/test/src/network/parse_network_retry_test.dart(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- packages/dart/lib/src/network/parse_http_client.dart
⏰ 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). (6)
- GitHub Check: Test Flutter beta
- GitHub Check: Test Dart 3.10, Windows
- GitHub Check: Test Flutter 3.38, Ubuntu
- GitHub Check: Test Flutter 3.38, Windows
- GitHub Check: Test Flutter 3.38, Ubuntu, WASM
- GitHub Check: Test Flutter 3.38, macOS
🔇 Additional comments (21)
packages/dart/lib/src/data/parse_core_data.dart (3)
35-36: LGTM! Clean parameter additions.The optional nullable parameters preserve backward compatibility while enabling configuration of retry behavior for both read and write operations.
57-60: Appropriate default retry configuration.The defaults align well with the PR objectives:
- Read operations (GET): 5 attempts with exponential backoff to handle transient network issues
- Write operations (POST/PUT): No retries by default to prevent duplicate data creation
The exponential backoff intervals (0, 250, 500, 1000, 2000 ms) provide a good balance between quick recovery and avoiding server overload.
98-99: LGTM! Field declarations are appropriate.The
latemodifier is correct since these fields are initialized in theinitmethod. Non-final allows runtime reconfiguration if needed (as seen in test files).packages/dart/test/src/network/parse_client_retry_integration_test.dart (3)
9-34: Excellent architectural documentation!The documentation clearly explains why these integration tests bypass the retry mechanism and how they complement the unit tests. This will help future developers understand the testing strategy and avoid confusion about coverage metrics.
47-75: Well-structured integration test.The test correctly verifies that HTML error responses are processed without retry when using a mocked client. The expectation of
callCount == 1accurately demonstrates the architectural decision that retry logic operates at the HTTP layer, not at the ParseClient interface.
181-208: Correctly validates non-retry behavior for Parse errors.The test properly demonstrates that valid Parse Server error codes (like 101 ObjectNotFound) are not retried, which is the correct behavior since these are application-level errors that won't resolve with retries.
packages/dart/test/src/network/parse_network_retry_test.dart (4)
12-65: Comprehensive coverage of success paths.These tests properly validate that successful responses (200, 201) and valid Parse Server errors (101) don't trigger unnecessary retries, confirming efficient operation.
198-218: Timing test with appropriate tolerance.The test validates retry delay intervals but uses a relaxed threshold (>200ms for expected 300ms) to account for CI environment variability. The comment acknowledges this consideration, which is good practice for reliable CI execution.
220-234: Important safety validation test.The max retry limit of 100 prevents accidental infinite retry loops. This test ensures the guard rail is properly enforced.
514-560: Critical write operation behavior properly tested.These tests validate the important distinction between read and write retry behavior. The default of no retries for writes (line 531:
callCount == 1) prevents accidental duplicate data creation, while still allowing custom configuration when needed.packages/dart/lib/parse_server_sdk.dart (1)
39-39: LGTM! Proper part file inclusion.The new part file is correctly placed with other network-related parts and follows the existing structure.
packages/dart/lib/src/network/parse_network_retry.dart (4)
3-81: Excellent comprehensive documentation.The documentation thoroughly addresses retry behavior, idempotency concerns, and provides clear guidance on safe usage. The warning about POST/PUT non-idempotency and default no-retry behavior for writes demonstrates careful consideration of data integrity.
92-100: Good safety guard with clear error message.The max retry limit of 100 prevents accidental configuration errors that could cause excessive retry attempts. The error message clearly explains the constraint and includes the actual length for debugging.
106-163: Retry loop logic is correct.The loop properly handles:
- Initial attempt + configured retries (loop from 0 to length, inclusive)
- Early return on success (lines 113-120)
- Final attempt return when retries exhausted (lines 124-130)
- Exception handling with retry (lines 142-162)
- Appropriate delays between attempts (line 141)
The debug logging provides useful insight into retry progress.
172-191: Simple and correct retry logic.The
_shouldRetryResponsefunction correctly identifies retriable responses by checking for status code -1 (ParseError.otherCause). The documentation mentions HTML responses, which makes sense because HTML parsing failures are converted to status -1 by the HTTP client layer before reaching this function.packages/dart/lib/src/network/parse_dio_client.dart (6)
8-20: Helpful coverage documentation.The documentation explaining the low coverage percentage and the architectural rationale is very useful. It prevents confusion about coverage metrics and clarifies that retry logic is tested elsewhere at 100%.
36-61: Clean retry integration for GET requests.The implementation correctly wraps the GET operation with retry logic. GET operations are idempotent and safe to retry, so the default retry behavior (no
isWriteOperationflag) is appropriate.
229-248: DELETE retry behavior is appropriate.DELETE operations are not flagged with
isWriteOperation: true, meaning they use the default read retry intervals. This is correct because:
- DELETE operations are typically idempotent (deleting an already-deleted object usually succeeds or returns 404)
- Transient network failures during DELETE are safe to retry
- This aligns with the PR objectives (line 35 in parse_network_retry.dart: "DELETE operations: Retries enabled")
211-222: Well-implemented error JSON helper.The
_buildErrorJsonhelper properly addresses the JSON escaping concern from past reviews by usingjsonEncodeon a Map structure. This ensures error messages containing quotes or special characters won't produce malformed JSON.
165-202: Proper write operation handling with consistent error payloads.The
postBytesmethod correctly:
- Marks the operation as a write (
isWriteOperation: true)- Handles both DioException cases (with and without response)
- Uses
_buildErrorJsonfor consistent error formatting when response is unavailable
251-251: Consistent error payload for fallback case.The
_fallbackErrorDatagetter now returns a properly structured JSON error payload using_buildErrorJson, ensuring consistency across all error cases.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
packages/dart/lib/parse_server_sdk.dart (1)
104-113: Clarify how retry configs apply across all HTTP methodsThe parameter docs clearly separate read vs write retries and explain the defaults, which is good. Since the HTTP clients also expose
headandpatchmethods (per the PR/AI summary), consider briefly noting which bucket those fall into (read-like vs write-like) so callers aren’t left guessing.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/dart/lib/parse_server_sdk.dart(4 hunks)
⏰ 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). (5)
- GitHub Check: Test Flutter 3.38, Ubuntu, WASM
- GitHub Check: Test Flutter 3.38, Windows
- GitHub Check: Test Flutter beta
- GitHub Check: Test Flutter 3.38, macOS
- GitHub Check: Test Flutter 3.38, Ubuntu
🔇 Additional comments (3)
packages/dart/lib/parse_server_sdk.dart (3)
39-39: Wiring inparse_network_retry.dartas a library part looks correctThe new
partplacement is consistent with the existing network parts and cleanly exposes the shared retry implementation to the public SDK surface.
132-133: New retry parameters onParse.initializeare backward compatibleAdding
restRetryIntervalsandrestRetryIntervalsForWritesas nullable named parameters preserves existing call sites while cleanly surfacing retry configuration through the main initialization API. Naming and types align with their intended usage.
160-161: Retry configuration is correctly threaded intoParseCoreData.initPassing both retry lists through to
ParseCoreData.initkeeps all behavior centralized in the core data layer. Please just double‑check thatParseCoreData.initapplies the documented defaults ([0, 250, 500, 1000, 2000]for reads and[]for writes) when these arenull, so the docs and behavior stay in sync.
1 participant
Pull Request
Issue
Closes: #1043
Approach
Implemented automatic retry mechanism for network operations at the HTTP client layer to handle transient network failures and HTML error responses from reverse proxies/load balancers.
Core Implementation:
parse_network_retry.dartwithexecuteWithRetry()function that wraps HTTP operationsParseHTTPClientandParseDioClientby wrapping all 6 HTTP methods (get, post, put, delete, head, patch)ParseErrorResponseto detect and handle HTML responsesRetry Conditions:
-1(network/connection errors)Configuration:
restRetryIntervalsparameter toParse().initialize()[0, 250, 500, 1000, 2000](5 retry attempts with exponential backoff)Testing:
Documentation:
Tasks
Summary by CodeRabbit
New Features
Bug Fixes
Tests
Documentation
✏️ Tip: You can customize this high-level summary in your review settings.