Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[android] Migrate installation identifier to non-backed-up storage #11005

Merged
merged 10 commits into from
Nov 20, 2020

Conversation

sjchmiela
Copy link
Contributor

@sjchmiela sjchmiela commented Nov 12, 2020

Why

#10261 (comment). Also fixes #11008 by making expo-notifications use the same installation ID as expo-constants and expoview (f1ecd07).

How

Found a (in my opinion) nicer way to store a string in a non-backed-up storage (than defining a <full-backup-content> XML file and requiring developers to implement their own BackupAgent in some circumstances, etc.) — using getNoBackupFilesDir to get a directory where we create a simple .txt file. The advantage it provides is not requiring developers to modify any native files to incorporate this feature.

I wrote a class that tries to migrate the UUID from SharedPreferences to noBackupFilesDir on getUUID call. It should handle invalid UUIDs well (by ignoring it). Then I copied it from expoview to expo-constants and expo-notifications in case there are bare projects that use one and not the other (we don't want to depend on migration in -constants in -notifications and vice versa). It follows the implementation outline of #10261 (comment) with the following modifications:

  • instead of removing the SharedPreferences/keychain entry "so we recover from corrupt data" I decided to ignore it
  • if we didn't read a valid ID and we wouldn't intend to create one if it wasn't present we don't immediately generate a new one. There are still parts of code that do not "get-or-create" the UUID, just "get". For them we need a sensible "just-get" implementation.
  • instead of computing the v5 UUID from the Firebase Instance ID (as proposed by the main overview) I've decided to stick with a random v4 UUID since fetching the Firebase Instance ID starts some kind of connection with Firebase servers — it may also require Firebase app configured (but I haven't verified that) and even though installationId is mostly used in expo-notifications, we don't say anywhere that accessing expo-constants.installationId requires Firebase configured.
  • instead of using SharedPreferences I decided to save the file in noBackupFilesDir which seems less breakable than using SharedPreferences and configuring full-backup-content.

Another option I was thinking of was to create a new unimodule expo-installations (expo-installation-id) just for this class and depend on the new unimodule in expoview, expo-constants and expo-notifications. Since we intend to deprecate and eventually remove .installationId creating a unimodule just for half a year and deprecating it immediately doesn't seem like the best idea.

Test Plan

I have verified that:

  • Constants.installationId from running Expo client on master is the same as .installationId returned when running Expo client on this branch
  • expo-notifications's installation ID from running Expo client on master is the same as installationId returned when running Expo client on this branch
  • removing and reinstalling Expo client sets a different installationId
  • modifying the file so that is does not contain a valid UUID discards its contents and persists a new UUID

Test approach #2

Test scenarios for installation identifiers:

  • on an experience running on SDK39 when Expo client upgrades
    • SDK39 ConstantsBinding keeps using mExponentSharedPreferences.getOrCreateUUID. Unversioned ExponentSharedPreferences migrates UUID from unscoped SharedPreferences to unscoped non-backed-up storage. No change. ✅
    • SDK39 InstallationIdProvider keeps using scoped SharedPreferences. Migration isn't being added to versioned InstallationIdProviders, identifier keeps being backed-up but it doesn't change. ⚠️
  • when an experience using SDK39 upgrades to SDK40 in Expo client
    • Both SDK39 and SDK40 ConstantsBindings use mExponentSharedPreferences.getOrCreateUUID which uses migrated non-backed-up storage. No change ✅
    • SDK39 InstallationIdProvider used scoped SharedPreferences, SDK40 ScopedInstallationIdProvider uses mExponentSharedPreferences.getOrCreateUUID if there is no existing ID (new project) or migrates legacy UUID from scoped SharedPreferences to scoped no-backup-dir if it exists and keeps using it in the future. All in all there's no change. ✅
  • when standalone app using SDK39 upgrades to SDK40
    • Both SDK39 and SDK40 ConstantsBindings use mExponentSharedPreferences.getOrCreateUUID which uses migrated non-backed-up storage. No change ✅
    • SDK39 InstallationIdProvider had ID saved in scoped SharedPreferences, SDK40 InstallationIdProvider migrates that ID to scoped noBackupDir. No change ✅
  • when an SDK39 project ejects to bare
    • SDK39 ConstantsBinding was using mExponentSharedPreferences.getOrCreateUUID which persisted ID in unscoped SharedPreferences. Upon ejection we start using ConstantsService with unscoped Context which results in using the same SharedPreferences. No change ✅
    • SDK39 InstallationIdProvider uses scoped SharedPreferences to persist installation ID. Upon ejection we start using unscoped SharedPreferences, ID changes to one equal to Constants.installationId. ⚠️ We can live with that.
  • when an SDK40 project ejects to bare
    • SDK40 ConstantsBinding was using mExponentSharedPreferences which persisted ID in unscoped non-backed-up storage. ConstantsService uses the same storage location, ID doesn't change. ✅
    • SDK40 ScopedInstallationProvider was using either mExponentSharedPreferences which persisted ID in unscoped non-backed-up storage (in this case bare InstallationIdProvider uses unscoped common installation ID and there are no changes. ✅) or used ID migrated from scoped SharedPreferences to scoped noBackupDir in which case the ID changes, but we can live with that). ⚠️
  • when a bare project upgrades expo-notifications or expo-constants
    • Previous installation ID providers used unscoped SharedPreferences. Upon upgrade, the ID gets migrated to the same location by either expo-notifications or expo-constants. ID stays the same. ✅

