feat(analytics): Legacy data migration of Pinpoint Endpoint ID

[fjnoyp]

Issue #, if available:

Description of changes:

Add code to extract the previous Analytics Pinpoint Endpoint ID stored in Android or iOS.

Users upgrading to our next release won't have their Pinpoint Endpoint regenerated and can keep pinpoint session on a given user device between the upgrade.

Testing

Before, when upgrading Analytics, a new Pinpoint ID would be assigned. This causes the AWS Pinpoint console to register a new endpoint and therefore increment the 'daily active endpoint' in the analytics dashboard. This is problematic because Pinpoint is now detecting a new user.

With this change, when upgrading and running the same Analytics app on the same device, the 'daily active endpoint' number should NOT increment.

You can test this by modifying the Analytics example app pubspec.yaml to use 0.6.1 and to remove the local path dependencies. You will also need to delete the pubspec_overrides.yaml generated by aft bootstrap. Then modify example app's main.dart to simply add the analytics/auth plugins (in dart first there are new initialization options). Run the app and verify the AWS console registers your session and 'daily active endpoint' increases by 1. Afterwards, undo the previous changes and run the Analytics dart first version. The code will now retrieve the old pinpoint id, and thus 'daily active endpoint' will not increment.

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

[fjnoyp]

I based this on the other PR for integration tests because that PR refactors several parts of Analytics that this PR relies on (primarily with configure of the plugin).

[dnys1]

Initialize inline or add check if already initialized

[Jordan-Nelson]

I think what Dillon might be suggesting is to combine initialize and getEndpointId, which makes sense to me since it looks like init is always called right before getEndpointId. You can check if sharedPrefs has been initialized, and if not, do so. For example:

var sharedPrefs: SharedPreferences? = null

fun getEndpointId(pinpointAppId: String, result: Messages.Result<String?>){    
    sharedPrefs = sharedPrefs ?: context.getSharedPreferences(
        "${pinpointAppId}$PINPOINT_SHARED_PREFS_SUFFIX",
        Context.MODE_PRIVATE
    )
    result.success(sharedPrefs!!.getString(UNIQUE_ID_KEY, null))
}

[Jordan-Nelson]

Did you find out if there was any specific reason secure_storage wasn't working? The code here is similar to _getValue() in secure_storage. One difference I see is that secure_storage sets an explicit value for kSecAttrAccessible, which is not done here. This means it will fallback on the default, which might be what the SDK uses. I can look into what impact that has in accessing data.

[fjnoyp]

Thanks for helping here, looks like this was resolved.

[Jordan-Nelson]

Should this be endpointInfoStore? It seems like this was renamed in some places and not in others.

[ragingsquirrel3]

+1, getting build errors here which prevent testing, seen in integ tests

[fjnoyp]

Thanks resolved. I did a rebase and appear to have missed a commit somehow ...

[Jordan-Nelson]

This was marked as resolved but I think still applies

[Jordan-Nelson]

What is the purpose of this? Can you add a comment? Would an enum make more sense here?

[fjnoyp]

  /// Version of metadata stored into the `endpointInfoStore`
  /// If stored data changes, increment this value
  static const String _endpointInformationCurrentVersionValue = '0';

Added the above comments. The initial idea is to track if we have read legacy data from native.

If so, we store this value into secure storage to avoid further reads from legacy data.

In case of future changes, if we need to change legacy read behavior to read other values we would increment this value and store that instead. Having this value stored before allows us to quickly know what the storage state is (either no value, "0", "1", etc)

[Jordan-Nelson]

Okay. I would suggest using an enum for this as we did in Auth. It provides a nice way to document this.

amplify-flutter/packages/auth/amplify_auth_cognito_dart/lib/src/credentials/credential_store_keys.dart

Lines 10 to 20 in 834a81d

	/// The credential store version.
	enum CredentialStoreVersion {
	/// No credential store version is present.
	///
	/// Either the credential store has never been initialized on this device, or
	/// the credentials are stored in a legacy format.
	none,
	/// The initial implementation of Credential in Amplify Flutter Auth.
	v1,
	}

[codecov-commenter]

Codecov Report

Merging #2489 (5b12a78) into next (b9d6d08) will increase coverage by 40.34%.
The diff coverage is n/a.

@@             Coverage Diff             @@
##             next    #2489       +/-   ##
===========================================
+ Coverage   56.19%   96.53%   +40.34%     
===========================================
  Files         115       13      -102     
  Lines        6987     1182     -5805     
===========================================
- Hits         3926     1141     -2785     
+ Misses       3061       41     -3020     

Flag	Coverage Δ
android-unit-tests	?
flutter-unit-tests	?
ios-unit-tests	96.53% <ø> (+1.20%)	⬆️

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

