search

GetDutchie / brick

An intuitive way to work with persistent data in Dart
https://getdutchie.github.io/brick/#/
298 stars 27 forks source link

Supabase Integration (WIP) #401

Open tshedor opened 4 weeks ago

tshedor commented 4 weeks ago

Please use this issue to follow progress on the Supabase integration. This issue will be opened for comments when the branch is considered stable enough for early testing, so consider subscribing to Notifications. Related: #359

brick_supabase

brick_offline_first_with_supabase

brick_offline_first_with_supabase_build

brick_supabase_generators

~brick_supabase_abstract~

No longer needed; using the supabase_dart library

~brick_offline_first_with_supabase_abstract~

No longer needed; using the supabase_dart library

devj3ns commented 3 weeks ago

I just migrated my code from using the RestProvider for Supabase implementation to the new first party Supabase Integration. While doing this, I encountered some issues, as described below:

  1. I could not use brick_offline_first_from_supabase v0.1.0 from pub.dev, as it does not include the latest changes (e.g., the additions inside the OfflineFirstWithSupabaseRepository class). I used the version from GitHub instead
  2. I found a bug in generating the adapters: https://github.com/GetDutchie/brick/issues/416
  3. I was unable to use the OfflineFirstWithSupabaseRepository.clientQueue method as seen in the example because it does not allow to pass the reattemptForStatusCodes, databaseName, processingInterval and serialProcessing attributes. More info below (*)

(*): I had to use the following code when initializing the OfflineFirstWithSupabaseRepository offlineRequestQueue. This requires importing the brick_offline_first_with_rest package. While this is acceptable, I would prefer not to import the REST package when using the Supabase integration.

offlineRequestQueue: RestOfflineRequestQueue(
    client: RestOfflineQueueClient(
        http.Client(),
        reattemptForStatusCodes: [400, 401, 403, 404 405,408,409,429,500501,502,503,504],
        RestRequestSqliteCacheManager(
              'offline_queue.sqlite',
              databaseFactory: kIsWeb ? databaseFactoryFfiWeb : databaseFactory,
              processingInterval: const Duration(seconds: 5),
              serialProcessing: true,
        ),
  ),
),

When the adapter issue is resolved, I can run the app and provide additional feedback.

tshedor commented 2 weeks ago

@devj3ns Regarding "While this is acceptable, I would prefer not to import the REST package when using the Supabase integration," I largely agree. Do you think it'd be better to move this logic to the brick_offline_first package? It is somewhat REST specific and the brick_offline_first_with_rest doesn't have any other heavy dependencies.

I wrestled with this and felt recycling the code was a suitable tradeoff for the amount of testing/stability behind the REST queue, but I'm less attached to where the code properly lives (as long as the decision can be defended).

tshedor commented 2 weeks ago

I was unable to use the OfflineFirstWithSupabaseRepository.clientQueue method as seen in the example because it does not allow to pass the reattemptForStatusCodes, databaseName, processingInterval and serialProcessing attributes. More info below (*)

Are all of these status codes necessary from the Supabase API? If they are, they should be a default value in the argument.

Latest versions have been published off the #420 (yo @mateominato tell the Dutchie team) branch:

brick_offline_first_with_supabase: 0.1.0+1
brick_supabase: 0.1.1
brick_supabase_generators: 0.1.1

For easier testing, it's best to clone this repo locally, git checkout supabase-integration, then reference any brick packages via path in dependency_overrides in your app's pubspec. Once you clone it, run dart pub get; dart run melos bootsrap in the root so that all related packages install from path instead of remote.

devj3ns commented 2 weeks ago

@devj3ns Regarding "While this is acceptable, I would prefer not to import the REST package when using the Supabase integration," I largely agree. Do you think it'd be better to move this logic to the brick_offline_first package? It is somewhat REST specific and the brick_offline_first_with_rest doesn't have any other heavy dependencies.

I wrestled with this and felt recycling the code was a suitable tradeoff for the amount of testing/stability behind the REST queue, but I'm less attached to where the code properly lives (as long as the decision can be defended).

I think reusing the rest_offline_queue code for the Supabase package is totally fine for now. And I would personally not move it to the brick_offline_first package because it is REST related.

When using the OfflineFirstWithSupabaseRepository.clientQueue method, one does not have to add the rest package as a direct dependency, so in my opinion it is okay as it is.

devj3ns commented 2 weeks ago

Are all of these status codes necessary from the Supabase API? If they are, they should be a default value in the argument.