@github-actions
Copy link
Contributor

Native Component List for this branch is ready

@sjchmiela sjchmiela force-pushed the @sjchmiela/migrate-installation-id-no-backup-android branch 2 times, most recently from bde1ec0 to f1ecd07 Compare November 12, 2020 22:01
@sjchmiela sjchmiela marked this pull request as ready for review November 13, 2020 18:15
@sjchmiela sjchmiela requested review from ide and removed request for tsapeta, bbarthec and cruzach November 13, 2020 18:15
@sjchmiela sjchmiela force-pushed the @sjchmiela/migrate-installation-id-no-backup-android branch from f1ecd07 to d5a5f88 Compare November 17, 2020 16:41
Copy link
Member

@ide ide left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you help comment / describe what code can be deleted when:

  • we remove legacy Notifications?
  • we remove Constants.installationId?

sjchmiela added a commit that referenced this pull request Nov 19, 2020
# Why

- iOS companion for #11005.
- 1e22e08 fixes #11008 (comment) ensuring future versioned `EXInstallationIdProvider`s use the common installation ID

# How

Implemented #10261 (comment).

- As in #11005, the migration code is present in ~3~2 places:
  - ~in `ExpoKit` for managed apps relying on legacy notifications~
  - in `expo-constants` for bare workflow apps and for managed workflow apps (`EXKernel` now uses `expo-constants` directly to fetch installation ID)
  - in `expo-notifications` for bare workflow apps that for some reason do not get UUID migrated by `expo-constants` (eg. because they do not have `expo-constants` installed or have installed in older version).
- To expose common, migrated `deviceInstallUUID` to versioned `expo-constants` code I've added a simple kernel service plugged into already versioned `EXConstantsBinding`s.

# Test Plan

