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.

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

Issues with Querying Associated Documents and REST Parameter Mapping in Supabase Integration #429

Closed
devj3ns opened this issue Aug 29, 2024 · 21 comments

Comments

@devj3ns
Copy link
Contributor

devj3ns commented Aug 29, 2024

In my project, I have a model Project and a model Customer:

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

  ...
  
  @Supabase(foreignKey: 'customer')
  @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;
}

On one page of my I app, I want to query all projects that are associated with a user.

Before migrating to the Supabase integration, using the following brick query worked fine:

query: Query(
    where: [
      const Where('customer').isExactly(
        const Where('id').isExactly(customer.id.value),
      ),
    ],
    providerArgs: {'orderBy': 'project.createdAt DESC'},
  ),

With the new Supabase integration, their are two problems:

1. Problem: DatabaseException

When querying, the following exception is thrown:

DatabaseException(ambiguous column name: created_at (code 1 SQLITE_ERROR): , while compiling: SELECT `Project`._brick_id FROM `Project` INNER JOIN `Customer` ON `Project`.customer_Customer_brick_id = `Customer`._brick_id WHERE `Customer`.id = ? ORDER BY created_at DESC LIMIT 1000 OFFSET 0) sql 'SELECT `Project`._brick_id FROM `Project` INNER JOIN `Customer` ON `Project`.customer_Customer_brick_id = `Customer`._brick_id WHERE `Customer`.id = ? ORDER BY created_at DESC LIMIT 1000 OFFSET 0' args [de8a7f34-1b22-4fd4-a26d-eb17f7e8c6f2]
E/flutter ( 8337): [ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: DatabaseException(ambiguous column name: created_at (code 1 SQLITE_ERROR): , while compiling: SELECT COUNT(*) FROM `Project` INNER JOIN `Customer` ON `Project`.customer_Customer_brick_id = `Customer`._brick_id WHERE `Customer`.id = ? ORDER BY created_at DESC) sql 'SELECT COUNT(*) FROM `Project` INNER JOIN `Customer` ON `Project`.customer_Customer_brick_id = `Customer`._brick_id WHERE `Customer`.id = ? ORDER BY created_at DESC' args [de8a7f34-1b22-4fd4-a26d-eb17f7e8c6f2]
E/flutter ( 8337): #0      wrapDatabaseException (package:sqflite/src/exception_impl.dart:11:7)
E/flutter ( 8337): <asynchronous suspension>
E/flutter ( 8337): #1      SqfliteDatabaseMixin.txnRawQuery.<anonymous closure> (package:sqflite_common/src/database_mixin.dart:586:30)
E/flutter ( 8337): <asynchronous suspension>
E/flutter ( 8337): #2      BasicLock.synchronized (package:synchronized/src/basic_lock.dart:33:16)
E/flutter ( 8337): <asynchronous suspension>
E/flutter ( 8337): #3      SqfliteDatabaseMixin.txnSynchronized (package:sqflite_common/src/database_mixin.dart:517:14)
E/flutter ( 8337): <asynchronous suspension>
E/flutter ( 8337): #4      SqliteProvider.exists (package:brick_sqlite/src/sqlite_provider.dart:99:24)
E/flutter ( 8337): <asynchronous suspension>
E/flutter ( 8337): #5      OfflineFirstRepository.exists (package:brick_offline_first/src/offline_first_repository.dart:165:12)
E/flutter ( 8337): <asynchronous suspension>
E/flutter ( 8337): #6      OfflineFirstRepository.get (package:brick_offline_first/src/offline_first_repository.dart:204:27)
E/flutter ( 8337): <asynchronous suspension>
E/flutter ( 8337): #7      OfflineFirstRepository.subscribe.<anonymous closure> (package:brick_offline_first/src/offline_first_repository.dart:375:52)
E/flutter ( 8337): <asynchronous suspension>
E/flutter ( 8337):

2. Problem: Incorrect select in the REST query

(I removed the providerArgs orderBy to test if the rest is working, and noticed the following)

The query contains the following select:

?select=id,customer:customers!customer(id)&id=eq.customer.de8a7f34-1b22-4fd4-a26d-eb17f7e8c6f2

The issue is with the id=eq.customer.de8... part. This should be customer=eq.de8... instead.

I hope this helps, unfortunately I do not have time to create a PR for this today...


I just found another, maybe related issue:

When upserting a Project there is another exception thrown:

DatabaseException(ambiguous column name: created_at (code 1 SQLITE_ERROR): , while compiling: SELECT COUNT(*) FROM `Project` INNER JOIN `Customer` ON `Project`.customer_Customer_brick_id = `Customer`._brick_id WHERE `Customer`.id = ? ORDER BY created_at DESC) sql 'SELECT COUNT(*) FROM `Project` INNER JOIN `Customer` ON `Project`.customer_Customer_brick_id = `Customer`._brick_id WHERE `Customer`.id = ? ORDER BY created_at DESC' args [de8a7f34-1b22-4fd4-a26d-eb17f7e8c6f2] #0      wrapDatabaseException (package:sqflite/src/exception_impl.dart:11:7)
<asynchronous suspension>
#1      SqfliteDatabaseMixin.txnRawQuery.<anonymous closure> (package:sqflite_common/src/database_mixin.dart:586:30)
<asynchronous suspension>
#2      BasicLock.synchronized (package:synchronized/src/basic_lock.dart:33:16)
<asynchronous suspension>
#3      SqfliteDatabaseMixin.txnSynchronized (package:sqflite_common/src/database_mixin.dart:517:14)
<asynchronous suspension>
#4      SqliteProvider.exists (package:brick_sqlite/src/sqlite_provider.dart:99:24)
<asynchronous suspension>
#5      OfflineFirstRepository.notifySubscriptionsWithLocalData (package:brick_offline_first/src/offline_first_repository.dart:324:30)
<asynchronous suspension>
#6      OfflineFirstRepository.upsert (package:brick_offline_first/src/offline_first_repository.dart:400:7)

Upserting all other models works fine, I guess the problem is the association.

@tshedor
Copy link
Collaborator

tshedor commented Aug 29, 2024

Ok good! Now we are finally getting into the real problems with integration. Again, I appreciate your persistence and patience here.

orderBy

Before migrating to the Supabase integration, using the following brick query worked fine:

I'm very curious about this. It shouldn't have worked fine, because the problem would be in the QuerySqlTransformer. It doesn't support ordering by an association columns. It should've thrown this error - you're trying to query by a SQL column that doesn't exist on the source table.

I've added some additional documentation in #430: "The ReferencedTable params are awkward but necessary to not collide with other providers (like SqliteProvider) that also use orderBy and limit. While a foreign_table.foreign_column syntax is more Supabase-like, it is not supported."

SQLite Exception

  1. Problem: DatabaseException

This is exactly what I would've expected from the orderBy problem. Could you verify that this bug does not exist in the Supbase-from-REST provider version of your code?

REST query

  1. Problem: Incorrect select in the REST query

Now this is what I call a bug.

The issue is with the id=eq.customer.de8... part. This should be customer=eq.de8... instead.

This should actually be customer.id=eq.de8... right? Since you're querying an association column

@tshedor
Copy link
Collaborator

tshedor commented Aug 29, 2024

I've published a fix for brick_supabase re: the association (assuming it's customer.id) in #430

