fix(logging): use indexedDB on web if it is supported #3961

NikaHsn · 2023-10-16T18:36:10Z

Issue #, if available:

Description of changes:

indexedDb adapter to wait for the test DB open result when checking if indexedDB is supported.
DartQueuedItemStore web implementation to wait for indexedDb adapter and fall back to in memory if indexedDb is not supported
indexedDb adapter getAll method to read result as Object instead of Map

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

fjnoyp · 2023-10-17T14:56:17Z

...udwatch/amplify_logging_cloudwatch/lib/src/queued_item_store/dart_queued_item_store.web.dart

@@ -15,18 +17,23 @@ class DartQueuedItemStore
    implements QueuedItemStore, Closeable {
  /// {@macro amplify_logging_cloudwatch.index_db_queued_item_store}
  // ignore: avoid_unused_constructor_parameters
-  DartQueuedItemStore(String? storagePath);
+  DartQueuedItemStore(String? storagePath) {


This should be an async factory method? It seems introduces race conditions if you call the constructor and then call one of the async instance methods? The instance methods do not check if _init() was completed.

it should not cause race conditions because Dart event loop handles microtasks queue first and then events queue.
to do it async and wait for _database to init means all instance methods needs to be async. currently the isFull method is a sync method. can make all instance method async and wait for _database if that's the right way to do it.

Thanks that makes a lot more sense then. We haven't really used microtasks in the code before and I'm unfamiliar with them but that makes sense.

But yes, the async Future methods will be good but sync method isFull will likely not work.

make sense. I will update the code to make the _database be a Future and the isFull to be async.

fjnoyp · 2023-10-17T14:57:16Z

...dwatch/amplify_logging_cloudwatch/lib/src/queued_item_store/index_db/indexed_db_adapter.dart

-      final id = value['id'] as int;
-      final itemValue = value['value'] as String;
-      final timestamp = value['timestamp'] as String;
+      final value = elem as Object;


This code appears to be a duplicate of getCount - can we please refactor the code to avoid duplication?

fjnoyp · 2023-10-17T14:57:42Z

...dwatch/amplify_logging_cloudwatch/lib/src/queued_item_store/index_db/indexed_db_adapter.dart

-      final id = value['id'] as int;
-      final itemValue = value['value'] as String;
-      final timestamp = value['timestamp'] as String;
+      final value = elem as Object;


Why are we treating value as Object now? We should prefer Dart's typing safety instead of casting objects to Object or dynamic.

not casting as Object throws exception.

What exception do you get though? Casting to the wrong type will get an exception but doesn't mean we should cast to Object.

it was casting to Map<String, dynamic> and the exception was something like javascript object is not Map ....
updated the code to call getCount method to reuse the implementation as you suggested.

Yeah it's tricky with JSInterop. I think you need to call
JSON.decode( method to convert from a javascript object to a map. Better would be to use codegen described here: #3961 (comment)

fjnoyp · 2023-10-17T14:58:32Z

...dwatch/amplify_logging_cloudwatch/lib/src/queued_item_store/index_db/indexed_db_adapter.dart

-      final timestamp = value['timestamp'] as String;
+      final value = elem as Object;
+      final id = getProperty<int>(value, 'id');
+      final string = getProperty<String>(value, 'value');


Let's avoid naming variables as data types. String and string can be used interchangeably in some languages making this more confusing. Why did we rename from itemValue?

fjnoyp · 2023-10-17T14:59:04Z

...dwatch/amplify_logging_cloudwatch/lib/src/queued_item_store/index_db/indexed_db_adapter.dart

    if (indexedDB == null) {
      return false;
    }
    // indexedDB will be non-null in Firefox private browsing,
    // but will fail to open.
    try {
-      indexedDB!.open('test', 1).result;
+      final openRequest = indexedDB!.open('test', 1);


Shouldn't we just await indexedDB!.open('test', 1); instead of making 2 lines?

fjnoyp · 2023-10-17T15:16:40Z

@NikaHsn looking through related code, I think we have a similar problem in: dart_queued_item_store.vm.dart

In the below code we appear to also be 'hiding' the async initialization of DartQueuedItemStore. We have async code in the constructor that we do not await. If the call to getApplicationSupportDirectory returns a future, we then call the constructor of DriftQueuedItemStore with a future object which could cause an exception/crash.

class DartQueuedItemStore implements QueuedItemStore, Closeable {
  /// {@macro amplify_logging_cloudwatch.dart_queued_item_store}
  factory DartQueuedItemStore(String? storagePath) {
    final FutureOr<String> path;
    if (storagePath == null) {
      path = getApplicationSupportDirectory().then((value) => value.path);
    } else {
      path = storagePath;
    }
    final database = DriftQueuedItemStore(path);
    return DartQueuedItemStore._(database);
  }

NikaHsn · 2023-10-17T18:53:43Z

@NikaHsn looking through related code, I think we have a similar problem in: dart_queued_item_store.vm.dart

In the below code we appear to also be 'hiding' the async initialization of DartQueuedItemStore. We have async code in the constructor that we do not await. If the call to getApplicationSupportDirectory returns a future, we then call the constructor of DriftQueuedItemStore with a future object which could cause an exception/crash.
class DartQueuedItemStore implements QueuedItemStore, Closeable {
  /// {@macro amplify_logging_cloudwatch.dart_queued_item_store}
  factory DartQueuedItemStore(String? storagePath) {
    final FutureOr<String> path;
    if (storagePath == null) {
      path = getApplicationSupportDirectory().then((value) => value.path);
    } else {
      path = storagePath;
    }
    final database = DriftQueuedItemStore(path);
    return DartQueuedItemStore._(database);
  }

factory DriftQueuedItemStore(FutureOr<String> storagePath) : DriftQueuedItemStore accepts Future as input parameter. so no need to wait for getApplicationSupportDirectory. packages/common/amplify_db_common_dart/lib/src/connect_io.dart wait for async call to get path if it is a Future.

fjnoyp · 2023-10-17T22:27:30Z

...dwatch/amplify_logging_cloudwatch/lib/src/queued_item_store/index_db/indexed_db_adapter.dart

-    }
-
-    return readValues;
+    return getCount();


Why do we have 2 separate methods if they do the same exact thing?

this is the case only for IDBObjectStore. other implementations of QueuedItemStore have different impl for getCount and getAll.

Ah ok thanks for clarifying

fjnoyp · 2023-10-17T22:32:26Z

Regarding the as Object cast - there's a better way to do this using the JSONSerializable package instead.

See the github_jobs.dart file for an example of how to do it. In the future we should refactor code to use that.

fjnoyp

Looks good thanks for addressing those comments!

NikaHsn requested a review from a team as a code owner October 16, 2023 18:36

khatruong2009 approved these changes Oct 16, 2023

View reviewed changes

Equartey approved these changes Oct 17, 2023

View reviewed changes

fjnoyp suggested changes Oct 17, 2023

View reviewed changes

NikaHsn force-pushed the fix/logging/indexdb branch from 6e0d073 to dc4ae8a Compare October 17, 2023 18:07

fix(logging): use indexedDB on web if it is supported

b79e068

NikaHsn force-pushed the fix/logging/indexdb branch from dc4ae8a to b79e068 Compare October 17, 2023 21:22

fjnoyp reviewed Oct 17, 2023

View reviewed changes

fjnoyp approved these changes Oct 17, 2023

View reviewed changes

NikaHsn merged commit c4467c0 into aws-amplify:feat/logging/cloudwatch Oct 18, 2023
119 of 152 checks passed

NikaHsn deleted the fix/logging/indexdb branch October 18, 2023 16:10

khatruong2009 pushed a commit that referenced this pull request Nov 7, 2023

fix(logging): use indexedDB on web if it is supported (#3961)

94514d3

khatruong2009 pushed a commit that referenced this pull request Nov 27, 2023

fix(logging): use indexedDB on web if it is supported (#3961)

ae2fcfd

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(logging): use indexedDB on web if it is supported #3961

fix(logging): use indexedDB on web if it is supported #3961

NikaHsn commented Oct 16, 2023

fjnoyp Oct 17, 2023 •

edited

Loading

NikaHsn Oct 17, 2023

fjnoyp Oct 17, 2023

NikaHsn Oct 17, 2023

fjnoyp Oct 17, 2023

fjnoyp Oct 17, 2023 •

edited

Loading

NikaHsn Oct 17, 2023

fjnoyp Oct 17, 2023

NikaHsn Oct 17, 2023

fjnoyp Oct 17, 2023 •

edited

Loading

fjnoyp Oct 17, 2023 •

edited

Loading

fjnoyp Oct 17, 2023

fjnoyp commented Oct 17, 2023

NikaHsn commented Oct 17, 2023

fjnoyp Oct 17, 2023

NikaHsn Oct 17, 2023 •

edited

Loading

fjnoyp Oct 17, 2023

fjnoyp commented Oct 17, 2023

fjnoyp left a comment

fix(logging): use indexedDB on web if it is supported #3961

fix(logging): use indexedDB on web if it is supported #3961

Conversation

NikaHsn commented Oct 16, 2023

fjnoyp Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjnoyp Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjnoyp Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

fjnoyp Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjnoyp commented Oct 17, 2023

NikaHsn commented Oct 17, 2023

Choose a reason for hiding this comment

NikaHsn Oct 17, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fjnoyp commented Oct 17, 2023

fjnoyp left a comment

Choose a reason for hiding this comment

fjnoyp Oct 17, 2023 •

edited

Loading

fjnoyp Oct 17, 2023 •

edited

Loading

fjnoyp Oct 17, 2023 •

edited

Loading

fjnoyp Oct 17, 2023 •

edited

Loading

NikaHsn Oct 17, 2023 •

edited

Loading