fix(supabase): improve query transformer with recursion and column na…

…me (#474)

docs/offline_first/offline_first_with_supabase_repository.md

@@ -175,3 +175,48 @@ class Address extends OfflineFirstWithSupabaseModel{
 ```
 
 !> If your association is nullable (e.g. `Address?`), the Supabase response may include all `User`s from the database from a loosely-specified query. This is caused by PostgREST's [filtering](https://docs.postgrest.org/en/v12/references/api/resource_embedding.html#top-level-filtering). Brick does not use `!inner` to query tables because there is no guarantee that a model does not have multiple fields relating to the same association; it instead explicitly declares the foreign key with [not.is.null](https://docs.postgrest.org/en/v12/references/api/resource_embedding.html#null-filtering-on-embedded-resources) filtering. If a Dart association is nullable, Brick will not append the `not.is.null` which could return [all results](https://github.com/GetDutchie/brick/issues/429#issuecomment-2325941205). If you have a use case that requires a nullable association and you cannot circumvent this problem with [Supabase's policies](https://supabase.com/docs/guides/database/postgres/row-level-security), please open an issue and provide extensive detail.
+
+### Recursive/Looped Associations
+
+If a request includes a nested parent-child recursion, the generated Supabase query will remove the association to prevent a stack overflow.
+
+For example, given the following models:
+
+```dart
+class Parent extends OfflineFirstWithSupabaseModel {
+  final String parentId;
+  final List<Child> children;
+}
+class Child extends OfflineFirstWithSupabaseModel {
+  final String childId;
+  final Parent parent;
+}
+```
+
+A query for `MyRepository().get<Parent>()` would generate a Supabase query that only gets the shallow properties of Parent on the second level of recursion:
+
+```
+parent:parent_table(
+  parentId,
+  children:child_table(
+    childId,
+    parent:parent_table(
+      parentId
+    )
+  )
+)
+```
+
+Implementations using looping associations like this should design for their parent (first-level) models to accept `null` or empty child associations:
+
+```dart
+class Parent extends OfflineFirstWithSupabaseModel {
+  final String parentId;
+  final List<Child> children;
+
+  Parent({
+    required this.parentId,
+    List<Child>? children,
+  }) : children = children ?? List<Child>[];
+}
+```

packages/brick_offline_first_with_supabase/README.md

@@ -151,6 +151,51 @@ class Address extends OfflineFirstWithSupabaseModel{
 
 :warning: If your association is nullable (e.g. `Address?`), the Supabase response may include all `User`s from the database from a loosely-specified query. This is caused by PostgREST's [filtering](https://docs.postgrest.org/en/v12/references/api/resource_embedding.html#top-level-filtering). Brick does not use `!inner` to query tables because there is no guarantee that a model does not have multiple fields relating to the same association; it instead explicitly declares the foreign key with [not.is.null](https://docs.postgrest.org/en/v12/references/api/resource_embedding.html#null-filtering-on-embedded-resources) filtering. If a Dart association is nullable, Brick will not append the `not.is.null` which could return [all results](https://github.com/GetDutchie/brick/issues/429#issuecomment-2325941205). If you have a use case that requires a nullable association and you cannot circumvent this problem with [Supabase's policies](https://supabase.com/docs/guides/database/postgres/row-level-security), please open an issue and provide extensive detail.
 
+#### Recursive/Looped Associations
+
+If a request includes a nested parent-child recursion, the generated Supabase query will remove the association to prevent a stack overflow.
+
+For example, given the following models:
+
+```dart
+class Parent extends OfflineFirstWithSupabaseModel {
+  final String parentId;
+  final List<Child> children;
+}
+class Child extends OfflineFirstWithSupabaseModel {
+  final String childId;
+  final Parent parent;
+}
+```
+
+A query for `MyRepository().get<Parent>()` would generate a Supabase query that only gets the shallow properties of Parent on the second level of recursion:
+
+```
+parent:parent_table(
+  parentId,
+  children:child_table(
+    childId,
+    parent:parent_table(
+      parentId
+    )
+  )
+)
+```
+
+Implementations using looping associations like this should design for their parent (first-level) models to accept `null` or empty child associations:
+
+```dart
+class Parent extends OfflineFirstWithSupabaseModel {
+  final String parentId;
+  final List<Child> children;
+
+  Parent({
+    required this.parentId,
+    List<Child>? children,
+  }) : children = children ?? List<Child>[];
+}
+```
+
 #### OfflineFirst(where:)
 
 Ideally, `@OfflineFirst(where:)` shouldn't be necessary to specify to make the association between local Brick and remote Supabase because the generated Supabase `.select` should include all nested fields. However, if there are [too many](https://github.com/GetDutchie/brick/issues/399) REST calls, it may be necessary to guide Brick to the right foreign keys.

packages/brick_supabase/CHANGELOG.md

@@ -1,5 +1,11 @@
 ## Unreleased
 
+## 1.1.1
+
+- Fix a query builder infinite recursion bug where a parent has a child that has a parent association
+- Use declared Supabase `columnName` when requesting an association from Supabase
+- Do not declare a `:tableName` on a Supabase query if no table name is available on the adapter. This should never happen since the code is generated
+
 ## 1.1.0
 
 - Reorganized `testing.dart` to `src/testing`

packages/brick_supabase/lib/src/query_supabase_transformer.dart

@@ -24,7 +24,7 @@ class QuerySupabaseTransformer<_Model extends SupabaseModel> {
   }) : adapter = adapter ?? modelDictionary.adapterFor[_Model]!;
 
   String get selectFields {
-    return destructureAssociationProperties(adapter.fieldsToSupabaseColumns).join(',');
+    return destructureAssociationProperties(adapter.fieldsToSupabaseColumns, _Model).join(',');
   }
 
   PostgrestTransformBuilder<List<Map<String, dynamic>>> applyProviderArgs(
@@ -55,25 +55,50 @@ class QuerySupabaseTransformer<_Model extends SupabaseModel> {
   @protected
   @visibleForTesting
   List<String> destructureAssociationProperties(
-    Map<String, RuntimeSupabaseColumnDefinition>? columns,
-  ) {
+    Map<String, RuntimeSupabaseColumnDefinition>? columns, [
+    Type? destructuringFromType,
+    int recursionDepth = 0,
+  ]) {
     final selectedFields = <String>[];
 
     if (columns == null) return selectedFields;
 
     for (final entry in columns.entries) {
       final field = entry.value;
       if (field.association && field.associationType != null) {
-        var associationOutput =
-            '${entry.key}:${modelDictionary.adapterFor[field.associationType!]?.supabaseTableName}';
+        var associationOutput = field.columnName;
+        if (modelDictionary.adapterFor[field.associationType!]?.supabaseTableName != null) {
+          associationOutput +=
+              ':${modelDictionary.adapterFor[field.associationType!]?.supabaseTableName}';
+        }
         if (field.foreignKey != null) {
           associationOutput += '!${field.foreignKey}';
         }
         associationOutput += '(';
+
+        // If a request includes a nested parent-child recursion, prevent the destructurization
+        // from hitting a stack overflow by removing the association from the child destructure.
+        //
+        // For example, given `Parent(id:, child: Child(id:, parent:))`, this would strip the
+        // association query without touching the parennt's original properties as
+        // `parent(id, child(id, parent(id)))`.
+        //
+        // Implementations using looping associations like this should design for their
+        // parent (first-level) models to accept null or empty child associations.
+        var fieldsToDestructure =
+            modelDictionary.adapterFor[field.associationType!]?.fieldsToSupabaseColumns;
+        if (recursionDepth >= 1) {
+          // Clone to avoid concurrent writes to the map
+          fieldsToDestructure = {...?fieldsToDestructure}
+            ..removeWhere((k, v) => v.associationType == destructuringFromType);
+        }
         final fields = destructureAssociationProperties(
-          modelDictionary.adapterFor[field.associationType!]?.fieldsToSupabaseColumns,
+          fieldsToDestructure,
+          field.associationType,
+          recursionDepth + 1,
         );
         associationOutput += fields.join(',');
+
         associationOutput += ')';
 
         selectedFields.add(associationOutput);

packages/brick_supabase/pubspec.yaml

@@ -4,7 +4,7 @@ homepage: https://github.com/GetDutchie/brick/tree/main/packages/brick_supabase
 issue_tracker: https://github.com/GetDutchie/brick/issues
 repository: https://github.com/GetDutchie/brick
 
-version: 1.1.0
+version: 1.1.1
 
 environment:
   sdk: ">=3.0.0 <4.0.0"