I have verified (previously) that:
- `Constants.installationId` from running Expo client on `master` is the same as `.installationId` returned when running Expo client on this branch
- `expo-notifications`'s installation ID from running Expo client on `master` is the same as `installationId` returned when running Expo client on this branch
- removing and reinstalling Expo client **does not** generate a different installationId (as we know keychain isn't removed!)
- if we fetch an invalid UUID from the Keychain, new UUID is generated
- I experienced an app freeze once on `__ulock_wait`, when waiting for return from `SecItemCopyMatching` which isn't a common issue and were only a few reports online of

# Test approach #2

Test scenarios for installation identifiers:
- **on an experience running on SDK39 when Expo client upgrades**
  - SDK39 `EXConstantsService` used `NSUserDefaults.EXDeviceInstallUUIDKey` which is now cleared by migration. Patched `EXConstantsService` uses `EXDeviceInstallUUIDManager` kernel service which provides it with the "common" ID, migrated from the `NSUserDefaults.EXDeviceInstallUUIDKey`, so the value doesn't change. ✅ 
  - SDK39 `EXInstallationIdProvider` used `NSUserDefaults.ABI39_0_0EXDeviceInstallUUIDKey` which is not cleared by any migration. **Identifier keeps being backed-up** but it doesn't change. ⚠️ 
- **when an experience using SDK39 upgrades to SDK40 in Expo client**
  - SDK39 `EXConstantsService` used `NSUserDefaults.EXDeviceInstallUUIDKey` which got migrated to keychain. SDK40 `EXConstantsService` uses the same keychain entry as the migrator, so they're using the same value. ✅ 
  - SDK39 `EXInstallationIdProvider` used `NSUserDefaults.ABI39_0_0EXDeviceInstallUUIDKey` while SDK40 uses common device UUID the sources are obviously different. Token changes. ❌  This is a bug introduced with `expo-notifications` Expo client integration, we can't do anything about it apart from fixing it in SDK40, IDs for old SDKs are already created and will be used in corresponding SDKs and if we didn't do anything about it in SDK40, the ID per experience would change nonetheless, so let's change it this time for the last time.
- when standalone app using SDK39 upgrades to SDK40
  - Unversioned SDK39 `EXConstantsService` used `NSUserDefaults.EXDeviceInstallUUIDKey` which gets migrated to keychain, unversioned SDK40 `EXConstantsService` uses the same keychain entry as common UUID, no changes. ✅ 
  - Unversioned SDK39 `EXInstallationIdProvider` used `NSUserDefaults.EXDeviceInstallUUIDKey` which gets migrated to keychain, unversioned SDK40 `EXInstallationIdProvider` uses the same keychain entry as common UUID, no changes. ✅ 
- when an SDK39 project ejects to bare
  - SDK39 `EXConstantsService` used `NSUserDefaults.EXDeviceInstallUUIDKey`, the same which is used in bare, ✅ 
  - SDK39 `EXInstallationIdProvider` used `NSUserDefaults.EXDeviceInstallUUIDKey` in standalone apps, but `NSUserDefaults.ABI39_0_0EXDeviceInstallUUIDKey` in Expo client. Token changes on developers' devices. ⚠️ 
- when an SDK40 project ejects to bare
  - SDK40 `EXConstantsService` used `EXDeviceInstallUUIDKey` keychain entry, the same which is used in bare, ✅ 
  - SDK40 `EXInstallationIdProvider` used `EXDeviceInstallUUIDKey` keychain entry, the same which is used in bare (`EXDeviceInstallUUIDKey`), ✅ 
- when a bare project upgrades `expo-notifications` or `expo-constants`
  - both projects have migrators that move `EXDeviceInstallUUIDKey` value from `NSUserDefaults` to keychain, token doesn't change. ✅
@sjchmiela sjchmiela force-pushed the @sjchmiela/migrate-installation-id-no-backup-android branch from 5953801 to 84ec511 Compare November 19, 2020 12:57
@sjchmiela
Copy link
Contributor Author

What can we remove when:

  • once we remove legacy Notifications — we will be able to remove ExponentNotificationIntentService (not mentioned in this PR), but I think we would still keep ExponentSharedPreferences and its migrator as it provides installation ID for scoped expo-constants and expo-notifications
  • once we remove legacy Notifications and Constants.installationId — we would be able to remove ExponentSharedPreferences' getOrCreateUUID in favor of moving the unscoped UUID code "closer" to expo-notifications (as this would be the only remaining piece of code using it). ScopedInstallationIdProvider could be initialized with ScopedContext (it already is) and it could try to fetch installation ID from either scoped storage or from unscoped storage (as it already does
    @Override
    public void getInstallationIdAsync(Promise promise) {
    // If there is an existing installation ID, so if:
    // - we're in Expo client and running an experience
    // which has previously been run on an older SDK
    // (where it persisted an installation ID in
    // the legacy storage) or
    // - we're in a standalone app after update
    // from SDK where installation ID has been
    // persisted in legacy storage
    // we let the migration do its job of moving
    // expo-notifications-specific installation ID
    // from scoped SharedPreferences to scoped noBackupDir
    // and use it from now on.
    String legacyUuid = mInstallationId.getUUID();
    if (legacyUuid != null) {
    promise.resolve(legacyUuid);
    } else {
    // Otherwise we can use the "common" installation ID
    // that has the benefit of being used if the project
    // is ejected to bare.
    promise.resolve(mExponentSharedPreferences.getOrCreateUUID());
    }
    }
    ), but it wouldn't need ExponentSharedPreferences to do so.

