From 742837b2d1547a3d6c7c23e4d42367a5d82b3a32 Mon Sep 17 00:00:00 2001 From: Nikolas Rimikis Date: Fri, 15 Mar 2024 14:00:40 +0100 Subject: [PATCH] refactor(neon_framework): add account column to RequestCache Signed-off-by: Nikolas Rimikis --- .../lib/src/storage/request_cache.dart | 87 ++++++++++++------- .../lib/src/utils/request_manager.dart | 15 ++-- .../neon_framework/test/persistence_test.dart | 27 +++--- .../test/request_manager_test.dart | 77 ++++++++-------- 4 files changed, 122 insertions(+), 84 deletions(-) diff --git a/packages/neon_framework/lib/src/storage/request_cache.dart b/packages/neon_framework/lib/src/storage/request_cache.dart index e5469a525cb..b598c54746d 100644 --- a/packages/neon_framework/lib/src/storage/request_cache.dart +++ b/packages/neon_framework/lib/src/storage/request_cache.dart @@ -2,6 +2,7 @@ import 'dart:async'; import 'package:logging/logging.dart'; import 'package:meta/meta.dart'; +import 'package:neon_framework/models.dart'; import 'package:neon_framework/src/platform/platform.dart'; import 'package:neon_framework/src/utils/request_manager.dart'; import 'package:path/path.dart' as p; @@ -17,18 +18,18 @@ abstract interface class RequestCache { /// /// Use [getParameters] if you only need to check whether the cache is still /// valid. - Future get(String key); + Future get(Account account, String key); /// Set's the cached [value] at the given [key]. /// /// If a value is already present it will be updated with the new one. - Future set(String key, String value, CacheParameters? parameters); + Future set(Account account, String key, String value, CacheParameters? parameters); /// Retrieves the cache parameters for the given [key]. - Future getParameters(String key); + Future getParameters(Account account, String key); /// Updates the cache [parameters] for a given [key] without modifying the `value`. - Future updateParameters(String key, CacheParameters? parameters); + Future updateParameters(Account account, String key, CacheParameters? parameters); } /// Default implementation of the [RequestCache]. @@ -64,30 +65,32 @@ final class DefaultRequestCache implements RequestCache { final cacheDir = await getApplicationCacheDirectory(); database = await openDatabase( p.join(cacheDir.path, 'cache.db'), - version: 2, + version: 3, onCreate: onCreate, onUpgrade: (db, oldVersion, newVersion) async { - final batch = db.batch(); - if (oldVersion == 1) { - batch - ..execute('ALTER TABLE cache ADD COLUMN etag TEXT') - ..execute('ALTER TABLE cache ADD COLUMN expires INTEGER'); - } - await batch.commit(); + // We can safely drop the table as it only contains cached data. + // Non breaking migrations should not drop the cache. The next + // breaking change should remove all non breaking migrations before it. + await db.transaction((txn) async { + if (oldVersion <= 2) { + await txn.execute('DROP TABLE cache'); + await onCreate(txn); + } + }); }, ); } @visibleForTesting - static Future onCreate(Database db, int version) async { + static Future onCreate(DatabaseExecutor db, [int? version]) async { await db.execute(''' CREATE TABLE "cache" ( - "id" INTEGER UNIQUE, - "key" TEXT UNIQUE, - "value" TEXT, + "account" TEXT NOT NULL, + "key" TEXT NOT NULL, + "value" TEXT NOT NULL, "etag" TEXT, "expires" INTEGER, - PRIMARY KEY("id") + PRIMARY KEY("key","account") ); '''); } @@ -104,10 +107,17 @@ CREATE TABLE "cache" ( } @override - Future get(String key) async { + Future get(Account account, String key) async { List>? result; try { - result = await _requireDatabase.rawQuery('SELECT value FROM cache WHERE key = ?', [key]); + result = await _requireDatabase.rawQuery( + ''' +SELECT value +FROM cache +WHERE account = ? AND key = ? + ''', + [account.id, key], + ); } on DatabaseException catch (error, stackTrace) { _log.severe( 'Error while getting `$key` from cache.', @@ -120,7 +130,7 @@ CREATE TABLE "cache" ( } @override - Future set(String key, String value, CacheParameters? parameters) async { + Future set(Account account, String key, String value, CacheParameters? parameters) async { try { // UPSERT is only available since SQLite 3.24.0 (June 4, 2018). // Using a manual solution from https://stackoverflow.com/a/38463024 @@ -128,17 +138,28 @@ CREATE TABLE "cache" ( ..update( 'cache', { + 'account': account.id, 'key': key, 'value': value, 'etag': parameters?.etag, 'expires': parameters?.expires?.millisecondsSinceEpoch, }, - where: 'key = ?', - whereArgs: [key], + where: 'account = ? AND key = ?', + whereArgs: [account.id, key], ) ..rawInsert( - 'INSERT INTO cache (key, value, etag, expires) SELECT ?, ?, ?, ? WHERE (SELECT changes() = 0)', - [key, value, parameters?.etag, parameters?.expires?.millisecondsSinceEpoch], + ''' +INSERT INTO cache (account, key, value, etag, expires) +SELECT ?, ?, ?, ?, ? +WHERE (SELECT changes() = 0) +''', + [ + account.id, + key, + value, + parameters?.etag, + parameters?.expires?.millisecondsSinceEpoch, + ], ); await batch.commit(noResult: true); } on DatabaseException catch (error, stackTrace) { @@ -151,10 +172,17 @@ CREATE TABLE "cache" ( } @override - Future getParameters(String key) async { + Future getParameters(Account account, String key) async { List>? result; try { - result = await _requireDatabase.rawQuery('SELECT etag, expires FROM cache WHERE key = ?', [key]); + result = await _requireDatabase.rawQuery( + ''' +SELECT etag, expires +FROM cache +WHERE account = ? AND key = ? +''', + [account.id, key], + ); } on DatabaseException catch (error, stackTrace) { _log.severe( 'Error getting the cache parameters for `$key` from cache.', @@ -178,16 +206,17 @@ CREATE TABLE "cache" ( } @override - Future updateParameters(String key, CacheParameters? parameters) async { + Future updateParameters(Account account, String key, CacheParameters? parameters) async { try { await _requireDatabase.update( 'cache', { + 'account': account.id, 'etag': parameters?.etag, 'expires': parameters?.expires?.millisecondsSinceEpoch, }, - where: 'key = ?', - whereArgs: [key], + where: 'account = ? AND key = ?', + whereArgs: [account.id, key], ); } on DatabaseException catch (error, stackTrace) { _log.severe( diff --git a/packages/neon_framework/lib/src/utils/request_manager.dart b/packages/neon_framework/lib/src/utils/request_manager.dart index 6178baa093e..a20bd2013d1 100644 --- a/packages/neon_framework/lib/src/utils/request_manager.dart +++ b/packages/neon_framework/lib/src/utils/request_manager.dart @@ -212,8 +212,6 @@ class RequestManager { bool disableTimeout = false, Duration timeLimit = kDefaultTimeout, }) async { - final key = '${account.id}-$cacheKey'; - if (subject.isClosed) { return; } @@ -221,11 +219,11 @@ class RequestManager { subject.add(Result.loading()); } - final cachedParameters = await _cache?.getParameters(key); + final cachedParameters = await _cache?.getParameters(account, cacheKey); if (cachedParameters != null) { if (cachedParameters.expires != null && !cachedParameters.isExpired) { - final cachedValue = await _cache?.get(key); + final cachedValue = await _cache?.get(account, cacheKey); if (cachedValue != null) { if (!subject.isClosed) { subject.add(Result(unwrap(deserialize(cachedValue)), null, isLoading: false, isCached: true)); @@ -255,11 +253,12 @@ class RequestManager { } if (cacheParameters != null && cacheParameters.etag == cachedParameters.etag) { - final cachedValue = await _cache?.get(key); + final cachedValue = await _cache?.get(account, cacheKey); if (cachedValue != null) { unawaited( _cache?.updateParameters( - key, + account, + cacheKey, cacheParameters, ), ); @@ -272,7 +271,7 @@ class RequestManager { } } - final cachedValue = await _cache?.get(key); + final cachedValue = await _cache?.get(account, cacheKey); if (subject.isClosed) { return; } @@ -307,7 +306,7 @@ class RequestManager { cacheParameters = CacheParameters.parseHeaders(rawHeaders); } - await _cache?.set(key, serialized, cacheParameters); + await _cache?.set(account, cacheKey, serialized, cacheParameters); break; } on TimeoutException catch (error, stackTrace) { _log.info( diff --git a/packages/neon_framework/test/persistence_test.dart b/packages/neon_framework/test/persistence_test.dart index 39c245d1439..870d283125e 100644 --- a/packages/neon_framework/test/persistence_test.dart +++ b/packages/neon_framework/test/persistence_test.dart @@ -3,20 +3,25 @@ import 'package:built_collection/built_collection.dart'; import 'package:flutter_test/flutter_test.dart'; +import 'package:mocktail/mocktail.dart'; import 'package:neon_framework/src/storage/request_cache.dart'; import 'package:neon_framework/src/storage/sqlite_persistence.dart'; import 'package:neon_framework/src/utils/request_manager.dart'; +import 'package:neon_framework/testing.dart'; import 'package:sqflite_common_ffi/sqflite_ffi.dart'; import 'package:timezone/timezone.dart' as tz; void main() { group('Persistences', () { test('RequestCache', () async { + final account = MockAccount(); + when(() => account.id).thenReturn('clientID'); + final cache = DefaultRequestCache(); sqfliteFfiInit(); databaseFactory = databaseFactoryFfi; - expect(() async => cache.get('key'), throwsA(isA())); + expect(() async => cache.get(account, 'key'), throwsA(isA())); cache.database = await openDatabase( inMemoryDatabasePath, @@ -24,23 +29,23 @@ void main() { onCreate: DefaultRequestCache.onCreate, ); - dynamic result = await cache.get('key'); + dynamic result = await cache.get(account, 'key'); expect(result, isNull); - await cache.set('key', 'value', null); - result = await cache.get('key'); + await cache.set(account, 'key', 'value', null); + result = await cache.get(account, 'key'); expect(result, equals('value')); - await cache.set('key', 'upsert', null); - result = await cache.get('key'); + await cache.set(account, 'key', 'upsert', null); + result = await cache.get(account, 'key'); expect(result, equals('upsert')); var parameters = const CacheParameters(etag: null, expires: null); - result = await cache.getParameters('newKey'); + result = await cache.getParameters(account, 'newKey'); expect(result, equals(parameters)); - await cache.set('key', 'value', parameters); - result = await cache.getParameters('key'); + await cache.set(account, 'key', 'value', parameters); + result = await cache.getParameters(account, 'key'); expect(result, equals(parameters)); parameters = CacheParameters( @@ -50,8 +55,8 @@ void main() { tz.TZDateTime.now(tz.UTC).millisecondsSinceEpoch, ), ); - await cache.updateParameters('key', parameters); - result = await cache.getParameters('key'); + await cache.updateParameters(account, 'key', parameters); + result = await cache.getParameters(account, 'key'); expect(result, equals(parameters)); }); diff --git a/packages/neon_framework/test/request_manager_test.dart b/packages/neon_framework/test/request_manager_test.dart index 13dd91c2c75..c78a258e4f1 100644 --- a/packages/neon_framework/test/request_manager_test.dart +++ b/packages/neon_framework/test/request_manager_test.dart @@ -271,22 +271,26 @@ void main() { group('wrap with cache', () { late MockRequestCache cache; + setUpAll(() { + registerFallbackValue(account); + }); + setUp(() async { cache = MockRequestCache(); - when(() => cache.get(any())).thenAnswer( + when(() => cache.get(any(), any())).thenAnswer( (_) => Future.value('Cached value'), ); - when(() => cache.set(any(), any(), any())).thenAnswer( + when(() => cache.set(any(), any(), any(), any())).thenAnswer( (_) => Future.value(), ); - when(() => cache.getParameters(any())).thenAnswer( + when(() => cache.getParameters(any(), any())).thenAnswer( (_) => Future.value(const CacheParameters(etag: null, expires: null)), ); - when(() => cache.updateParameters(any(), any())).thenAnswer( + when(() => cache.updateParameters(any(), any(), any())).thenAnswer( (_) => Future.value(), ); @@ -318,8 +322,8 @@ void main() { ); await subject.close(); - verify(() => cache.get('clientID-key')).called(1); - verify(() => cache.set('clientID-key', 'Test value', null)).called(1); + verify(() => cache.get(account, 'key')).called(1); + verify(() => cache.set(account, 'key', 'Test value', null)).called(1); subject = BehaviorSubject>.seeded(Result.success('Seed value')); @@ -345,8 +349,8 @@ void main() { ); await subject.close(); - verify(() => cache.get('clientID-key')).called(1); - verify(() => cache.set('clientID-key', 'Test value', null)).called(1); + verify(() => cache.get(account, 'key')).called(1); + verify(() => cache.set(account, 'key', 'Test value', null)).called(1); }); test('timeout request', () async { @@ -391,8 +395,8 @@ void main() { ); await subject.close(); - verify(() => cache.get('clientID-key')).called(1); - verifyNever(() => cache.set(any(), any(), any())); + verify(() => cache.get(account, 'key')).called(1); + verifyNever(() => cache.set(any(), any(), any(), any())); subject = BehaviorSubject>.seeded(Result.success('Seed value')); @@ -435,8 +439,8 @@ void main() { ); await subject.close(); - verify(() => cache.get('clientID-key')).called(1); - verifyNever(() => cache.set(any(), any(), any())); + verify(() => cache.get(account, 'key')).called(1); + verifyNever(() => cache.set(any(), any(), any(), any())); }); test('throwing request', () async { @@ -464,8 +468,8 @@ void main() { ); await subject.close(); - verify(() => cache.get('clientID-key')).called(1); - verifyNever(() => cache.set(any(), any(), any())); + verify(() => cache.get(account, 'key')).called(1); + verifyNever(() => cache.set(any(), any(), any(), any())); subject = BehaviorSubject>.seeded(Result.success('Seed value')); @@ -491,12 +495,12 @@ void main() { ); await subject.close(); - verify(() => cache.get('clientID-key')).called(1); - verifyNever(() => cache.set(any(), any(), any())); + verify(() => cache.get(account, 'key')).called(1); + verifyNever(() => cache.set(any(), any(), any(), any())); }); test('cached Expires', () async { - when(() => cache.getParameters(any())).thenAnswer( + when(() => cache.getParameters(any(), any())).thenAnswer( (_) => Future.value( CacheParameters( etag: null, @@ -504,7 +508,7 @@ void main() { ), ), ); - when(() => cache.get(any())).thenAnswer( + when(() => cache.get(any(), any())).thenAnswer( (_) => Future.value('Cached value'), ); @@ -531,10 +535,10 @@ void main() { ); await subject.close(); - verify(() => cache.get('clientID-key')).called(1); - verifyNever(() => cache.set(any(), any(), any())); + verify(() => cache.get(account, 'key')).called(1); + verifyNever(() => cache.set(any(), any(), any(), any())); - when(() => cache.getParameters(any())).thenAnswer( + when(() => cache.getParameters(any(), any())).thenAnswer( (_) => Future.value( CacheParameters( etag: null, @@ -542,7 +546,7 @@ void main() { ), ), ); - when(() => cache.get(any())).thenAnswer( + when(() => cache.get(any(), any())).thenAnswer( (_) => Future.value('Cached value'), ); @@ -570,8 +574,8 @@ void main() { ); await subject.close(); - verify(() => cache.get('clientID-key')).called(1); - verify(() => cache.set(any(), any(), any())).called(1); + verify(() => cache.get(account, 'key')).called(1); + verify(() => cache.set(any(), any(), any(), any())).called(1); }); test('cached ETag', () async { @@ -581,7 +585,7 @@ void main() { tz.UTC, ); - when(() => cache.getParameters(any())).thenAnswer( + when(() => cache.getParameters(any(), any())).thenAnswer( (_) => Future.value( const CacheParameters( etag: 'a', @@ -589,7 +593,7 @@ void main() { ), ), ); - when(() => cache.get(any())).thenAnswer( + when(() => cache.get(any(), any())).thenAnswer( (_) => Future.value('Cached value'), ); when(callback.call).thenAnswer( @@ -623,12 +627,12 @@ void main() { ); await subject.close(); - verify(() => cache.get('clientID-key')).called(1); + verify(() => cache.get(account, 'key')).called(1); verify(callback.call).called(1); - verify(() => cache.updateParameters('clientID-key', CacheParameters(etag: 'a', expires: newExpires))).called(1); - verifyNever(() => cache.set(any(), any(), any())); + verify(() => cache.updateParameters(account, 'key', CacheParameters(etag: 'a', expires: newExpires))).called(1); + verifyNever(() => cache.set(any(), any(), any(), any())); - when(() => cache.getParameters(any())).thenAnswer( + when(() => cache.getParameters(any(), any())).thenAnswer( (_) => Future.value( const CacheParameters( etag: 'a', @@ -636,7 +640,7 @@ void main() { ), ), ); - when(() => cache.get(any())).thenAnswer( + when(() => cache.get(any(), any())).thenAnswer( (_) => Future.value('Cached value'), ); when(callback.call).thenAnswer( @@ -671,10 +675,10 @@ void main() { ); await subject.close(); - verify(() => cache.get('clientID-key')).called(1); + verify(() => cache.get(account, 'key')).called(1); verify(callback.call).called(1); - verify(() => cache.set('clientID-key', 'Test value', null)); - verifyNever(() => cache.updateParameters(any(), any())); + verify(() => cache.set(account, 'key', 'Test value', null)); + verifyNever(() => cache.updateParameters(any(), any(), any())); }); test('cache ETag and Expires', () async { @@ -730,10 +734,11 @@ void main() { ); await subject.close(); - verify(() => cache.get('clientID-key')).called(1); + verify(() => cache.get(account, 'key')).called(1); verify( () => cache.set( - 'clientID-key', + account, + 'key', 'Test value', CacheParameters(etag: 'a', expires: isSet ? newExpires : null), ),