GetDutchie / brick

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

Redundant REST API calls for fetching an association (Supabase Integration) #433

Closed devj3ns closed 1 month ago

devj3ns commented 1 month ago

Context: https://github.com/GetDutchie/brick/issues/399, https://github.com/GetDutchie/brick/issues/429#issuecomment-2327967709

In my project using the new Supabase integration, I have a model Project and a model Customer:

@ConnectOfflineFirstWithSupabase( supabaseConfig: SupabaseSerializable(tableName: 'projects'))
class Project extends OfflineFirstWithSupabaseModel {

  ...

  @Supabase(name: 'customer', toGenerator: '%INSTANCE_PROPERTY%.id.value')
  @Sqlite(index: true)
  final Customer customer;
}
@ConnectOfflineFirstWithSupabase( supabaseConfig: SupabaseSerializable(tableName: 'customers'))
class Customer extends OfflineFirstWithSupabaseModel {

  ...

  @Sqlite(unique: true, index: true)
  @Supabase(unique: true)
  final UUID id;
}

Currently, when I fetch a list of projects with await repository.getBatched<Project>() Brick fetches all the projects with their associated customers in one REST Request:

https://[project_id]/rest/v1/projects?select=id,customer:customers!customer(id,first_name)&limit=50

But unfortunately, for each project, a separate REST Request is sent to fetch the customer:

https://[project_id]/rest/v1/customers?select=id,first_name&id=eq.xyz&limit=1

This means, when I batch fetch 50 projects, 50 REST requests to the customers' endpoint are sent. But because we already joined the customers in the REST Request in which the projects were fetched, this is redundant.

The generated Project adapter code looks like follows:

(Note that since I removed the @OfflineFirst(where:) annotation, see https://github.com/GetDutchie/brick/issues/429#issuecomment-2327974648, this is not the part where the redundant REST call is triggered).

Future<Project> _$ProjectFromSupabase(Map<String, dynamic> data,
    {required SupabaseProvider provider,
    OfflineFirstWithSupabaseRepository? repository}) async {
    return Project(
        ....
         customer: await CustomerAdapter().fromSupabase(data['customer'],
          provider: provider, repository: repository),
        ....
      ),
}
Future<Project> _$ProjectFromSqlite(Map<String, dynamic> data,
    {required SqliteProvider provider,
    OfflineFirstWithSupabaseRepository? repository}) async {
    return Project(
        ...
        customer: (await repository!.getAssociation<Customer>(
            Query.where(
                'primaryKey', data['customer_Customer_brick_id'] as int,
                limit1: true),
          ))!.first,
         ...
     ),
}

The repository!.getAssociation triggers the redundant REST API requests. In https://github.com/GetDutchie/brick/issues/429#issuecomment-2330178471 I wrote that I think this part should be generated differently (only fetch the customer locally) to prevent the REST Call. But I am not sure if this would not lead to other issues.

Because of https://github.com/GetDutchie/brick/issues/429#issuecomment-2330478848, I tried using seedOnly. This helps to reduce the number of requests to fetch a customer sent to the REST API. But there are still happening many unnecessary requests.

I created two videos in which you can see all the requests made. Context: I am using the alwaysHydrate policy.

seedOnly: false: https://www.loom.com/share/18709cae44c24046a4a6abf7c4a84ac9 seedOnly: true: https://www.loom.com/share/39b6ac74e95d4035b65664ac1569378f

I hope this helps. Thanks in advance 🙏

tshedor commented 1 month ago

Context: I am using the alwaysHydrate policy.

Ah. This is critical context......Brick is doing what you're asking it to do. It's going to always hydrate all the way down through associations, per #371 and the fix in #372.

I've created a test case in #434 to ensure that Brick does store associations correctly. So your problem is due to this policy. I believe the best case scenario fix would be to override the getAssociation method:

class MyRepository extends OfflineFirstWithSupabaseRepository {
  @override
  Future<List<TModel>?> getAssociation<TModel extends OfflineFirstWithSupabaseModel>(Query query) async {
    logger.finest('#getAssociation: $TModel $query');
    final results = await get<TModel>(
      query: query,
      // This removes the semi-stateful _latestGetPolicy
      policy: OfflineFirstGetPolicy.awaitRemoteWhenNoneExist,
    );
    if (results.isEmpty) return null;
    return results;
  }
}
devj3ns commented 1 month ago

Oh, this makes sense!

The fix is working great, thanks @tshedor!