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

[devj3ns]

This PR introduces two optional callbacks to the RestOfflineQueueClient, which closes #393.

1. onReattemptableResponse: This callback is invoked when a request returns a response with a status code listed in the reattemptForStatusCodes. It allows for detecting cases where a request is repeatedly rejected by the remote provider (e.g., when receiving an HTTP 409 (Conflict) response). When serial processing is enabled, this helps detect a blocked queue, allowing the app implementation to handle it appropriately.
2. onRequestError: This callback is triggered when a request throws an exception during execution (e.g., a SocketException). It helps identify the current state of the offline queue synchronization process.

In my application, these two callbacks are used to monitor the status of the offline queue, determining whether the queue is actively processing, blocked by a rejected request, or paused due to the client being offline. This allows me to provide users with real-time feedback on the system's sync status, which is important for my application.

@tshedor, what do you think of this implementation?

[jit-ci]

✅ Great news! Jit hasn't found any security issues in your PR. Good Job! 🏆

[tshedor]

@devj3ns Thanks for opening this PR. A few high level notes -

1. I'm still concerned about http.StreamedResponse. From the discussion on Proposal: add the response details to the offline request queue #393:

Yes, I'm aware of that. In my case, the HTTP status code provides enough information, so this limitation shouldn't be an issue.

This may be the way your app uses this function, however, you'd be amazed at how people use Brick in ways you did not expect them to. If you expose functionality, people expect to use it. So either 1) http.StreamedResponse needs to be passed as a parsed object or 2) only the status code can be passed to the callbacks or 3) a unit test needs to prove that the response can be parsed twice.

2. This feature needs unit testing. If you're just throwing up this PR to get architecture discussion, we can come back to unit testing when the arch is finalized.

3. This requires an equivalent in the GraphQL domain. From the Proposal: add the response details to the offline request queue #393 discussion:

I tried this, but ran into an issue I currently do not know how to resolve properly: the RestOfflineRequestQueueClient relies on the http.StreamedResponse type for a response, while GraphqlOfflineQueueLink uses the Response type.

OfflineRequestQueue has a generic type of TRequest that should provides the placeholder for the Request/http.Request. A second generic type of TResponse can be provided for Response/http.StreamedResponse on `OfflineRequestQueue.

4. There needs to be documentation in the README, the CHANGELOG, and the docs/ Markdown. Again, can wait until we're settled on architecture.

---

Overall, this approach is simple. Not using a Stream feels like an anti-pattern...but then you'd have to manage a StreamController and handle the dispose, which would require state, and Brick shouldn't be stateful. But there's a lingering feeling that the callback-stored-as-instance-field doesn't feel quite right... @mateominato do you have any ideas here?

[tshedor]

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;

[tshedor]

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?

[devj3ns]

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.

[devj3ns]

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?

[tshedor]

What about onRequestFailure?

[devj3ns]

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?

[devj3ns]

Thanks for the feedback @tshedor, I'm currently on vacation and will respond to your feedback and update the PR next week.

[devj3ns]

Sorry for the late response @tshedor! I had a very busy last week...

I have already addressed some of your feedback and will address the rest this evening.

This feature needs unit testing

Yes! I will add tests once we have agreed on an implementation.

There needs to be documentation

Same as with the tests.

[devj3ns]

@tshedor, changed the implementation to only include the response status code instead of the http.StreamedResponse. Moreover, I added the callbacks to the GraphqlOfflineQueueLink.

The RestOfflineRequestQueueClient was never really meant to be exposed, and in the repositories where it's implemented, it isn't available on the Repository level. However, RestOfflineRequestQueue is exposed on the repository, so I'd recommend creating an abstract method on OfflineRequestQueue, and implement it in RestOfflineRequestQueue and forward it to RestOfflineRequestQueueClient.

OfflineRequestQueue has a generic type of TRequest that should provides the placeholder for the Request/http.Request. A second generic type of TResponse can be provided for Response/http.StreamedResponse on `OfflineRequestQueue.

I'm in favor of creating the generic methods in the OfflineRequestQueue and implementing them in the RestOfflineRequestQueue and GraphqlOfflineRequestQueue, but how to pass them to the RestOfflineQueueClient and GraphqlOfflineQueueLink?

[tshedor]

@devj3ns I've taken a pass through the code. You're right that adding an abstract to OfflineRequestQueue is not feasible - the flow from queue -> client -> queue doesn't make much sense. RequestManager is the next most likely candidate since it's known to Link and Client, but that's an abuse of its core purpose (convert to and from SQLite). So let's move forward with this design.

After tests and documentation, I think this is good to go.

[devj3ns]

@tshedor I’ve added a new section to the documentation that covers this feature.

For the unit tests, I included some to cover the RestOfflineQueueClient callbacks. However, I wasn’t able to make a failing request with GraphqlOfflineQueueLink since I don’t have experience with this part of the package. Could you help out here?

[devj3ns]

Regarding the naming, I would propose the following, more precise names:

• onReattemptableRequestFailure (instead of onReattemptableResponse)
• onRequestException (instead of onRequestError)

[tshedor]

@devj3ns Added some GraphQL tests. It's been a minute since I've been in GraphQL, so I also changed the method signature slightly (GraphQL is sort of REST in a costume, but it doesn't natively support status codes as it turns out).

onRequestException

Sounds great

onReattemptableRequestFailure

What about just onReattempt? The Request part feels redundant since it's already in a RequestQueue constructor, and the Failure part could be confusing given the onRequestException argument. It might just be better to stay simple?

Lastly, do you want to add a quick CHANGELOG note? I added a header and the version numbers in 3330270

[devj3ns]

@tshedor okay, good to know. I have added the changelog and renamed both functions.

I'm glad that this is now complete! :)

[tshedor]

@devj3ns made one minor change to the REST repository to mirror the Supabase repository (not storing the callback as a final) in 69d6c25