@devj3ns
Copy link
Contributor Author

devj3ns commented Aug 30, 2024

orderBy

Using orderByReferencedTable instead of orderBy in the providerArgs fixes the problem, thanks!

SQLite Exception

Could you verify that this bug does not exist in the Supbase-from-REST provider version of your code?

Yes, I am using providerArgs: {'orderBy': 'project.created_at DESC'} in production without issues (note the snake case instead of the camel case).

REST query

This should actually be customer.id=eq.de8... right? Since you're querying an association column

I would assume so too, as it seems to make sense to include the .id, but I just tested it, and it does not work. The REST API ignores the filter and returns all projects and their customer field is set to null.

What works is using customer=eq.de8..., which is what I am currently using in the production version of my app. To be honest, I do not understand why this works but including .id not.

@tshedor
Copy link
Collaborator

tshedor commented Aug 30, 2024

@devj3ns could you please include the full URL with sanitized values for this association query?

tshedor added a commit that referenced this issue Aug 30, 2024
@tshedor
Copy link
Collaborator

tshedor commented Aug 30, 2024

Yes, I am using providerArgs: {'orderBy': 'project.created_at DESC'} in production without issues (note the snake case instead of the camel case).

This is wild stuff. The SQL provider should have caught this. I just added a few unit tests to the SqlProvider and found that there are no exceptions. Thanks for retesting this, and amazing that this hasn't been a problem before. IIRC, several years ago I switched from column names in orderBy to field names, and I believe I kept the column name functionality as graceful degradation.