Co-authored-by: James Ide <ide@users.noreply.github.com>
@sjchmiela sjchmiela force-pushed the @sjchmiela/migrate-installation-id-no-backup-android branch from 0bad05d to ead4c85 Compare November 19, 2020 19:11
brentvatne pushed a commit that referenced this pull request Nov 19, 2020
- iOS companion for #11005.
- 1e22e08 fixes #11008 (comment) ensuring future versioned `EXInstallationIdProvider`s use the common installation ID

Implemented #10261 (comment).

- As in #11005, the migration code is present in ~3~2 places:
  - ~in `ExpoKit` for managed apps relying on legacy notifications~
  - in `expo-constants` for bare workflow apps and for managed workflow apps (`EXKernel` now uses `expo-constants` directly to fetch installation ID)
  - in `expo-notifications` for bare workflow apps that for some reason do not get UUID migrated by `expo-constants` (eg. because they do not have `expo-constants` installed or have installed in older version).
- To expose common, migrated `deviceInstallUUID` to versioned `expo-constants` code I've added a simple kernel service plugged into already versioned `EXConstantsBinding`s.

I have verified (previously) that:
- `Constants.installationId` from running Expo client on `master` is the same as `.installationId` returned when running Expo client on this branch
- `expo-notifications`'s installation ID from running Expo client on `master` is the same as `installationId` returned when running Expo client on this branch
- removing and reinstalling Expo client **does not** generate a different installationId (as we know keychain isn't removed!)
- if we fetch an invalid UUID from the Keychain, new UUID is generated
- I experienced an app freeze once on `__ulock_wait`, when waiting for return from `SecItemCopyMatching` which isn't a common issue and were only a few reports online of

Test scenarios for installation identifiers:
- **on an experience running on SDK39 when Expo client upgrades**
  - SDK39 `EXConstantsService` used `NSUserDefaults.EXDeviceInstallUUIDKey` which is now cleared by migration. Patched `EXConstantsService` uses `EXDeviceInstallUUIDManager` kernel service which provides it with the "common" ID, migrated from the `NSUserDefaults.EXDeviceInstallUUIDKey`, so the value doesn't change. ✅
  - SDK39 `EXInstallationIdProvider` used `NSUserDefaults.ABI39_0_0EXDeviceInstallUUIDKey` which is not cleared by any migration. **Identifier keeps being backed-up** but it doesn't change. ⚠️
- **when an experience using SDK39 upgrades to SDK40 in Expo client**
  - SDK39 `EXConstantsService` used `NSUserDefaults.EXDeviceInstallUUIDKey` which got migrated to keychain. SDK40 `EXConstantsService` uses the same keychain entry as the migrator, so they're using the same value. ✅
  - SDK39 `EXInstallationIdProvider` used `NSUserDefaults.ABI39_0_0EXDeviceInstallUUIDKey` while SDK40 uses common device UUID the sources are obviously different. Token changes. ❌  This is a bug introduced with `expo-notifications` Expo client integration, we can't do anything about it apart from fixing it in SDK40, IDs for old SDKs are already created and will be used in corresponding SDKs and if we didn't do anything about it in SDK40, the ID per experience would change nonetheless, so let's change it this time for the last time.
- when standalone app using SDK39 upgrades to SDK40
  - Unversioned SDK39 `EXConstantsService` used `NSUserDefaults.EXDeviceInstallUUIDKey` which gets migrated to keychain, unversioned SDK40 `EXConstantsService` uses the same keychain entry as common UUID, no changes. ✅
  - Unversioned SDK39 `EXInstallationIdProvider` used `NSUserDefaults.EXDeviceInstallUUIDKey` which gets migrated to keychain, unversioned SDK40 `EXInstallationIdProvider` uses the same keychain entry as common UUID, no changes. ✅
- when an SDK39 project ejects to bare
  - SDK39 `EXConstantsService` used `NSUserDefaults.EXDeviceInstallUUIDKey`, the same which is used in bare, ✅
  - SDK39 `EXInstallationIdProvider` used `NSUserDefaults.EXDeviceInstallUUIDKey` in standalone apps, but `NSUserDefaults.ABI39_0_0EXDeviceInstallUUIDKey` in Expo client. Token changes on developers' devices. ⚠️
- when an SDK40 project ejects to bare
  - SDK40 `EXConstantsService` used `EXDeviceInstallUUIDKey` keychain entry, the same which is used in bare, ✅
  - SDK40 `EXInstallationIdProvider` used `EXDeviceInstallUUIDKey` keychain entry, the same which is used in bare (`EXDeviceInstallUUIDKey`), ✅
- when a bare project upgrades `expo-notifications` or `expo-constants`
  - both projects have migrators that move `EXDeviceInstallUUIDKey` value from `NSUserDefaults` to keychain, token doesn't change. ✅
@brentvatne
Copy link
Member

@sjchmiela - i cherry-picked the ios companion to this into sdk-40, shall we land this and cherry-pick it there as well? cc @byCedric to time versioning android around this

@sjchmiela sjchmiela merged commit 3df6eb5 into master Nov 20, 2020
@sjchmiela sjchmiela deleted the @sjchmiela/migrate-installation-id-no-backup-android branch November 20, 2020 16:38
sjchmiela added a commit that referenced this pull request Nov 20, 2020
…11005)

