From 7023924f92a40e29c0cc9c8ddae91d8cd7f74d21 Mon Sep 17 00:00:00 2001 From: Jordan Nelson Date: Tue, 23 Jul 2024 15:04:02 -0400 Subject: [PATCH 1/6] fix(secure_storage): process fs events in the order they are received --- .../lib/src/utils/file_key_value_store.dart | 67 ++++++++++++++++--- .../test/file_key_value_store_test.dart | 31 ++++++++- 2 files changed, 85 insertions(+), 13 deletions(-) diff --git a/packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart b/packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart index 6ce470b42f..63401f569a 100644 --- a/packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart +++ b/packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart @@ -1,6 +1,7 @@ // Copyright Amazon.com, Inc. or its affiliates. All Rights Reserved. // SPDX-License-Identifier: Apache-2.0 +import 'dart:async'; import 'dart:convert'; import 'dart:io'; @@ -18,7 +19,7 @@ import 'package:path/path.dart' as pkg_path; // without bringing in flutter as a dependency to the tests. class FileKeyValueStore { /// {@macro amplify_secure_storage_dart.file_key_value_store} - const FileKeyValueStore({ + FileKeyValueStore({ required this.path, required this.fileName, this.fs = const local_file.LocalFileSystem(), @@ -32,6 +33,8 @@ class FileKeyValueStore { /// The file will be created if it does not yet exist. final String fileName; + final TaskScheduler _scheduler = TaskScheduler(); + @visibleForTesting final pkg_file.FileSystem fs; @@ -47,9 +50,11 @@ class FileKeyValueStore { required String key, required Object value, }) async { - final data = await readAll(); - data[key] = value; - return writeAll(data); + return _scheduler.schedule(() async { + final data = await readAll(); + data[key] = value; + return writeAll(data); + }); } /// Overwrites the existing data. @@ -67,25 +72,31 @@ class FileKeyValueStore { Future readKey({ required String key, }) async { - final data = await readAll(); - return data[key]; + return _scheduler.schedule(() async { + final data = await readAll(); + return data[key]; + }); } /// Removes a single key from storage. Future removeKey({ required String key, }) async { - final data = await readAll(); - data.remove(key); - await writeAll(data); + return _scheduler.schedule(() async { + final data = await readAll(); + data.remove(key); + await writeAll(data); + }); } /// Returns true if the key exists in storage Future containsKey({ required String key, }) async { - final data = await readAll(); - return data.containsKey(key); + return _scheduler.schedule(() async { + final data = await readAll(); + return data.containsKey(key); + }); } /// Reads all the key-value pairs from storage. @@ -102,3 +113,37 @@ class FileKeyValueStore { return {}; } } + +/// A class for processing async tasks one at a time in the order that they are +/// scheduled. +class TaskScheduler { + final _taskQueue = >[]; + bool _isProcessing = false; + Future schedule(Future Function() task) { + final completer = Completer(); + _taskQueue.add(Task(task, completer)); + _processTasks(); + return completer.future; + } + + Future _processTasks() async { + if (_isProcessing) return; + _isProcessing = true; + while (_taskQueue.isNotEmpty) { + final currentTask = _taskQueue.removeAt(0); + try { + final result = await currentTask.task(); + currentTask.completer.complete(result); + } on Object catch (e, stackTrace) { + currentTask.completer.completeError(e, stackTrace); + } + } + _isProcessing = false; + } +} + +class Task { + Task(this.task, this.completer); + final Future Function() task; + final Completer completer; +} diff --git a/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart b/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart index 8fa16d436c..bf03deac1a 100644 --- a/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart +++ b/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart @@ -4,7 +4,6 @@ @TestOn('vm') import 'package:amplify_secure_storage_dart/src/utils/file_key_value_store.dart'; -import 'package:file/memory.dart'; import 'package:test/test.dart'; void main() { @@ -14,10 +13,13 @@ void main() { storage = FileKeyValueStore( path: 'path', fileName: 'file', - fs: MemoryFileSystem(), ); }); + tearDown(() async { + await storage.file.delete(); + }); + test('readKey & writeKey', () async { // assert initial state is null final value1 = await storage.readKey(key: 'key'); @@ -81,5 +83,30 @@ void main() { final includesKey2 = await storage.containsKey(key: 'key2'); expect(includesKey2, isFalse); }); + + test('parallel writes should occur in the order they are called', () async { + final items = List.generate(1000, ((i) => i)); + final futures = items.map( + (i) async => storage.writeKey(key: 'key', value: i), + ); + await Future.wait(futures); + final value = await storage.readKey(key: 'key'); + expect(value, items.last); + }); + + // Reference: https://github.com/aws-amplify/amplify-flutter/issues/5190 + test('parallel write/remove operations should not corrupt the file', + () async { + final items = List.generate(1000, ((i) => i)); + final futures = items.map( + (i) async { + if (i % 5 == 1) { + await storage.removeKey(key: 'key_${i - 1}'); + } + return storage.writeKey(key: 'key_$i', value: 'value_$i'); + }, + ); + await Future.wait(futures); + }); }); } From 9b0c9fc8b796370eb97a075f9dfa7bc5c266055e Mon Sep 17 00:00:00 2001 From: Jordan Nelson Date: Tue, 23 Jul 2024 15:56:08 -0400 Subject: [PATCH 2/6] chore: clear file when it is corrupted --- .../lib/src/utils/file_key_value_store.dart | 18 +++++++++++++++--- .../test/file_key_value_store_test.dart | 14 ++++++++++++++ 2 files changed, 29 insertions(+), 3 deletions(-) diff --git a/packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart b/packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart index 63401f569a..acd4187aa2 100644 --- a/packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart +++ b/packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart @@ -5,6 +5,7 @@ import 'dart:async'; import 'dart:convert'; import 'dart:io'; +import 'package:aws_common/aws_common.dart'; import 'package:file/file.dart' as pkg_file; import 'package:file/local.dart' as local_file; import 'package:meta/meta.dart'; @@ -35,6 +36,8 @@ class FileKeyValueStore { final TaskScheduler _scheduler = TaskScheduler(); + final AWSLogger logger = AWSLogger('FileKeyValueStore'); + @visibleForTesting final pkg_file.FileSystem fs; @@ -104,9 +107,18 @@ class FileKeyValueStore { if (await file.exists()) { final stringMap = await file.readAsString(); if (stringMap.isNotEmpty) { - final Object? data = json.decode(stringMap); - if (data is Map) { - return data.cast(); + try { + final Object? data = json.decode(stringMap); + if (data is Map) { + return data.cast(); + } + } on FormatException catch (e) { + logger.error( + 'Cannot read file. The file may be corrupted. ' + 'Clearing file contents. See error for more details. ' + 'Error: $e', + ); + await writeAll({}); } } } diff --git a/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart b/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart index bf03deac1a..b9a38bc982 100644 --- a/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart +++ b/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart @@ -3,6 +3,8 @@ @TestOn('vm') +import 'dart:io'; + import 'package:amplify_secure_storage_dart/src/utils/file_key_value_store.dart'; import 'package:test/test.dart'; @@ -108,5 +110,17 @@ void main() { ); await Future.wait(futures); }); + + test('File is cleared when corrupted and can be re-written to', () async { + await storage.writeKey(key: 'foo', value: 'value'); + final value1 = await storage.readKey(key: 'foo'); + expect(value1, 'value'); + await storage.file.writeAsString('{invalid json}', mode: FileMode.append); + final value2 = await storage.readKey(key: 'foo'); + expect(value2, null); + await storage.writeKey(key: 'foo', value: 'value'); + final value3 = await storage.readKey(key: 'foo'); + expect(value3, 'value'); + }); }); } From 55a64b4f38c5b7df512c0bd13767fb6c54370811 Mon Sep 17 00:00:00 2001 From: Jordan Nelson Date: Wed, 24 Jul 2024 11:50:42 -0400 Subject: [PATCH 3/6] chore: add test for stale data during parallel writes --- .../test/file_key_value_store_test.dart | 54 +++++++++++-------- 1 file changed, 33 insertions(+), 21 deletions(-) diff --git a/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart b/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart index b9a38bc982..7d6e7e2bd4 100644 --- a/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart +++ b/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart @@ -86,29 +86,41 @@ void main() { expect(includesKey2, isFalse); }); - test('parallel writes should occur in the order they are called', () async { + group('parallel operations', () { final items = List.generate(1000, ((i) => i)); - final futures = items.map( - (i) async => storage.writeKey(key: 'key', value: i), - ); - await Future.wait(futures); - final value = await storage.readKey(key: 'key'); - expect(value, items.last); - }); - // Reference: https://github.com/aws-amplify/amplify-flutter/issues/5190 - test('parallel write/remove operations should not corrupt the file', - () async { - final items = List.generate(1000, ((i) => i)); - final futures = items.map( - (i) async { - if (i % 5 == 1) { - await storage.removeKey(key: 'key_${i - 1}'); - } - return storage.writeKey(key: 'key_$i', value: 'value_$i'); - }, - ); - await Future.wait(futures); + test('should occur in the order they are called', () async { + final futures = items.map( + (i) async => storage.writeKey(key: 'key', value: i), + ); + await Future.wait(futures); + final value = await storage.readKey(key: 'key'); + expect(value, items.last); + }); + + test('should not result in stale data written to the file', () async { + final futures = items.map( + (i) async => storage.writeKey(key: 'key_$i', value: i), + ); + await Future.wait(futures); + for (final i in items) { + final value = await storage.readKey(key: 'key_$i'); + expect(value, items[i]); + } + }); + + // Reference: https://github.com/aws-amplify/amplify-flutter/issues/5190 + test('should not corrupt the file', () async { + final futures = items.map( + (i) async { + if (i % 5 == 1) { + await storage.removeKey(key: 'key_${i - 1}'); + } + return storage.writeKey(key: 'key_$i', value: 'value_$i'); + }, + ); + await Future.wait(futures); + }); }); test('File is cleared when corrupted and can be re-written to', () async { From e4ceef4183cc428f9164075423424d0e94e0900a Mon Sep 17 00:00:00 2001 From: Jordan Nelson Date: Wed, 24 Jul 2024 11:54:19 -0400 Subject: [PATCH 4/6] chore: add visibleForTesting annotations --- .../lib/src/utils/file_key_value_store.dart | 71 ++++++++++--------- 1 file changed, 37 insertions(+), 34 deletions(-) diff --git a/packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart b/packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart index acd4187aa2..b87b9b1f32 100644 --- a/packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart +++ b/packages/secure_storage/amplify_secure_storage_dart/lib/src/utils/file_key_value_store.dart @@ -41,6 +41,7 @@ class FileKeyValueStore { @visibleForTesting final pkg_file.FileSystem fs; + @visibleForTesting File get file => fs.file( pkg_path.join( path, @@ -48,6 +49,42 @@ class FileKeyValueStore { ), ); + /// Overwrites the existing data in [file] with the key-value pairs in [data]. + @visibleForTesting + Future writeAll( + Map data, + ) async { + if (!await file.exists()) { + await file.create(recursive: true); + } + final stringMap = json.encode(data); + await file.writeAsString(stringMap); + } + + /// Reads all the key-value pairs from [file]. + @visibleForTesting + Future> readAll() async { + if (await file.exists()) { + final stringMap = await file.readAsString(); + if (stringMap.isNotEmpty) { + try { + final Object? data = json.decode(stringMap); + if (data is Map) { + return data.cast(); + } + } on FormatException catch (e) { + logger.error( + 'Cannot read file. The file may be corrupted. ' + 'Clearing file contents. See error for more details. ' + 'Error: $e', + ); + await writeAll({}); + } + } + } + return {}; + } + /// Writes a single key to storage. Future writeKey({ required String key, @@ -60,17 +97,6 @@ class FileKeyValueStore { }); } - /// Overwrites the existing data. - Future writeAll( - Map data, - ) async { - if (!await file.exists()) { - await file.create(recursive: true); - } - final stringMap = json.encode(data); - await file.writeAsString(stringMap); - } - /// Reads a single key from storage. Future readKey({ required String key, @@ -101,29 +127,6 @@ class FileKeyValueStore { return data.containsKey(key); }); } - - /// Reads all the key-value pairs from storage. - Future> readAll() async { - if (await file.exists()) { - final stringMap = await file.readAsString(); - if (stringMap.isNotEmpty) { - try { - final Object? data = json.decode(stringMap); - if (data is Map) { - return data.cast(); - } - } on FormatException catch (e) { - logger.error( - 'Cannot read file. The file may be corrupted. ' - 'Clearing file contents. See error for more details. ' - 'Error: $e', - ); - await writeAll({}); - } - } - } - return {}; - } } /// A class for processing async tasks one at a time in the order that they are From 2fcc798036a1ea5780e53e2bf2765648352405fa Mon Sep 17 00:00:00 2001 From: Jordan Nelson Date: Wed, 24 Jul 2024 12:05:56 -0400 Subject: [PATCH 5/6] chore: add tests for in-memory fs and local fs --- .../test/file_key_value_store_test.dart | 217 +++++++++--------- 1 file changed, 114 insertions(+), 103 deletions(-) diff --git a/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart b/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart index 7d6e7e2bd4..7d12a48616 100644 --- a/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart +++ b/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart @@ -6,133 +6,144 @@ import 'dart:io'; import 'package:amplify_secure_storage_dart/src/utils/file_key_value_store.dart'; +import 'package:aws_common/aws_common.dart'; +import 'package:file/local.dart'; +import 'package:file/memory.dart'; import 'package:test/test.dart'; void main() { late FileKeyValueStore storage; - group('FileKeyValueStore', () { - setUp(() { - storage = FileKeyValueStore( - path: 'path', - fileName: 'file', - ); - }); - - tearDown(() async { - await storage.file.delete(); - }); - test('readKey & writeKey', () async { - // assert initial state is null - final value1 = await storage.readKey(key: 'key'); - expect(value1, isNull); + final fileSystems = [MemoryFileSystem(), const LocalFileSystem()]; - // write key-value pair - await storage.writeKey(key: 'key', value: 'value'); + for (final fileSystem in fileSystems) { + group('FileKeyValueStore (${fileSystem.runtimeType})', () { + safePrint(fileSystem.runtimeType); + setUp(() { + storage = FileKeyValueStore( + path: 'path', + fileName: 'file', + fs: fileSystem, + ); + }); - // assert value is updated - final value2 = await storage.readKey(key: 'key'); - expect(value2, 'value'); - }); + tearDown(() async { + await storage.file.delete(); + }); - test('removeKey', () async { - // seed storage & assert value is present - await storage.writeKey(key: 'key', value: 'value'); - final value1 = await storage.readKey(key: 'key'); - expect(value1, 'value'); + test('readKey & writeKey', () async { + // assert initial state is null + final value1 = await storage.readKey(key: 'key'); + expect(value1, isNull); - // remove key - await storage.removeKey(key: 'key'); + // write key-value pair + await storage.writeKey(key: 'key', value: 'value'); - // assert key is removed - final value2 = await storage.readKey(key: 'key'); - expect(value2, isNull); - }); + // assert value is updated + final value2 = await storage.readKey(key: 'key'); + expect(value2, 'value'); + }); - test('readAll', () async { - // write key-value pairs - await storage.writeKey(key: 'key1', value: 'value1'); - await storage.writeKey(key: 'key2', value: 'value2'); + test('removeKey', () async { + // seed storage & assert value is present + await storage.writeKey(key: 'key', value: 'value'); + final value1 = await storage.readKey(key: 'key'); + expect(value1, 'value'); - // assert values are updated - final data = await storage.readAll(); - expect(data['key1'], 'value1'); - expect(data['key2'], 'value2'); - }); + // remove key + await storage.removeKey(key: 'key'); - test('writeAll', () async { - // write key-value pairs - await storage.writeAll({ - 'key1': 'value1', - 'key2': 'value2', + // assert key is removed + final value2 = await storage.readKey(key: 'key'); + expect(value2, isNull); }); - // assert values are updated - final data = await storage.readAll(); - expect(data['key1'], 'value1'); - expect(data['key2'], 'value2'); - }); + test('readAll', () async { + // write key-value pairs + await storage.writeKey(key: 'key1', value: 'value1'); + await storage.writeKey(key: 'key2', value: 'value2'); - test('includes', () async { - // write key-value pair - await storage.writeKey(key: 'key1', value: 'value1'); + // assert values are updated + final data = await storage.readAll(); + expect(data['key1'], 'value1'); + expect(data['key2'], 'value2'); + }); - // assert that existing key returns true - final includesKey1 = await storage.containsKey(key: 'key1'); - expect(includesKey1, isTrue); + test('writeAll', () async { + // write key-value pairs + await storage.writeAll({ + 'key1': 'value1', + 'key2': 'value2', + }); + + // assert values are updated + final data = await storage.readAll(); + expect(data['key1'], 'value1'); + expect(data['key2'], 'value2'); + }); - // assert that a non existing key returns false - final includesKey2 = await storage.containsKey(key: 'key2'); - expect(includesKey2, isFalse); - }); + test('includes', () async { + // write key-value pair + await storage.writeKey(key: 'key1', value: 'value1'); - group('parallel operations', () { - final items = List.generate(1000, ((i) => i)); + // assert that existing key returns true + final includesKey1 = await storage.containsKey(key: 'key1'); + expect(includesKey1, isTrue); - test('should occur in the order they are called', () async { - final futures = items.map( - (i) async => storage.writeKey(key: 'key', value: i), - ); - await Future.wait(futures); - final value = await storage.readKey(key: 'key'); - expect(value, items.last); + // assert that a non existing key returns false + final includesKey2 = await storage.containsKey(key: 'key2'); + expect(includesKey2, isFalse); }); - test('should not result in stale data written to the file', () async { - final futures = items.map( - (i) async => storage.writeKey(key: 'key_$i', value: i), - ); - await Future.wait(futures); - for (final i in items) { - final value = await storage.readKey(key: 'key_$i'); - expect(value, items[i]); - } + group('parallel operations', () { + final items = List.generate(1000, ((i) => i)); + + test('should occur in the order they are called', () async { + final futures = items.map( + (i) async => storage.writeKey(key: 'key', value: i), + ); + await Future.wait(futures); + final value = await storage.readKey(key: 'key'); + expect(value, items.last); + }); + + test('should not result in stale data written to the file', () async { + final futures = items.map( + (i) async => storage.writeKey(key: 'key_$i', value: i), + ); + await Future.wait(futures); + for (final i in items) { + final value = await storage.readKey(key: 'key_$i'); + expect(value, items[i]); + } + }); + + // Reference: https://github.com/aws-amplify/amplify-flutter/issues/5190 + test('should not corrupt the file', () async { + final futures = items.map( + (i) async { + if (i % 5 == 1) { + await storage.removeKey(key: 'key_${i - 1}'); + } + return storage.writeKey(key: 'key_$i', value: 'value_$i'); + }, + ); + await Future.wait(futures); + }); }); - // Reference: https://github.com/aws-amplify/amplify-flutter/issues/5190 - test('should not corrupt the file', () async { - final futures = items.map( - (i) async { - if (i % 5 == 1) { - await storage.removeKey(key: 'key_${i - 1}'); - } - return storage.writeKey(key: 'key_$i', value: 'value_$i'); - }, - ); - await Future.wait(futures); + test('File is cleared when corrupted and can be re-written to', () async { + await storage.writeKey(key: 'foo', value: 'value'); + final value1 = await storage.readKey(key: 'foo'); + expect(value1, 'value'); + await storage.file + .writeAsString('{invalid json}', mode: FileMode.append); + final value2 = await storage.readKey(key: 'foo'); + expect(value2, null); + await storage.writeKey(key: 'foo', value: 'value'); + final value3 = await storage.readKey(key: 'foo'); + expect(value3, 'value'); }); }); - - test('File is cleared when corrupted and can be re-written to', () async { - await storage.writeKey(key: 'foo', value: 'value'); - final value1 = await storage.readKey(key: 'foo'); - expect(value1, 'value'); - await storage.file.writeAsString('{invalid json}', mode: FileMode.append); - final value2 = await storage.readKey(key: 'foo'); - expect(value2, null); - await storage.writeKey(key: 'foo', value: 'value'); - final value3 = await storage.readKey(key: 'foo'); - expect(value3, 'value'); - }); - }); + } } From 30c8a1b7e5d037d97affd0573ee16cbebba4ef68 Mon Sep 17 00:00:00 2001 From: Jordan Nelson Date: Wed, 24 Jul 2024 12:07:20 -0400 Subject: [PATCH 6/6] chore: remove print from test --- .../test/file_key_value_store_test.dart | 2 -- 1 file changed, 2 deletions(-) diff --git a/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart b/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart index 7d12a48616..5f75aa4c37 100644 --- a/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart +++ b/packages/secure_storage/amplify_secure_storage_test/test/file_key_value_store_test.dart @@ -6,7 +6,6 @@ import 'dart:io'; import 'package:amplify_secure_storage_dart/src/utils/file_key_value_store.dart'; -import 'package:aws_common/aws_common.dart'; import 'package:file/local.dart'; import 'package:file/memory.dart'; import 'package:test/test.dart'; @@ -18,7 +17,6 @@ void main() { for (final fileSystem in fileSystems) { group('FileKeyValueStore (${fileSystem.runtimeType})', () { - safePrint(fileSystem.runtimeType); setUp(() { storage = FileKeyValueStore( path: 'path',