refactor(neon_framework): add account column to RequestCache

Signed-off-by: Nikolas Rimikis <leptopoda@users.noreply.github.com>
nextcloud · Mar 15, 2024 · 742837b · 742837b
1 parent f1a1598
commit 742837b
Show file tree

Hide file tree

Showing 4 changed files with 122 additions and 84 deletions.
diff --git 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<String?> get(String key);
+  Future<String?> 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<void> set(String key, String value, CacheParameters? parameters);
+  Future<void> set(Account account, String key, String value, CacheParameters? parameters);
 
   /// Retrieves the cache parameters for the given [key].
-  Future<CacheParameters> getParameters(String key);
+  Future<CacheParameters> getParameters(Account account, String key);
 
   /// Updates the cache [parameters] for a given [key] without modifying the `value`.
-  Future<void> updateParameters(String key, CacheParameters? parameters);
+  Future<void> 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<void> onCreate(Database db, int version) async {
+  static Future<void> 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<String?> get(String key) async {
+  Future<String?> get(Account account, String key) async {
     List<Map<String, Object?>>? 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,25 +130,36 @@ CREATE TABLE "cache" (
   }
 
   @override
-  Future<void> set(String key, String value, CacheParameters? parameters) async {
+  Future<void> 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
       final batch = _requireDatabase.batch()
         ..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<CacheParameters> getParameters(String key) async {
+  Future<CacheParameters> getParameters(Account account, String key) async {
     List<Map<String, Object?>>? 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<void> updateParameters(String key, CacheParameters? parameters) async {
+  Future<void> 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
@@ -212,20 +212,18 @@ class RequestManager {
     bool disableTimeout = false,
     Duration timeLimit = kDefaultTimeout,
   }) async {
-    final key = '${account.id}-$cacheKey';
-
     if (subject.isClosed) {
       return;
     }
     if (!subject.hasValue) {
       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
@@ -3,44 +3,49 @@
 
 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<StateError>()));
+      expect(() async => cache.get(account, 'key'), throwsA(isA<StateError>()));
 
       cache.database = await openDatabase(
         inMemoryDatabasePath,
         version: 1,
         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));
     });