# Why

#10261 (comment). Also fixes #11008 by making `expo-notifications` use the same installation ID as `expo-constants` and `expoview` (f1ecd07).

# How

Found a (in my opinion) nicer way to store a string in a non-backed-up storage (than defining a `<full-backup-content>` XML file and requiring developers to implement their own `BackupAgent` in some circumstances, etc.) — using [`getNoBackupFilesDir`](https://developer.android.com/reference/android/content/Context#getNoBackupFilesDir()) to get a directory where we create a simple `.txt` file. The advantage it provides is not requiring developers to [modify any native files](https://github.com/expo/expo/pull/10261/files#diff-d8d19d8e0ef909f84b94eb86534e4dde2f0659b56c72ce5bcde12c5815e8b2fd) to incorporate this feature.

I wrote a class that tries to migrate the UUID from `SharedPreferences` to `noBackupFilesDir` on `getUUID` call. It should handle invalid UUIDs well (by ignoring it). Then I copied it from `expoview` to `expo-constants` and `expo-notifications` in case there are bare projects that use one and not the other (we don't want to depend on migration in `-constants` in `-notifications` and vice versa). It follows the implementation outline of #10261 (comment) with the following modifications:
- instead of removing the `SharedPreferences/keychain` entry "so we recover from corrupt data" I decided to ignore it
- if we didn't read a valid ID and we wouldn't intend to create one if it wasn't present we don't immediately generate a new one. There are still parts of code that do not "get-or-create" the UUID, just "get". For them we need a sensible "just-get" implementation.
- instead of computing the v5 UUID from the Firebase Instance ID (as proposed by the main overview) I've decided to stick with a random v4 UUID since fetching the Firebase Instance ID [starts some kind of connection with Firebase servers](https://github.com/TheWizard91/Album_base_source_from_JADX/blob/e1d228fc2ee550ac19eeac700254af8b0f96080a/sources/com/google/firebase/iid/FirebaseInstanceId.java#L227-L231) — it may also require Firebase app configured (but I haven't verified that) and even though `installationId` is mostly used in `expo-notifications`, we don't say anywhere that accessing `expo-constants.installationId` requires Firebase configured.
- instead of using `SharedPreferences` I decided to save the file in `noBackupFilesDir` which seems less breakable than using `SharedPreferences` and configuring `full-backup-content`.

Another option I was thinking of was to create a new unimodule `expo-installations` (`expo-installation-id`) just for this class and depend on the new unimodule in `expoview`, `expo-constants` and `expo-notifications`. Since we intend to deprecate and eventually remove `.installationId` creating a unimodule just for half a year and deprecating it immediately doesn't seem like the best idea.

# Test Plan

I have verified that:
- `Constants.installationId` from running Expo client on `master` is the same as `.installationId` returned when running Expo client on this branch
- `expo-notifications`'s installation ID from running Expo client on `master` is the same as `installationId` returned when running Expo client on this branch
- removing and reinstalling Expo client sets a different `installationId`
- modifying the file so that is does not contain a valid UUID discards its contents and persists a new UUID

# Test approach #2

Test scenarios for installation identifiers:
- **on an experience running on SDK39 when Expo client upgrades**
  - SDK39 `ConstantsBinding` keeps using `mExponentSharedPreferences.getOrCreateUUID`. Unversioned `ExponentSharedPreferences` migrates UUID from unscoped `SharedPreferences` to unscoped non-backed-up storage. No change. ✅ 
  - SDK39 `InstallationIdProvider` keeps using scoped `SharedPreferences`. Migration isn't being added to versioned `InstallationIdProvider`s, **identifier keeps being backed-up** but it doesn't change. ⚠️ 
- **when an experience using SDK39 upgrades to SDK40 in Expo client**
  - Both SDK39 and SDK40 `ConstantsBinding`s use `mExponentSharedPreferences.getOrCreateUUID` which uses migrated non-backed-up storage. No change ✅ 
  - SDK39 `InstallationIdProvider` used scoped `SharedPreferences`, SDK40 `ScopedInstallationIdProvider` uses `mExponentSharedPreferences.getOrCreateUUID` if there is no existing ID (new project) or migrates legacy UUID from scoped `SharedPreferences` to scoped no-backup-dir if it exists and keeps using it in the future. All in all there's no change. ✅ 
- when standalone app using SDK39 upgrades to SDK40
  - Both SDK39 and SDK40 `ConstantsBinding`s use `mExponentSharedPreferences.getOrCreateUUID` which uses migrated non-backed-up storage. No change ✅ 
  - SDK39 `InstallationIdProvider` had ID saved in scoped `SharedPreferences`, SDK40 `InstallationIdProvider` migrates that ID to scoped `noBackupDir`. No change ✅ 
- when an SDK39 project ejects to bare
  - SDK39 `ConstantsBinding` was using `mExponentSharedPreferences.getOrCreateUUID` which persisted ID in unscoped `SharedPreferences`. Upon ejection we start using `ConstantsService` with unscoped `Context` which results in using the same `SharedPreferences`. No change ✅ 
  - SDK39 `InstallationIdProvider` uses scoped `SharedPreferences` to persist installation ID. Upon ejection we start using unscoped `SharedPreferences`, ID changes to one equal to `Constants.installationId`. ⚠️ [We can live with that.](#11008 (comment))
- when an SDK40 project ejects to bare
  - SDK40 `ConstantsBinding` was using `mExponentSharedPreferences` which persisted ID in unscoped non-backed-up storage. `ConstantsService` uses the same storage location, ID doesn't change. ✅ 
  - SDK40 `ScopedInstallationProvider` was using either `mExponentSharedPreferences` which persisted ID in unscoped non-backed-up storage (in this case bare `InstallationIdProvider` uses unscoped common installation ID and there are no changes. ✅) or used ID migrated from scoped `SharedPreferences` to scoped `noBackupDir` in which case the ID changes, but [we can live with that](#11008 (comment))). ⚠️
- when a bare project upgrades `expo-notifications` or `expo-constants`
  - Previous installation ID providers used unscoped `SharedPreferences`. Upon upgrade, the ID gets migrated to the same location by either `expo-notifications` or `expo-constants`. ID stays the same. ✅
@sjchmiela
Copy link
Contributor Author

Thanks, sorry for not letting you know — I planned to cherry-pick both of the "companion pull request" at once once this got 100% approved as well. Thank you for taking care of that!

As one can see, I merged this and cherry-picked the commit to sdk-40. 🙂

ide pushed a commit that referenced this pull request Dec 9, 2020
… apps migrate legacy ID to a different location (#11249)

Why
--

During beta testing period I have been verifying if recent changes #11005 and #11019 work as we would expect them to. I noticed one of the scenarios, where we upgrade an SDK39 app to SDK40 and expect the `expo-notifications` installation ID to stay the same isn't true. I have investigated the issue (see [explanatory post](https://gist.github.com/sjchmiela/c9e8529dc4dd8ab425b8d0c33e7836d1)).

How
--

In order to prepare for simple deprecation of `Constants.installationId`, `expo-notifications`' installation identifier migration now takes 3 different storage places into account in the following order:
1. `noBackupDir/expo_notifications_installation_uuid.txt` — the primary destination storage. If we have an ID there, this is the one we should use.
2. `SharedPreferences/UUID` — the legacy storage used by `expo-constants` and `expo-notifications` up to SDK40. If we have an ID there we are either:
    - in managed workflow where `SharedPreferences` are scoped, so this ID is most probably different from `Constants.installationId`
    - in bare workflow and `expo-constants` have not migrated the ID to `expo_installation_uuid.txt` file yet (maybe they're old?)
    
    
    If we detected an ID in this location we copy the value to our primary storage, but **do not remove the SharedPreferences entry**. While in the first scenario it would not cause any issues, in the second scenario if we removed the value `expo-constants`, when updated, would think it needs to create a new installation identifier, which we do not want.

    ⚠️ **Not removing `SharedPreferences` entry leaves app susceptible to the already existing bug** — if backup already has a `SharedPreferences` ID inside and we do not remove it, devices restored in the future will have the same ID. I see a couple of solutions to this problem:
      - keep using `expo_notifications_installation_uuid.txt ?? expo_installation_uuid.txt` for notifications' ID (as in original PR proposal)
      - remove `SharedPreferences` ID and guard against incompatible versions of dependencies with `peerDependencies` — a halfway solution, peer dependencies warnings are often ignored, in some projects may cause `Constants.installationId` to reset.
3. `noBackupDir/expo_installation_uuid.txt` — the legacy storage used by `expo-constants` and `expoview` since SDK40. If we have an ID there we are either (and we haven't already read ID from any of the aforementioned locations):
    - in managed workflow in an app that has not yet used `expo-notifications` (since there's no ID in scoped `SharedPreferences`)
    - in bare workflow where `expo-constants` have already migrated the ID.
    
    Either way we copy the value to our primary storage **not removing the file** not to cause the ID to change in other modules.

Test Plan
--

I have confirmed that running an SDK40 app with these changes included, the ID gets migrated from scoped `SharedPreferences` to `expo_notifications_installation_id.txt`.
esamelson pushed a commit that referenced this pull request Dec 9, 2020
… apps migrate legacy ID to a different location (#11249)

Why
--

During beta testing period I have been verifying if recent changes #11005 and #11019 work as we would expect them to. I noticed one of the scenarios, where we upgrade an SDK39 app to SDK40 and expect the `expo-notifications` installation ID to stay the same isn't true. I have investigated the issue (see [explanatory post](https://gist.github.com/sjchmiela/c9e8529dc4dd8ab425b8d0c33e7836d1)).

How
--

In order to prepare for simple deprecation of `Constants.installationId`, `expo-notifications`' installation identifier migration now takes 3 different storage places into account in the following order:
1. `noBackupDir/expo_notifications_installation_uuid.txt` — the primary destination storage. If we have an ID there, this is the one we should use.
2. `SharedPreferences/UUID` — the legacy storage used by `expo-constants` and `expo-notifications` up to SDK40. If we have an ID there we are either:
    - in managed workflow where `SharedPreferences` are scoped, so this ID is most probably different from `Constants.installationId`
    - in bare workflow and `expo-constants` have not migrated the ID to `expo_installation_uuid.txt` file yet (maybe they're old?)
    
    
    If we detected an ID in this location we copy the value to our primary storage, but **do not remove the SharedPreferences entry**. While in the first scenario it would not cause any issues, in the second scenario if we removed the value `expo-constants`, when updated, would think it needs to create a new installation identifier, which we do not want.

    ⚠️ **Not removing `SharedPreferences` entry leaves app susceptible to the already existing bug** — if backup already has a `SharedPreferences` ID inside and we do not remove it, devices restored in the future will have the same ID. I see a couple of solutions to this problem:
      - keep using `expo_notifications_installation_uuid.txt ?? expo_installation_uuid.txt` for notifications' ID (as in original PR proposal)
      - remove `SharedPreferences` ID and guard against incompatible versions of dependencies with `peerDependencies` — a halfway solution, peer dependencies warnings are often ignored, in some projects may cause `Constants.installationId` to reset.
3. `noBackupDir/expo_installation_uuid.txt` — the legacy storage used by `expo-constants` and `expoview` since SDK40. If we have an ID there we are either (and we haven't already read ID from any of the aforementioned locations):
    - in managed workflow in an app that has not yet used `expo-notifications` (since there's no ID in scoped `SharedPreferences`)
    - in bare workflow where `expo-constants` have already migrated the ID.
    
    Either way we copy the value to our primary storage **not removing the file** not to cause the ID to change in other modules.

Test Plan
--

I have confirmed that running an SDK40 app with these changes included, the ID gets migrated from scoped `SharedPreferences` to `expo_notifications_installation_id.txt`.
byCedric pushed a commit that referenced this pull request Dec 14, 2020
… apps migrate legacy ID to a different location (#11249)

Why
--

During beta testing period I have been verifying if recent changes #11005 and #11019 work as we would expect them to. I noticed one of the scenarios, where we upgrade an SDK39 app to SDK40 and expect the `expo-notifications` installation ID to stay the same isn't true. I have investigated the issue (see [explanatory post](https://gist.github.com/sjchmiela/c9e8529dc4dd8ab425b8d0c33e7836d1)).

How
--

In order to prepare for simple deprecation of `Constants.installationId`, `expo-notifications`' installation identifier migration now takes 3 different storage places into account in the following order:
1. `noBackupDir/expo_notifications_installation_uuid.txt` — the primary destination storage. If we have an ID there, this is the one we should use.
2. `SharedPreferences/UUID` — the legacy storage used by `expo-constants` and `expo-notifications` up to SDK40. If we have an ID there we are either:
    - in managed workflow where `SharedPreferences` are scoped, so this ID is most probably different from `Constants.installationId`
    - in bare workflow and `expo-constants` have not migrated the ID to `expo_installation_uuid.txt` file yet (maybe they're old?)
    
    
    If we detected an ID in this location we copy the value to our primary storage, but **do not remove the SharedPreferences entry**. While in the first scenario it would not cause any issues, in the second scenario if we removed the value `expo-constants`, when updated, would think it needs to create a new installation identifier, which we do not want.

    ⚠️ **Not removing `SharedPreferences` entry leaves app susceptible to the already existing bug** — if backup already has a `SharedPreferences` ID inside and we do not remove it, devices restored in the future will have the same ID. I see a couple of solutions to this problem:
      - keep using `expo_notifications_installation_uuid.txt ?? expo_installation_uuid.txt` for notifications' ID (as in original PR proposal)
      - remove `SharedPreferences` ID and guard against incompatible versions of dependencies with `peerDependencies` — a halfway solution, peer dependencies warnings are often ignored, in some projects may cause `Constants.installationId` to reset.
3. `noBackupDir/expo_installation_uuid.txt` — the legacy storage used by `expo-constants` and `expoview` since SDK40. If we have an ID there we are either (and we haven't already read ID from any of the aforementioned locations):
    - in managed workflow in an app that has not yet used `expo-notifications` (since there's no ID in scoped `SharedPreferences`)
    - in bare workflow where `expo-constants` have already migrated the ID.
    
    Either way we copy the value to our primary storage **not removing the file** not to cause the ID to change in other modules.

Test Plan
--

I have confirmed that running an SDK40 app with these changes included, the ID gets migrated from scoped `SharedPreferences` to `expo_notifications_installation_id.txt`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[expo-notifications] Device identifier used by expo-notifications is scoped per experience
3 participants