However, the association querying AFAIK has never been attempted in a project I've managed. SQL should've not been able to order by that table...unless it ignored the qualifier in the order by and skipped straight to the column name. I don't know enough about SQL to understand why this happened or why it was still valid for you, but I guess it's valid.

What works is using customer=eq.de8..., which is what I am currently using in the production version of my app. To be honest, I do not understand why this works but including .id not.

Looking more closely at this. Based on your initial comment on this issue, customer is the name of the column. Therefore, your query shouldn't be

const Where('customer').isExactly(
  const Where('id').isExactly(customer.id.value),
),

It should be

const Where('customer').isExactly(customer.id.value))

This is a collision, because Brick is trying to map the customer field and Supabase API is trying to map the customer column to the same field. Brick is fetching the association based on the customer name. This would never work with offline first then because it's two different values; You'd need to use the @OfflineFirst annotation to make the foreign key association.

What is the Supabase column type of customer? Assuming its a string, deserializing from Supabase will hit a snag because it's going to try to put a String into a Customer type.

Is there any chance you can rename this column to customer_id or the field to anything besides customer? Like customerStruct?

@tshedor tshedor mentioned this issue Aug 30, 2024
29 tasks
@devj3ns
Copy link
Contributor Author

devj3ns commented Sep 2, 2024

That's actually wild with the association querying, thanks for adding these tests.

You'd need to use the @offlinefirst annotation to make the foreign key association.

Oh, yes, good catch @tshedor. I unintentionally removed the @OfflineFirst annotation when I migrated the models for the Supabase integration. Now, the customer field in the project model looks like this:

@OfflineFirst(where: {'id': "data['customer']['id']"})
@Supabase(foreignKey: 'customer')
@Sqlite(index: true)
final Customer customer;

The only difference to before migrating to the Supabase Integration is @Supabase(foreignKey: 'customer'). The rest is the same as before.

What is the Supabase column type of customer

In Supabase the type of the customer column is String (to be more precise UUID) and it is a foreign key to the customers table. The deserialization in toSupabase method is fixed with the re-added @OfflineFirst annotation. It now correctly passes the customers' ID instead of the whole customer object to the REST request when upserting.

Is there any chance you can rename this column to customer_id or the field to anything besides customer?

I agree that storing the id of the customer in a column named customer is not ideal and leads to confusion and the collision. I will change that.

This means the annotation of the customer field of the Project, now, looks like this:

@OfflineFirst(where: {'id': "data['customer']['id']"})
@Supabase(foreignKey: 'customer_id')
@Sqlite(index: true)
final Customer customer;

Is this the correct setup? Because the columnName in the ProjectAdapter is still 'customer' instead of 'customer_id' after running the build_runner (But after the rename, the Supabase database has no column 'customer' anymore):

 @override
  final fieldsToSupabaseColumns = {
  ...
  'customer': const RuntimeSupabaseColumnDefinition(
        association: true,
        columnName: 'customer',
        associationForeignKey: 'customer_id',
        associationType: Customer,
      ),
  ...
}

And how should the where query look like after the rename of the Supabase db column name, to get all projects of a customer?

@tshedor
Copy link
Collaborator

tshedor commented Sep 3, 2024

Is this the correct setup? Because the columnName in the ProjectAdapter is still 'customer' instead of 'customer_id' after running the build_runner (But after the rename, the Supabase database has no column 'customer' anymore):

