Skip to content
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.

Sign up for GitHub

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

Jump to bottom

feat(offline_first_with_rest): add request/response callbacks to the RestOfflineQueueClient #447

Open
wants to merge 8 commits into
base: main
Choose a base branch
from
Open

feat(offline_first_with_rest): add request/response callbacks to the RestOfflineQueueClient #447

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7b7bc52
feat(offline_first_with_rest): add request/response callbacks to the …
devj3ns Sep 26, 2024
be725a7
refactor(OfflineFirstWithRestRepository): make callbacks abstract met…
devj3ns Oct 12, 2024
c22504d
refactor: guard logging onReattemptableRespons
devj3ns Oct 12, 2024
d659ee6
docs: extend doc comment for onRequestError callback
devj3ns Oct 12, 2024
bd29927
run dart format
devj3ns Oct 14, 2024
f78bb9d
onReattemptableResponse: return response status code instead of Strea…
devj3ns Oct 14, 2024
69b2692
fix doc comments
devj3ns Oct 15, 2024
327caad
add callbacks to GraphqlOfflineQueueLink
devj3ns Oct 15, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions packages/brick_offline_first_with_rest/lib/src/offline_first_with_rest_repository.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,6 +47,13 @@ abstract class OfflineFirstWithRestRepository
/// This property is forwarded to `RestOfflineQueueClient` and assumes
/// its defaults
List<int>? reattemptForStatusCodes,

/// This property is forwarded to `RestOfflineRequestQueue`.
void Function(http.Request request, http.StreamedResponse response)?
onReattemptableResponse,

/// This property is forwarded to `RestOfflineRequestQueue`.
void Function(http.Request, Object)? onRequestError,
devj3ns marked this conversation as resolved.
Show resolved Hide resolved
required RestProvider restProvider,
required super.sqliteProvider,
}) : remoteProvider = restProvider,
Expand All @@ -57,6 +64,8 @@ abstract class OfflineFirstWithRestRepository
restProvider.client,
offlineQueueManager,
reattemptForStatusCodes: reattemptForStatusCodes,
onRequestError: onRequestError,
onReattemptableResponse: onReattemptableResponse,
);
offlineRequestQueue = RestOfflineRequestQueue(
client: remoteProvider.client as RestOfflineQueueClient,
Expand Down
29 changes: 24 additions & 5 deletions packages/brick_offline_first_with_rest/lib/src/offline_queue/rest_offline_queue_client.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,14 @@ class RestOfflineQueueClient extends http.BaseClient {

final RequestSqliteCacheManager<http.Request> requestManager;

/// A callback triggered when the response of a request has a status code
/// which is present in the [reattemptForStatusCodes] list.
void Function(http.Request request, http.StreamedResponse response)?
onReattemptableResponse;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer to keep the names similar

Suggested change
/// A callback triggered when the response of a request has a status code
/// which is present in the [reattemptForStatusCodes] list.
void Function(http.Request request, http.StreamedResponse response)?
onReattemptableResponse;
/// A callback triggered when the response of a request has a status code
/// which is present in the [reattemptForStatusCodes] list.
void Function(http.Request request, http.StreamedResponse response)?
onReattemptResponse;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is the onReattemptResponse necessary? Your PR description notes that it can help resolve a blocked queue, but is it clear how many retries have been made on this request? Is that necessary to determine when to skip a request? If you've explicitly told Brick "reattempt in this scenario" and now you want to no longer attempt for that scenario, shouldn't you remove the status code from the reattempt list instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The onReattemptResponse helps to

  1. know that the client is online (because there was a response from the server)
  2. check if the status code of the returned response code indicates that the server rejected the response (e.g., HTTP 409) (in which case I prompt the user to handle the conflict)

If you've explicitly told Brick "reattempt in this scenario" and now you want to no longer attempt for that scenario, shouldn't you remove the status code from the reattempt list instead?

If I removed the status code, (e.g., HTTP 409) from the reattempt from status codes list, the client had no chance to detect and handle the rejected response and the data is lost.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would prefer to keep the names similar

I think naming the callback onReattemptResponse is misleading, as it sounds like a response would be reattempted. But my initial suggestion onReattemptableResponse has the same problem.

What about onReattemptableRequestFailed?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about onRequestFailure?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So you would go with onRequestFailure and onRequestError? Then it's difficult to guess from the name which one receives which callback.

What about
onReattemptableRequestFailure and onRequestException or
onResponse and onRequestException?


/// A callback triggered when a request throws an exception during execution.
devj3ns marked this conversation as resolved.
Show resolved Hide resolved
void Function(http.Request request, Object error)? onRequestError;

/// If the response returned from the client is one of these error codes, the request
/// **will not** be removed from the queue. For example, if the result of a request produces a
/// 404 status code response (such as in a Tunnel not found exception), the request will
Expand All @@ -37,6 +45,8 @@ class RestOfflineQueueClient extends http.BaseClient {
RestOfflineQueueClient(
this._inner,
this.requestManager, {
this.onReattemptableResponse,
this.onRequestError,
List<int>? reattemptForStatusCodes,

/// Any request URI that begins with one of these paths will not be
Expand Down Expand Up @@ -89,17 +99,26 @@ class RestOfflineQueueClient extends http.BaseClient {
// Attempt to make HTTP Request
final resp = await _inner.send(request);

if (cacheItem.requestIsPush && !reattemptForStatusCodes.contains(resp.statusCode)) {
final db = await requestManager.getDb();
// request was successfully sent and can be removed
_logger.finest('removing from queue: ${cacheItem.toSqlite()}');
await cacheItem.delete(db);
if (cacheItem.requestIsPush) {
if (!reattemptForStatusCodes.contains(resp.statusCode)) {
final db = await requestManager.getDb();
// request was successfully sent and can be removed
_logger.finest('removing from queue: ${cacheItem.toSqlite()}');
await cacheItem.delete(db);
} else {
_logger.finest(
'request failed, will be reattempted: ${cacheItem.toSqlite()}');
onReattemptableResponse?.call(request, resp);
}
devj3ns marked this conversation as resolved.
Show resolved Hide resolved
}

return resp;
} catch (e) {
// e.g. SocketExceptions will be caught here
onRequestError?.call(request, e);
_logger.warning('#send: $e');
} finally {
// unlock the request which results in a reattempt
devj3ns marked this conversation as resolved.
Show resolved Hide resolved
final db = await requestManager.getDb();
await cacheItem.unlock(db);
}
Expand Down
5 changes: 5 additions & 0 deletions ...ges/brick_offline_first_with_supabase/lib/src/offline_first_with_supabase_repository.dart
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -189,6 +189,9 @@ abstract class OfflineFirstWithSupabaseRepository
504,
],
bool? serialProcessing,
void Function(http.Request request, http.StreamedResponse response)?
onReattemptableResponse,
void Function(http.Request, Object)? onRequestError,
}) {
final client = RestOfflineQueueClient(
innerClient ?? http.Client(),
Expand All @@ -200,6 +203,8 @@ abstract class OfflineFirstWithSupabaseRepository
),
ignorePaths: ignorePaths,
reattemptForStatusCodes: reattemptForStatusCodes,
onReattemptableResponse: onReattemptableResponse,
onRequestError: onRequestError,
);
return (client, RestOfflineRequestQueue(client: client));
}
Expand Down
Loading