Closed devj3ns closed 1 week ago
Ok good! Now we are finally getting into the real problems with integration. Again, I appreciate your persistence and patience here.
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."
- 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?
- 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 becustomer=eq.de8...
instead.
This should actually be customer.id=eq.de8...
right? Since you're querying an association column
I've published a fix for brick_supabase
re: the association (assuming it's customer.id
) in #430
Using orderByReferencedTable
instead of orderBy
in the providerArgs
fixes the problem, thanks!
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).
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.
@devj3ns could you please include the full URL with sanitized values for this association query?
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
?
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?
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:
@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.@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. 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))`;
I would personally prefer option 2, as it's most explicit, but I am open to trying option 1 first.
I just tried out https://github.com/GetDutchie/brick/pull/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
```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
```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?
@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.
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
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
@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?
@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.
@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
@tshedor, I just tested my app with the changes from https://github.com/GetDutchie/brick/pull/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.
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?
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.
@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?
@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.
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.
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 https://github.com/GetDutchie/brick/issues/433
I appreciate your work and time @tshedor. I will close this issue as it is resolved.
In my project, I have a model
Project
and a modelCustomer
: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:
With the new Supabase integration, their are two problems:
1. Problem: DatabaseException
When querying, the following exception is thrown:
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 becustomer=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:Upserting all other models works fine, I guess the problem is the association.