@devj3ns This looks mostly right, but the Brick API may be incorrect because you're right that the columnName doesn't match associationForeignKey. This is because you're not using @Supabase(name: 'customer_id') but I also don't think you should define it twice. A few options, in order of what I think is the most intuitive API for implementations:

  1. Remove @Supabase(foreignKey:). name: must be specified if it's different than the field (in this case, you would have to specify name: 'customer_id'). Rely on the generator to assume that the column is a foreign key if it's an association. This solution would require less boilerplate but would hide a lot of magic and would be less customizable. I worry that you would expect to provide something here and be alarmed when you couldn't. Or it would simply "work." Brick took inspiration from Rails's ActiveRecord, and this would be the most Rails-y way.
  2. Remove @Supabase(foreignKey:). Add @Supabase(isForeignKey: bool). name: must be specified. Very explicit. You'd have to declare isForeignKey: true to grab the data from the Customer table.
  3. Change nothing and require end users to specify both foreignKey: and name:. There would be no lints, they would just have to do it and struggle with runtime errors if they didn't.

Let me know what option you think is best.

And how should the where query look like after the rename of the Supabase db column name, to get all projects of a customer?

await repository.get<Project>(query: Query.where('customer', Where.exact('id', id))`;

@devj3ns
Copy link
Contributor Author

devj3ns commented Sep 3, 2024

I would personally prefer option 2, as it's most explicit, but I am open to trying option 1 first.

I just tried out #432, and it looks good so far.

There is just the problem with the REST API Query left.

When using the following query:

await repository.getBatched<Project>(query: Query(
  where: [
     Where('customer', value: Where.exact('id', "xyz"))
  ],
  providerArgs: {
    'orderByReferencedTable': 'project.createdAt DESC'
  },
),

this URL is sent to the PostgREST API:

https://[project_id].supabase.co/rest/v1/projects?select=id,customer:customers!customer_id(id,first_name)&customers.id=eq.xyz&limit=50

API Return JSON

[
    {
        "id": "e11d5ce4-07ae-4c90-b9e2-942f2ca3057e",
        "customer": null
    },
    {
        "id": "c1797c69-0929-445d-aaa0-b8d2abbfdfcd",
        "customer": null
    },
    {
        "id": "943a01a2-2fc6-4f83-9440-c0e5f9328207",
        "customer": null
    },
    {
        "id": "e0e2074b-d5dd-40a5-9245-a167493ea1e6",
        "customer": null
    },
    {
        "id": "86b4b666-da3a-4770-8896-574354d391f4",
        "customer": null
    },
    {
        "id": "5b78f082-b0dd-4229-8d44-5e384d7bc486",
        "customer": null
    },
    {
        "id": "8de4c1c3-2c70-48fb-b4c0-bd2b2cffcc63",
        "customer": null
    },
    {
        "id": "8317dbf4-2194-49d2-882e-bfb642b6e4af",
        "customer": null
    },
    {
        "id": "e45e7450-4fe4-4ea9-801b-a5b366060c39",
        "customer": {
            "id": "de8a7f34-1b22-4fd4-a26d-eb17f7e8c6f2",
            "first_name": "a"
        }
    },
    {
        "id": "9438365f-4aac-445f-8375-aad54d313e2c",
        "customer": null
    },
    {
        "id": "f37438e7-cbd2-4ef8-a30f-0ce24701c5d4",
        "customer": null
    },
    {
        "id": "01a92e7e-fd16-4d1f-93be-ba50e0dd8efd",
        "customer": null
    },
    {
        "id": "78a151b0-617d-4ca9-90a3-a0d5d265dcc2",
        "customer": null
    },
    {
        "id": "782c88e2-9e7a-4722-83b9-9c0197fcad68",
        "customer": null
    },
    {
        "id": "93632d93-1853-4a4c-8f4f-b878d9361d58",
        "customer": null
    },
    {
        "id": "410772d7-1d6a-43db-aca2-713ab3a5f838",
        "customer": null
    },
    {
        "id": "416d976e-3639-42bb-b606-11426c0ca03c",
        "customer": null
    },
    {
        "id": "f2384e95-6af8-4092-ab87-dc683eb88f91",
        "customer": null
    },
    {
        "id": "8278fbad-6d8d-4173-bec7-1337f2c66c62",
        "customer": null
    },
    {
        "id": "40494088-443f-484e-be15-7c10f8d44822",
        "customer": {
            "id": "de8a7f34-1b22-4fd4-a26d-eb17f7e8c6f2",
            "first_name": "a"
        }
    }
]

As seen above, it also returns the projects, where not matching customer could be found. This is the case because an outer join is performed, meaning that projects without a matching customer are included, resulting in customer being null. This leads to a NoSuchMethodError in the adapter code of the app.

To only return projects matching the query, we could use an inner join like this:

https://[project_id].supabase.co/rest/v1/projects?select=id,customer:customers!inner(id,first_name)&customers.id=eq.xyz&limit=50

API Return JSON

[
    {
        "id": "e45e7450-4fe4-4ea9-801b-a5b366060c39",
        "customer": {
            "id": "de8a7f34-1b22-4fd4-a26d-eb17f7e8c6f2",
            "first_name": "a"
        }
    },
    {
        "id": "40494088-443f-484e-be15-7c10f8d44822",
        "customer": {
            "id": "de8a7f34-1b22-4fd4-a26d-eb17f7e8c6f2",
            "first_name": "a"
        }
    }
]

The PostgREST docs have a section covering this: https://postgrest.org/en/v12/references/api/resource_embedding.html#top-level-filtering

@tshedor I think we should always make an inner join or null filter when joining with a filter. What do you think?

@tshedor
Copy link
Collaborator

tshedor commented Sep 3, 2024

@devj3ns if your Supabase column is nullable, then your Dart definition should also be nullable - it should be final Customer? instead. Or, if you only want Projects with Customers, there should be a null filter in the query. When I provided the example I wasn't aware that some projects could not have customers.

@tshedor
Copy link
Collaborator

tshedor commented Sep 3, 2024

Or maybe there's another option like @Supabase(excludeNull: bool to provide this. But I think regardless it needs to be configurable since only doing an inner by default would exclude data that you're trying to request

@devj3ns
Copy link
Contributor Author

devj3ns commented Sep 3, 2024

if your Supabase column is nullable, then your Dart definition should also be nullable - it should be final Customer? instead

@tshedor, my customer column in the projects table is non-nullable. It is just that when combining a join with a filter, the PostgREST API by default, returns projects not matching the filter with customer: null (At least that's how I understand it). That's why in this case, an inner join or null filtering is necessary.

https://postgrest.org/en/v12/references/api/resource_embedding.html#top-level-filtering

@tshedor
Copy link
Collaborator

tshedor commented Sep 3, 2024

@devj3ns Ah ok, sorry, now I understand. Let me play with this today. Before I go further down the option 1 path, do you have any other arguments against it or any more support for option 2?

@devj3ns
Copy link
Contributor Author

devj3ns commented Sep 3, 2024

@tshedor, great, thanks!

No, I don't have any more arguments for option 2 except for the increased explicity. I am totally fine with option 1.

@tshedor
Copy link
Collaborator

tshedor commented Sep 4, 2024

@devj3ns Ok cool. I'd rather approach option 2 when the use case is clear for being explicit without being hidden.

I've published and merged #432

@devj3ns
Copy link
Contributor Author

devj3ns commented Sep 4, 2024

@tshedor, I just tested my app with the changes from #432, and it works like a charm, awesome!

It looks like the new version of brick_supabase from the PR (0.1.1+6) is not yet published to pub.dev.

@devj3ns
Copy link
Contributor Author

devj3ns commented Sep 4, 2024

Now I have only one question left and then I will close this issue.

It is related to Unexpected Remote Provider Calls for Associated Data #399. 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 version looks like follows:

Future<Project> _$ProjectFromSupabase(Map<String, dynamic> data,
    {required SupabaseProvider provider,
    OfflineFirstWithSupabaseRepository? repository}) async {
    return Project(
        ....
         customer: await repository!
          .getAssociation<Customer>(Query(
              where: [Where.exact('id', data['customer']['id'])],
              providerArgs: {'limit': 1}))
          .then((r) => r!.first),
        ....
      ),
}
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 customer field on the Project model looks like follows:

@OfflineFirst(where: {'id': "data['customer']['id']"})
@Supabase(name: 'customer_id', toGenerator: '%INSTANCE_PROPERTY%.id.value')
@Sqlite(index: true)
final Customer customer;

@tshedor with the new Supabase integration in place, is it possible that brick does not make these redundant requests by default?

@tshedor
Copy link
Collaborator

tshedor commented Sep 4, 2024

it works like a charm, awesome!

@devj3ns What a relief. I go to bed wondering if I've done enough for your overnight test. Great news.

It looks like the new version of brick_supabase from the PR (0.1.1+6) is not yet published to pub.dev.

Done, not sure why that didn't publish

with the new Supabase integration in place, is it possible that brick does not make these redundant requests by default?

This is a hunch. Forget my advice to include @OfflineFirst(where:). Try removing that annotation. The GraphQL provider doesn't need it because all of the associations are nested in each document. I added some similar documentation for Supabase in the last PR since Supabase similarly grabs all fields on the fetch.

@devj3ns
Copy link
Contributor Author

devj3ns commented Sep 4, 2024

@tshedor when I remove the @OfflineFirst(where:) annotation from the customer field, the _$ProjectFromSupabase adapter changes to the following:

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),
    ....
   );
}

This means the FromSupabase adapter does no longer produce these redundant REST-API Request.

However, the _$ProjectFromSqlite adapter code is still the same as with the @OfflineFirst(where:) annotation:

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,
         ...
     ),
}

Because of the repository!.getAssociation in there, the redundant REST API requests are sent.

If I would manually change this adapter code to only fetch the local data like follows, the redundant REST API requests are gone:

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

To me, this means that the FromSqlite adapter code generation should be adjusted, what do you think Tim?

@tshedor
Copy link
Collaborator

tshedor commented Sep 5, 2024

@devj3ns Unless I'm missing something, that's the same code block for both generated and manually changed.

However, I think I know the problem. You're in an esoteric part of Brick, so I'm going to explain with probably too much context for clarity. tl;dr use repository.get<Project>(seedOnly: true)

For the first app that used Brick, we discovered this same problem - our API was overwhelmed and often failed on the first fetch of a model with any associations (for this example, let's say that Customer has an Address association and Project has a Customer association). We initially resolved this by creating an eager loading class that carefully loaded the lowest associations first and then loaded the models that held those associations - load all Addresses, then load all Customers, then load all Projects. That way, the API would never do duplicate fetching.

This was fine until we realized that now SQLite was overwhelmed by fetching each association of each association of each association individually. By this point, we were nearing a key beta milestone and didn't have time to rewrite the adapter and SQL layer to do queries that fetched multiple models at once instead of fetching them by their primary key. So the parameter seedOnly was introduced.

seedOnly on the OfflineFirstRepository#get method bypassed the SQLite deserialization. We applied seedOnly on the sequential eager load and performance was good and subsequent get queries retrieved local associations.

After beta passed, we found the performance of SQLite to still be good without refactoring to do batch deserializations. We also found Brick's encapsulation to be acceptable - maybe the memory provider would have the instance and fetching it from the SQLite provider would be actually be more expensive than if we were to just fetch the missing models individually. So we decided not to revisit the batch SQL and retain the existing seedOnly.


For your case, I believe that SQLite is deserializing needlessly, so seedOnly: true should do the trick.

There is a chance that this theory is wrong and that the problem is associations are not being serialized into SQLite. If this is the case, the solution will be in changing storeRemoteResults to serialize associations first. Flutter's Dart doesn't support reflections, which is what we'd need here, so I'll have to think on this solution.


Let me know if seedOnly: true does the trick. If it doesn't, please open another issue for it with the context from your comment.

@devj3ns
Copy link
Contributor Author

devj3ns commented Sep 5, 2024

Unless I'm missing something, that's the same code block for both generated and manually changed.

Oh, my mistake, I updated my message above. I wanted to use repository!.get with the localOnly policy inside the FromSqlite adapter.

Thanks for the detailed explanation @tshedor! I will read through it later and test it out.

@devj3ns
Copy link
Contributor Author

devj3ns commented Sep 5, 2024

Thank you for the detailed explanation @tshedor! seedOnly: true helps to reduce the number of requests sent to fetch a customer, but there are still happening many unnecessary requests.

I created an separate issue with some more context #433

I appreciate your work and time @tshedor. I will close this issue as it is resolved.

@devj3ns devj3ns closed this as completed Sep 5, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

No branches or pull requests

2 participants