fix(datastore): fix observe may receive duplicate events in Android

[HuiSF]

Issue #, if available:

Description of changes:

• Add a flag to prevent multiple closure registration to amplify-android observe API
• Place flutterResult call to more appropriate spots

---

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

[dnys1]

• Does setUpObserve ever need to be called more than once? Should we just reject all subsequent calls?
• This code is not thread-safe. Do we know for sure observe will only ever be called from the main thread or should we add atomics to avoid a race condition?
• If there is an error besides "Failed to start DataStore" on the first setup call, DataStore will never be set up since all other calls will be ignored. Is that the correct behavior? Also, this approach of checking strings is very brittle.
• The PR changes what's sent back with the result (nil -> true in iOS, and true/false in Android), but there is no Dart code in the PR. Are these values handled appropriately already? Does a return of false mean something from an error?

[HuiSF]

• Does setUpObserve ever need to be called more than once? Should we just reject all subsequent calls?

setUpObserve is called in each DataStore API to ensure the event channel to be set up regardless the order of APIs that are being called by a developer. The actual set up logic on native side is executed only once, and subsequent calls will be ignored (by invoke flutterResult.success())

• This code is not thread-safe. Do we know for sure observe will only ever be called from the main thread or should we add atomics to avoid a race condition?

The method channel call happens on main thread, and the platform implementation didn't use TaskQueue should it should execute on main thread as well. Do you have any examples that we can invoke observe on a different thread?

• If there is an error besides "Failed to start DataStore" on the first setup call, DataStore will never be set up since all other calls will be ignored. Is that the correct behavior? Also, this approach of checking strings is very brittle.

I think there is a misunderstanding -
setUpObserve is for creating a bridge forwarding native observe events to Flutter event channel. As long as the native observe call is executed (where the callback closures are registered), the set up is completed and time to return the method channel call.

"Failed to start DataStore" doesn't cancel the set up, I picked it out here, in order to find a good reason to do flutterResult.success(false). Even when "Failed to start DataStore" happened, and later DataStore restarted, the observe channel should still work.

However, I think calling flutterResult.success() with a boolean is confusing... The dart implementation doesn't expect a specific value, as above described behavior. So we could get rid of flutterResult.success(false) and invoke flutterResult.success() when the callback closures are registered, and further we won't have the brittle string check on errors.

• The PR changes what's sent back with the result (nil -> true in iOS, and true/false in Android), but there is no Dart code in the PR. Are these values handled appropriately already? Does a return of false mean something from an error?

As I described above the value doesn't matter, we could unify both platform implementation.

[HuiSF]

I've pushed a new commit (fdf01f6) to use AtomicBoolean for setting and checking isSettingUpObserve to address the concern "This code is not thread-safe".

[codecov-commenter]

Codecov Report

Merging #1339 (fdf01f6) into main (117191e) will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##             main    #1339   +/-   ##
=======================================
  Coverage   47.64%   47.64%           
=======================================
  Files         234      234           
  Lines        9899     9899           
=======================================
  Hits         4716     4716           
  Misses       5183     5183           

Flag	Coverage Δ
android-unit-tests	∅ <ø> (∅)
flutter-unit-tests	36.14% <ø> (ø)
ios-unit-tests	95.41% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

[dnys1]

If the else clause is triggered, isSettingUpObserve is never reset

[HuiSF]

The else branch is not involved with observe set up, when it falls into the else it's just forwarding the normal error events into the observe channel. At this moment, the set up has been completed. Doing this all because amplify-android send all type errors to this failure consume...

[cshfang]

Really only had some minor comments. Lgtm otherwise