Impacted Files	Coverage Δ
...ib/src/widgets/form_fields/sign_in_form_field.dart
...src/graphql/model_helpers/model_subscriptions.dart
...widgets/form_fields/reset_password_form_field.dart
...mplify_authenticator/lib/src/utils/breakpoint.dart
...src/graphql/web_socket/types/web_socket_types.dart
...mplify_authenticator/lib/src/state/auth_state.dart
...or/lib/src/widgets/authenticator_input_config.dart
.../ios/unit_tests/amplify_flutter_exampleTests.swift
...nticator/lib/src/widgets/authenticator_banner.dart
...mplify_authenticator/lib/src/widgets/progress.dart
... and 92 more

[ragingsquirrel3]

I agree with other comments that would be good to see platform/pigeon stuff consolidated as well as comment about secure storage detail before reviewing again. Biggest question was how non android/ios platforms should avoid the legacy stuff as it did not seem correct when I tested on MacOS (I did not try other platforms).

[ragingsquirrel3]

question: Shouldn't this be for type: plugin like the other plugins? How was this file created?

[fjnoyp]

Thanks good catch there. It appears this was created on the flutter create . command not sure how/why it was modified since then though. It should be project_type plugin so I changed it. Looks to be used by flutter when performing upgrades so making this change should be fine.

Used this as a resource in case you're curious to learn more about what metadata is: https://dev.to/swimmingkiim/what-is-metadata-file-in-flutter-project-38m3

[ragingsquirrel3]

+1, getting build errors here which prevent testing, seen in integ tests

[ragingsquirrel3]

question: Why use this version of uuid instead of uuid() from aws_common? If there is specific requirement for that could we have a comment here saying why?

[fjnoyp]

Thanks there is no reason, wasn't aware of this method in core package.

[ragingsquirrel3]

question: Maybe I'm missing something here, but is this using DataProviderAndroid for all non-ios io platforms (desktop windows, macos, linux)? When I ran example app on macos I got this error:

[ERROR:flutter/runtime/dart_vm_initializer.cc(41)] Unhandled Exception: PlatformException(channel-error, Unable to establish connection on channel., null, null)
#0      PigeonDataProvider.initialize (package:amplify_analytics_pinpoint/src/legacy_native_data_provider/pigeon_data_provider.android.g.dart:40:7)
<asynchronous suspension>
#1      AmplifyAnalyticsPinpointDart.configure (package:amplify_analytics_pinpoint_dart/src/analytics_plugin_impl.dart:169:7)
<asynchronous suspension>
#2      Future.wait.<anonymous closure> (dart:async/future.dart:522:21)
<asynchronous suspension>
#3      AmplifyClassImpl.configurePlatform (package:amplify_core/src/amplify_class_impl.dart:73:5)
<asynchronous suspension>
#4      AmplifyClass.configure (package:amplify_core/src/amplify_class.dart:107:7)
<asynchronous suspension>
#5      _MyAppState._configureAmplify (package:amplify_analytics_pinpoint_example/main.dart:70:7)
<asynchronous suspension>

[fjnoyp]

Thanks good catch, I'll need to fix this then

[ragingsquirrel3]

question: Should this block only execute on ios/android like:

if (!zIsWeb && (Platform.isAndroid || Platform.isIOS)) {
}

I mentioned in another comment I got an error from here on macos but I wonder if it would simplify things just to stop this logic far upstream (here) to simplify non ios/android platforms as much as possible.

[fjnoyp]

Thanks yes updated the logic

[fjnoyp]

Do not merge until PR fixes to secure storage are in:

bfeae2a

That PR allows us to properly extract legacy ios data using the secure storage plugin instead of custom ios code.

[Jordan-Nelson]

**/**/**/ should be reducible to **/

Suggested change

	- "**/**/**/*.g.dart"
	- "**/*.g.dart"

[dnys1]

Merging with next was unsuccessful. Please fix and address Jordan's comment:

analyzer: 
  errors:
    implementation_imports: error #TODO(equartey): Remove when lint is enforced project-wide    
  exclude:
    - "**/*.g.dart"

[Jordan-Nelson]

Okay. I would suggest using an enum for this as we did in Auth. It provides a nice way to document this.

amplify-flutter/packages/auth/amplify_auth_cognito_dart/lib/src/credentials/credential_store_keys.dart

Lines 10 to 20 in 834a81d

	/// The credential store version.
	enum CredentialStoreVersion {
	/// No credential store version is present.
	///
	/// Either the credential store has never been initialized on this device, or
	/// the credentials are stored in a legacy format.
	none,
	/// The initial implementation of Credential in Amplify Flutter Auth.
	v1,
	}

[Jordan-Nelson]

these two functions are called twice (here & lines 153/154 above). Is that on purpose? I think they both get called during data migration. Since this is a one time event, that is not the end of the world, but it is a little confusing to follow.

[Jordan-Nelson]

I am having a bit of a hard time following this. I believe you want to achieve something like:

• Check version
• If version is null, attempt migration (one time event)
  • read ID from legacy storage
  • if non-null, write to new storage
  • update version to '0' to prevent migration on future app inits
• Read version from storage
• If null, generate a new one
  • generate new ID
  • write to storage

