From a0a879de0d8f78e35e2a85416ce131645f2c2dc0 Mon Sep 17 00:00:00 2001 From: Tim Shedor Date: Tue, 29 Oct 2024 09:16:41 -0700 Subject: [PATCH] resolve reduced delete payload; pr feedback --- .../lib/src/offline_first_repository.dart | 7 +- ...ffline_first_with_supabase_repository.dart | 80 ++++++---- ...e_first_with_supabase_repository_test.dart | 144 ++++++++++++++++-- packages/brick_sqlite/CHANGELOG.md | 2 - .../src/runtime_sqlite_column_definition.dart | 2 +- .../brick_sqlite/lib/src/sqlite_provider.dart | 8 - .../lib/src/testing/supabase_mock_server.dart | 11 +- 7 files changed, 192 insertions(+), 62 deletions(-) diff --git a/packages/brick_offline_first/lib/src/offline_first_repository.dart b/packages/brick_offline_first/lib/src/offline_first_repository.dart index 3dcb4b23..1c3b23b3 100644 --- a/packages/brick_offline_first/lib/src/offline_first_repository.dart +++ b/packages/brick_offline_first/lib/src/offline_first_repository.dart @@ -345,10 +345,9 @@ abstract class OfflineFirstRepository subscriptions.clear(); } - /// Listen for streaming changes when the [sqliteProvider]. - /// This method utilizes [remoteProvider]'s [get]. For example, whenever new - /// data is acquired from remote, or data is upserted locally, or data is - /// deleted locally, the stream will be notified with a local fetch of [query]. + /// Listen for streaming changes when the [sqliteProvider] is invoked. For example, + /// whenever new data is acquired from remote, or data is upserted locally, or + /// data is deleted locally, the stream will be notified with a local fetch of [query]. /// /// [get] is invoked on the [memoryCacheProvider] and [sqliteProvider] following an [upsert] /// invocation. For more, see [notifySubscriptionsWithLocalData]. diff --git a/packages/brick_offline_first_with_supabase/lib/src/offline_first_with_supabase_repository.dart b/packages/brick_offline_first_with_supabase/lib/src/offline_first_with_supabase_repository.dart index d32871e2..70249d3f 100644 --- a/packages/brick_offline_first_with_supabase/lib/src/offline_first_with_supabase_repository.dart +++ b/packages/brick_offline_first_with_supabase/lib/src/offline_first_with_supabase_repository.dart @@ -147,6 +147,33 @@ abstract class OfflineFirstWithSupabaseRepository await offlineRequestQueue.client.requestManager.migrate(); } + /// Supabase's realtime payload only returns unique columns; + /// the instance must be discovered from these values so it + /// can be deleted by all providers. + @protected + @visibleForOverriding + @visibleForTesting + Query queryFromSupabaseDeletePayload( + Map payload, { + required Map supabaseDefinitions, + }) { + final fieldsWithValues = payload.entries.fold({}, (acc, entry) { + for (final f in supabaseDefinitions.entries) { + if (f.value.columnName == entry.key) { + acc[f.key] = entry.value; + break; + } + } + + return acc; + }); + + return Query( + where: fieldsWithValues.entries.map((entry) => Where.exact(entry.key, entry.value)).toList(), + providerArgs: {'limit': 1}, + ); + } + @protected @visibleForTesting @visibleForOverriding @@ -195,12 +222,12 @@ abstract class OfflineFirstWithSupabaseRepository /// /// See [subscribe] for reactivity without using realtime. /// - /// `eventType` is the triggering remote event. + /// [eventType] is the triggering remote event. /// - /// `policy` determines how data is fetched (local or remote). When `.localOnly`, + /// [policy] determines how data is fetched (local or remote). When [OfflineFirstGetPolicy.localOnly], /// Supabase channels will not be used. /// - /// `query` is an optional query to filter the data. The query **must be** one level - + /// [query] is an optional query to filter the data. The query **must be** one level - /// `Query.where('user', Query.exact('name', 'Tom'))` is invalid but `Query.where('name', 'Tom')` /// is valid. The [Compare] operator is limited to a [PostgresChangeFilterType] equivalent. /// See [_compareToFilterParam] for a precise breakdown. @@ -247,46 +274,37 @@ abstract class OfflineFirstWithSupabaseRepository memoryCacheProvider.delete(deletableModel, repository: this); } - case PostgresChangeEvent.insert: - // Convert Supabase JSON to Brick model instance - final instance = await adapter.fromSupabase( - payload.newRecord, - provider: remoteProvider, - repository: this, + case PostgresChangeEvent.delete: + final query = queryFromSupabaseDeletePayload( + payload.oldRecord, + supabaseDefinitions: adapter.fieldsToSupabaseColumns, ); - // Save to local SQLite database for offline access - await sqliteProvider.upsert(instance as TModel, repository: this); - // Update memory cache for fast retrieval - memoryCacheProvider.upsert(instance, repository: this); - case PostgresChangeEvent.update: - // Convert Supabase JSON to Brick model instance + if (query.where?.isEmpty ?? true) return; + + final results = await get( + query: query, + policy: OfflineFirstGetPolicy.localOnly, + seedOnly: true, + ); + if (results.isEmpty) return; + + await sqliteProvider.delete(results.first, repository: this); + memoryCacheProvider.delete(results.first, repository: this); + + case PostgresChangeEvent.insert || PostgresChangeEvent.update: final instance = await adapter.fromSupabase( payload.newRecord, provider: remoteProvider, repository: this, ); - // Save to local SQLite database for offline access + await sqliteProvider.upsert(instance as TModel, repository: this); - // Update memory cache for fast retrieval memoryCacheProvider.upsert(instance, repository: this); - - case PostgresChangeEvent.delete: - // Convert Supabase JSON to Brick model instance - final instance = await adapter.fromSupabase( - payload.oldRecord, - provider: remoteProvider, - repository: this, - ); - final primaryKey = - await sqliteProvider.primaryKeyByUniqueColumns(instance as TModel); - instance.primaryKey = primaryKey; - await sqliteProvider.delete(instance, repository: this); - memoryCacheProvider.delete(instance, repository: this); } await notifySubscriptionsWithLocalData( - subscriptionsByQuery: supabaseRealtimeSubscriptions[TModel]![eventType], + subscriptionsByQuery: supabaseRealtimeSubscriptions[TModel]![eventType]!, ); }, ) diff --git a/packages/brick_offline_first_with_supabase/test/offline_first_with_supabase_repository_test.dart b/packages/brick_offline_first_with_supabase/test/offline_first_with_supabase_repository_test.dart index 191ac31a..1b785499 100644 --- a/packages/brick_offline_first_with_supabase/test/offline_first_with_supabase_repository_test.dart +++ b/packages/brick_offline_first_with_supabase/test/offline_first_with_supabase_repository_test.dart @@ -91,6 +91,126 @@ void main() async { }); }); + group('#queryFromSupabaseDeletePayload', () { + test('simple', () { + final payload = { + 'id': 1, + }; + + final supabaseDefinitions = { + 'id': RuntimeSupabaseColumnDefinition(columnName: 'id'), + 'name': RuntimeSupabaseColumnDefinition(columnName: 'name'), + }; + + final query = repository.queryFromSupabaseDeletePayload( + payload, + supabaseDefinitions: supabaseDefinitions, + ); + + expect(query.where, hasLength(1)); + expect(query.where!.first.evaluatedField, 'id'); + expect(query.where!.first.value, 1); + expect(query.providerArgs, equals({'limit': 1})); + }); + + test('payload entries not present in supabaseDefinitions', () { + final payload = { + 'id': 1, + 'unknown_field': 'some value', + }; + + final supabaseDefinitions = { + 'id': RuntimeSupabaseColumnDefinition(columnName: 'id'), + 'name': RuntimeSupabaseColumnDefinition(columnName: 'name'), + }; + + final query = repository.queryFromSupabaseDeletePayload( + payload, + supabaseDefinitions: supabaseDefinitions, + ); + + expect(query.where, hasLength(1)); + expect(query.where!.first.evaluatedField, 'id'); + expect(query.where!.first.value, 1); + expect(query.providerArgs, equals({'limit': 1})); + }); + + test('empty payload', () { + final payload = {}; + final supabaseDefinitions = { + 'id': RuntimeSupabaseColumnDefinition(columnName: 'id'), + }; + + final query = repository.queryFromSupabaseDeletePayload( + payload, + supabaseDefinitions: supabaseDefinitions, + ); + + expect(query.where, isEmpty); + }); + + test('payload with no matching definitions', () { + final payload = { + 'unknown_field': 'some value', + }; + final supabaseDefinitions = { + 'id': RuntimeSupabaseColumnDefinition(columnName: 'id'), + }; + + final query = repository.queryFromSupabaseDeletePayload( + payload, + supabaseDefinitions: supabaseDefinitions, + ); + + expect(query.where, isEmpty); + }); + + test('different column names', () { + final payload = { + 'user_id': 1, + }; + + final supabaseDefinitions = { + 'id': RuntimeSupabaseColumnDefinition(columnName: 'user_id'), + 'name': RuntimeSupabaseColumnDefinition(columnName: 'full_name'), + }; + + final query = repository.queryFromSupabaseDeletePayload( + payload, + supabaseDefinitions: supabaseDefinitions, + ); + + expect(query.where, hasLength(1)); + expect(query.where!.first.evaluatedField, 'id'); + expect(query.where!.first.value, 1); + expect(query.providerArgs, equals({'limit': 1})); + }); + + test('multiple columns', () { + final payload = { + 'user_id': 1, + 'full_name': 'Thomas', + }; + + final supabaseDefinitions = { + 'id': RuntimeSupabaseColumnDefinition(columnName: 'user_id'), + 'name': RuntimeSupabaseColumnDefinition(columnName: 'full_name'), + }; + + final query = repository.queryFromSupabaseDeletePayload( + payload, + supabaseDefinitions: supabaseDefinitions, + ); + + expect(query.where, hasLength(2)); + expect(query.where!.first.evaluatedField, 'id'); + expect(query.where!.first.value, 1); + expect(query.where!.last.evaluatedField, 'name'); + expect(query.where!.last.value, 'Thomas'); + expect(query.providerArgs, equals({'limit': 1})); + }); + }); + group('#queryToPostgresChangeFilter', () { group('returns null', () { test('for complex queries', () { @@ -111,21 +231,19 @@ void main() async { group('Compare', () { test('.between', () { - final query = - Query(where: [Where('firstName', value: 'Thomas', compare: Compare.between)]); + final query = Query(where: [Where('firstName').isBetween(1, 2)]); final filter = repository.queryToPostgresChangeFilter(query); expect(filter, isNull); }); test('.doesNotContain', () { - final query = - Query(where: [Where('firstName', value: 'Thomas', compare: Compare.doesNotContain)]); + final query = Query(where: [Where('firstName').doesNotContain('Thomas')]); final filter = repository.queryToPostgresChangeFilter(query); expect(filter, isNull); }); test('.exact', () { - final query = Query(where: [Where('firstName', value: 'Thomas', compare: Compare.exact)]); + final query = Query(where: [Where.exact('firstName', 'Thomas')]); final filter = repository.queryToPostgresChangeFilter(query); expect(filter!.type, PostgresChangeFilterType.eq); @@ -134,8 +252,7 @@ void main() async { }); test('.greaterThan', () { - final query = - Query(where: [Where('firstName', value: 'Thomas', compare: Compare.greaterThan)]); + final query = Query(where: [Where('firstName').isGreaterThan('Thomas')]); final filter = repository.queryToPostgresChangeFilter(query); expect(filter!.type, PostgresChangeFilterType.gt); @@ -145,7 +262,7 @@ void main() async { test('.greaterThanOrEqualTo', () { final query = Query( - where: [Where('firstName', value: 'Thomas', compare: Compare.greaterThanOrEqualTo)], + where: [Where('firstName').isGreaterThanOrEqualTo('Thomas')], ); final filter = repository.queryToPostgresChangeFilter(query); @@ -155,8 +272,7 @@ void main() async { }); test('.lessThan', () { - final query = - Query(where: [Where('firstName', value: 'Thomas', compare: Compare.lessThan)]); + final query = Query(where: [Where('firstName').isLessThan('Thomas')]); final filter = repository.queryToPostgresChangeFilter(query); expect(filter!.type, PostgresChangeFilterType.lt); @@ -166,7 +282,7 @@ void main() async { test('.lessThanOrEqualTo', () { final query = Query( - where: [Where('firstName', value: 'Thomas', compare: Compare.lessThanOrEqualTo)], + where: [Where('firstName').isLessThanOrEqualTo('Thomas')], ); final filter = repository.queryToPostgresChangeFilter(query); @@ -176,8 +292,7 @@ void main() async { }); test('.notEqual', () { - final query = - Query(where: [Where('firstName', value: 'Thomas', compare: Compare.notEqual)]); + final query = Query(where: [Where('firstName').isNot('Thomas')]); final filter = repository.queryToPostgresChangeFilter(query); expect(filter!.type, PostgresChangeFilterType.neq); @@ -186,8 +301,7 @@ void main() async { }); test('.contains', () { - final query = - Query(where: [Where('firstName', value: 'Thomas', compare: Compare.contains)]); + final query = Query(where: [Where('firstName').contains('Thomas')]); final filter = repository.queryToPostgresChangeFilter(query); expect(filter!.type, PostgresChangeFilterType.inFilter); diff --git a/packages/brick_sqlite/CHANGELOG.md b/packages/brick_sqlite/CHANGELOG.md index 9aeb0525..98c45907 100644 --- a/packages/brick_sqlite/CHANGELOG.md +++ b/packages/brick_sqlite/CHANGELOG.md @@ -1,7 +1,5 @@ ## Unreleased -- Add `#primaryKeyByUniqueColumns` to `SqliteProvider` to expose forward functionality - ## 3.1.1 - Expose a generic type for `MemoryCacheProvider` models diff --git a/packages/brick_sqlite/lib/src/runtime_sqlite_column_definition.dart b/packages/brick_sqlite/lib/src/runtime_sqlite_column_definition.dart index 5765cbc5..a7ce2610 100644 --- a/packages/brick_sqlite/lib/src/runtime_sqlite_column_definition.dart +++ b/packages/brick_sqlite/lib/src/runtime_sqlite_column_definition.dart @@ -1,4 +1,4 @@ -/// Used to define types in [SqliteAdapter#sqliteFieldsToColumns]. The build runner package +/// Used to define types in [SqliteAdapter#fieldsToSqliteColumns]. The build runner package /// extracts types and associations that would've been otherwise inaccessible at runtime. class RuntimeSqliteColumnDefinition { /// Whether this column relates to another SqliteModel diff --git a/packages/brick_sqlite/lib/src/sqlite_provider.dart b/packages/brick_sqlite/lib/src/sqlite_provider.dart index 27dbb3c8..ceb96be2 100644 --- a/packages/brick_sqlite/lib/src/sqlite_provider.dart +++ b/packages/brick_sqlite/lib/src/sqlite_provider.dart @@ -203,14 +203,6 @@ class SqliteProvider implements Provider { } } - /// Retrieve a primary key by querying its unique columns. - /// Advanced use only. - Future primaryKeyByUniqueColumns(TModel instance) async { - final adapter = modelDictionary.adapterFor[TModel]!; - final db = await getDb(); - return await adapter.primaryKeyByUniqueColumns(instance, db); - } - /// Fetch results for model with a custom SQL statement. /// It is recommended to use [get] whenever possible. **Advanced use only**. Future> rawGet( diff --git a/packages/brick_supabase/lib/src/testing/supabase_mock_server.dart b/packages/brick_supabase/lib/src/testing/supabase_mock_server.dart index 93e0db41..5dc8bc29 100644 --- a/packages/brick_supabase/lib/src/testing/supabase_mock_server.dart +++ b/packages/brick_supabase/lib/src/testing/supabase_mock_server.dart @@ -200,7 +200,7 @@ class SupabaseMockServer { PostgresChangeEvent? realtimeEvent, ModelRepository? repository, }) async { - assert(realtimeEvent != PostgresChangeEvent.all, '.all realtime events are not serialized'); + assert(realtimeEvent != PostgresChangeEvent.all, '.all events are not serialized'); final adapter = modelDictionary.adapterFor[TModel]!; final serialized = await adapter.toSupabase( @@ -211,6 +211,15 @@ class SupabaseMockServer { if (realtimeEvent == null) return serialized; + // Delete records from realtime are strictly unique/indexed fields; + // uniqueness is not tracked by [RuntimeSupabaseColumnDefinition] + // so filtering out associations is the closest simulation of an incomplete payload + if (realtimeEvent == PostgresChangeEvent.delete) { + for (final value in adapter.fieldsToSupabaseColumns.values) { + if (value.association) serialized.remove(value.columnName); + } + } + return { 'ref': null, 'event': 'postgres_changes',