@tshedor For my app, they are necessary because I want the app user to know when some data could not be upserted when processing the offline queue (no matter why). I think it depends on the app and conflict resolution strategy.

I would personally definitely add HTTP 401 (Unauthorized) to the default reattempt status codes for the Supabase queue, as this is the one returned when the JWT expired and that's a case where a reattempt is definitely important.

When being precise, HTTP 409 (Conflict) would rather be a "rejectedStatusCode" which had to be handled by the app user instead of reattempting it. But that is another topic which is somewhat related to https://github.com/GetDutchie/brick/issues/393, but unfortunately, I currently do not have much time to think about improving the offline request queue.

tshedor commented 1 week ago

@devj3ns I think we're finally close to a stable release. I've prepared a PR (#436) that updates all new packages to the 1.0.0 version.

I'd like this to go through the routine with your app and testing for another week. If there are no new bugs, let's call it stable on Friday, Sept 13.

devj3ns commented 1 week ago

@tshedor That timeline aligns perfectly with my plan. Next week, I'll be running additional tests to ensure that all the app's functionality works seamlessly with the Supabase integration. Once confirmed, I’ll create a new release of my app for production use.

I'll report back next week, but my feeling is that everything is looking good so far.

Before making a stable release, however, I would definitely suggest adjusting the default reattempt status codes. As mentioned in my previous comment. At a minimum HTTP 401 (Unauthorized) should be added. This status occurs when the client's JWT has expired, which can happen e. g. on the first request after the client comes back online.

tshedor commented 1 week ago

@devj3ns Done, merged, and published in #437

devj3ns commented 1 week ago

Nice 👍

I think we should add a note to the docs, that when this list includes status codes like 409 (Conflict) or 403 (Forbidden), which could mean that the request is permanently rejected by the Database and no reattempt will succeed, it can block other requests in the queue from sending. What do you think @tshedor?

Currently, the biggest thing missing from brick (in my opinion) is handling what happens when a request is permanently rejected by the Remote Provider and all reattempts fail (e.g. HTTP 409 or 403). In my app, which uses serial processing (because of association dependencies) I added a check which detects a blocked request queue and gives the user the ability to discard the rejected request to unblock the queue.

When I am done with the file upload thing (on which I will write something to the other issue next week), I will come back to the "more stateful" request queue topic (related: https://github.com/GetDutchie/brick/issues/393).

tshedor commented 1 week ago

I think we should add a note to the docs, that when this list includes status codes like 409 (Conflict) or 403 (Forbidden), which could mean that the request is permanently rejected by the Database and no reattempt will succeed, it can block other requests in the queue from sending.

This feels a little "water is wet" but if you think it would be useful to include then we should include it. Mind opening a PR?

Currently, the biggest thing missing from brick (in my opinion) is handling what happens when a request is permanently rejected by the Remote Provider and all reattempts fail

Yeah, I know this has been bothering you for a while. Do you want to lead the development of the feature since you're closest to it? I'd be more comfortable with this implementation in Supabase because the remote provider is known versus a GraphQL or Rest provider where, in theory, most applications would be using their own, manageable API and would therefore control the status codes.

tshedor commented 3 days ago

@devj3ns How'd this week go? Any further bugs?

devj3ns commented 3 days ago

Hey @tshedor, unfortunately, I was very busy with other tasks this week, but I already blocked some hours tomorrow morning to further test the supabase integration.

I will report back until tomorrow 10 UTC.

devj3ns commented 3 days ago

This feels a little "water is wet" but if you think it would be useful to include then we should include it. Mind opening a PR?

Yes, I’ll go ahead and open a small PR on Monday.

Do you want to lead the development of the feature since you're closest to it?

I'd be happy to do that!

I'd be more comfortable with this implementation in Supabase because the remote provider is known versus a GraphQL or Rest provider where, in theory, most applications would be using their own, manageable API and would therefore control the status codes.

I completely agree! I’ll comment on https://github.com/GetDutchie/brick/issues/393 once I’m ready to start on this, which should be around mid-October.

devj3ns commented 3 days ago

If there are no new bugs, let's call it stable on Friday, Sept 13.

@tshedor Unfortunately, there is an issue with the Supabase integration when upserting while being offline: https://github.com/GetDutchie/brick/issues/439

devj3ns commented 1 hour ago

@tshedor, I found two more issues with the Supabase integration: https://github.com/GetDutchie/brick/issues/440 https://github.com/GetDutchie/brick/issues/441