This is an example of a flow that I think is easier to follow.

    // Attempt migration if version is null.
    if (endpointInformationVersion == null) {
      await _legacyNativeDataProvider?.initialize(appId);
      final legacyEndpointId = await _legacyNativeDataProvider?.getEndpointId();
      // Migrate legacy data if it is non-null
      if (legacyEndpointId != null) {
        await _endpointInfoStore.write(
          key: endpointIdStorageKey,
          value: legacyEndpointId,
        );
      }
      // Update the version to prevent future legacy data migrations.
      await _endpointInfoStore.write(
        key: _endpointInformationVersionKey,
        value: _endpointInformationCurrentVersionValue,
      );
    }

    // Read the existing ID.
    final currentEndpointId = await _endpointInfoStore.read(
      key: endpointIdStorageKey,
    );

    final String fixedEndpointId;

    // Generate a new ID if one does not exist.
    if (currentEndpointId == null) {
      fixedEndpointId = uuid();
      await _endpointInfoStore.write(
        key: endpointIdStorageKey,
        value: fixedEndpointId,
      );
    } else {
      fixedEndpointId = currentEndpointId;
    }

This will result in reading the ID twice in the scenario where there is data to migrate (first from legacy storage, and then from the new storage), but since it is a one time event I think that is fine.

[fjnoyp]

Alright it seems like a bit longer of a solution and possibly more complex because there are now 2 conditionals but I see how some of the code before was implicit. I think it's good that the read call no longer requires a force cast, so if something goes wrong and currentEndpointId is null we no everything will still work and no crash will occur.

[Jordan-Nelson]

I think what Dillon might be suggesting is to combine initialize and getEndpointId, which makes sense to me since it looks like init is always called right before getEndpointId. You can check if sharedPrefs has been initialized, and if not, do so. For example:

var sharedPrefs: SharedPreferences? = null

fun getEndpointId(pinpointAppId: String, result: Messages.Result<String?>){    
    sharedPrefs = sharedPrefs ?: context.getSharedPreferences(
        "${pinpointAppId}$PINPOINT_SHARED_PREFS_SUFFIX",
        Context.MODE_PRIVATE
    )
    result.success(sharedPrefs!!.getString(UNIQUE_ID_KEY, null))
}

[ragingsquirrel3]

approved assuming web crash fixed per comments regarding !zIsWeb.

[dnys1]

Merging with next was unsuccessful. Please fix and address Jordan's comment:

analyzer: 
  errors:
    implementation_imports: error #TODO(equartey): Remove when lint is enforced project-wide    
  exclude:
    - "**/*.g.dart"

[dnys1]

Since the goal is to arrive at a fixed endpoint ID, use a single variable to convey that journey. This also cleans up the logic branches and saves you a read from the DB in some cases.

Suggested change

	if (endpointInformationVersion == null) {
	final legacyEndpointId =
	await legacyDataProvider?.getEndpointId(pinpointAppId);
	// Migrate legacy data if it is non-null
	if (legacyEndpointId != null) {
	await store.write(
	key: endpointIdStorageKey,
	value: legacyEndpointId,
	);
	}
	// Update the version to prevent future legacy data migrations.
	await store.write(
	key: EndpointStoreKey.version.name,
	value: EndpointStoreVersion.v1.name,
	);
	}
	// Read the existing ID.
	final currentEndpointId = await store.read(
	key: endpointIdStorageKey,
	);
	final String fixedEndpointId;
	// Generate a new ID if one does not exist.
	if (currentEndpointId == null) {
	fixedEndpointId = uuid();
	await store.write(
	key: endpointIdStorageKey,
	value: fixedEndpointId,
	);
	} else {
	fixedEndpointId = currentEndpointId;
	}
	return fixedEndpointId;
	String? fixedEndpointId;
	if (endpointInformationVersion == null) {
	final legacyEndpointId =
	await legacyDataProvider?.getEndpointId(pinpointAppId);
	// Migrate legacy data if it is non-null
	if (legacyEndpointId != null) {
	fixedEndpointId = legacyEndpointId;
	await store.write(
	key: endpointIdStorageKey,
	value: legacyEndpointId,
	);
	}
	// Update the version to prevent future legacy data migrations.
	await store.write(
	key: EndpointStoreKey.version.name,
	value: EndpointStoreVersion.v1.name,
	);
	}
	// Read the existing ID.
	fixedEndpointId ??= await store.read(
	key: endpointIdStorageKey,
	);
	// Generate a new ID if one does not exist.
	if (fixedEndpointId == null) {
	fixedEndpointId = uuid();
	await store.write(
	key: endpointIdStorageKey,
	value: fixedEndpointId,
	);
	}
	return fixedEndpointId;

[fjnoyp]

Yeah while you were gone I had a single variable implementation beforehand as well. Your suggestion is close to what I had before.

But previous comments suggested a different approach.

This is clearly a bit subjective, let's use your suggestion here.

cc @Jordan-Nelson

[dnys1]

My comments are not in conflict with Jordan's. He was confused about the duplication of logic and unnecessary reads/writes.