SharedPrefs migration - Invalid key check leads to crash [192824325]

Fixed

Bug

Status Update

No update yet.

Description

fl...@google.com

created issue #1

Jul 5, 2021 09:27AM

DataStore Component used: Proto DataStore (but behaviour should be common to Preferences as well given the scenario mentioned below)

DataStore Version used: rc01

Devices/Android versions reproduced on: all

Consider the following scenario: In SharedPreferences we have preference with key A saved. The app supports a preference with key B as well, but the user didn't get to the screen in the app where that is actually saved, therefore SharedPreferences is missing key B.

We implement a SharedPreferencesMigration, using the default MIGRATE_ALL_KEYS option and call sharedPreferencesView.get...(key = B) to define the mapping between SharedPrefs and DataStore. When migrating, we get "Can't access key outside migration". MIGRATE_ALL_KEYS creates a set of keys containing just key "A", as this was the only one set by the user. The solution to this is to manually define the list of keys to be migrated.

But, this is counter-intuitive to the MIGRATE_ALL_KEYS flag, as it assumes that all the keys are already saved in SharedPreferences.

Comments

fl...@google.com <fl...@google.com> #2Jul 5, 2021 04:46PM

Project: platform/frameworks/support
Branch: androidx-master-dev

commit c60f33e229e31ab328ef6b59dab63b264954831c
Author: Alexandre Elias <aelias@google.com>
Date: Fri Jul 10 16:23:09 2020

Semantics no-op cleanups

Partly in response to lmr's broad code review, I did a pass of
superficial API/implementation cleanup. The main changes are:

- I changed each Boolean SemanticsProperty where false is equivalent to
not being present to take "Unit" instead. This is conceptually
clearer: it avoids questions like "can I cancel out a semantics from a
merged child by setting it to false?" Because "property = Unit" looks
weird, I also changed the style of these to "property()".

- I moved the Semantics id generator closer to where it's used, in
SemanticsModifierCore. I made it internal and an AtomicInt.
(Note that integer ids are heavily used in the Android
AccessibilityNodeInfo APIs so I can't simply remove them entirely.)

- I deleted dead code. Some examples include SemanticsHintOverrides,
a public API not connected to anything, and SemanticsPropertyKey
merge() open method which is never called. (In both cases I have
a different plan in mind for accessibility.)

Fixes: 145951226
Fixes: 145955412
Test: existing tests
Relnote: "Single-value semantics properties now use a calling style.
For example, 'semantics { hidden = true }' is now written as:
'semantics { hidden() }'."

Change-Id: Ic1afd12ea22c926babc9662f1804d80b33aa0cfc

M ui/integration-tests/benchmark/src/androidTest/java/androidx/ui/benchmark/test/LayoutNodeModifierBenchmark.kt
M ui/ui-core/api/0.1.0-dev15.txt
M ui/ui-core/api/current.txt
M ui/ui-core/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-core/api/public_plus_experimental_current.txt
M ui/ui-core/api/restricted_0.1.0-dev15.txt
M ui/ui-core/api/restricted_current.txt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/graphics/vector/VectorTest.kt
M ui/ui-core/src/androidAndroidTest/kotlin/androidx/ui/semantics/SemanticsTests.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidActuals.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeView.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidComposeViewAccessibilityDelegateCompat.kt
M ui/ui-core/src/androidMain/kotlin/androidx/ui/core/AndroidPopup.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/Expect.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsConfiguration.kt
D ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsHintOverrides.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsModifier.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsNode.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsOwner.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/core/semantics/SemanticsWrapper.kt
M ui/ui-core/src/commonMain/kotlin/androidx/ui/semantics/SemanticsProperties.kt
M ui/ui-foundation/api/0.1.0-dev15.txt
M ui/ui-foundation/api/current.txt
M ui/ui-foundation/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-foundation/api/public_plus_experimental_current.txt
M ui/ui-foundation/api/restricted_0.1.0-dev15.txt
M ui/ui-foundation/api/restricted_current.txt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Clickable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Dialog.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/Scroller.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Selectable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/selection/Toggleable.kt
M ui/ui-foundation/src/main/java/androidx/ui/foundation/semantics/FoundationSemanticsProperties.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ButtonTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CardTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/CheckboxScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/RadioButtonScreenshotTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ScaffoldTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SnackbarTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/SurfaceTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/ripple/RippleIndicationTest.kt
M ui/ui-material/src/androidTest/java/androidx/ui/material/textfield/TextFieldScreenshotTest.kt
M ui/ui-material/src/main/java/androidx/ui/material/AppBar.kt
M ui/ui-material/src/main/java/androidx/ui/material/TextFieldImpl.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/AssertsTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/CallSemanticsActionTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ErrorMessagesTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/FindersTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/PrintToStringTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/ScrollToTest.kt
M ui/ui-test/src/androidTest/java/androidx/ui/test/TextActionsTest.kt
M ui/ui-test/src/main/java/androidx/ui/test/Actions.kt
M ui/ui-test/src/main/java/androidx/ui/test/Filters.kt
M ui/ui-text/api/0.1.0-dev15.txt
M ui/ui-text/api/current.txt
M ui/ui-text/api/public_plus_experimental_0.1.0-dev15.txt
M ui/ui-text/api/public_plus_experimental_current.txt
M ui/ui-text/api/restricted_0.1.0-dev15.txt
M ui/ui-text/api/restricted_current.txt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/CoreTextField.kt
M ui/ui-text/src/commonMain/kotlin/androidx/ui/text/TextSemanticsProperties.kt

https://android-review.googlesource.com/1360099

ap...@google.com <ap...@google.com> #3Jul 8, 2021 10:35PM

Project: platform/frameworks/support
Branch: androidx-main

commit 247fd51abbf6a6804d65a833d22423eef349d036
Author: Rohit Sathyanarayana <rohitsat@google.com>
Date: Wed Jul 07 13:33:40 2021

Stop throwing exceptions when a key doesn't exist when all keys is specified for SharedPreferencesMigration

Bug: 192824325
Relnote: Fix bug where SharedPreferencesMigration constructed with MIGRATE_ALL_KEYS would throw an exception if the key requested does not yet exist.
Test: new unit test
Change-Id: Ie318a9eb3772f5ef6dd231c18837842b887587e0

M buildSrc/src/main/kotlin/androidx/build/LibraryVersions.kt
M datastore/datastore/src/androidTest/java/androidx/datastore/migrations/SharedPreferencesMigrationTest.kt
M datastore/datastore/src/main/java/androidx/datastore/migrations/SharedPreferencesMigration.kt

https://android-review.googlesource.com/1758106

ap...@google.com <ap...@google.com> #4Jul 9, 2021 04:03PM

Project: platform/frameworks/support
Branch: androidx-main

commit 52a6dc065e4b794b7c40c208fcad6edd359d9b56
Author: Rohit Sathyanarayana <rohitsat@google.com>
Date: Thu Jul 08 16:24:40 2021

Add documentation to SharedPreferencesMigration

This was brought up in aosp/1758106 but I forgot to upload before submitting...

Relnote: clarify that SharedPreferencesMigration does not run if there are no keys.
Test: N/A
Bug: 192824325
Change-Id: Icfa327c72834bab2a53be8f3696f48c19781c93f

M datastore/datastore-rxjava2/src/main/java/androidx/datastore/rxjava2/RxSharedPreferencesMigration.kt
M datastore/datastore-rxjava3/src/main/java/androidx/datastore/rxjava3/RxSharedPreferencesMigration.kt
M datastore/datastore/src/main/java/androidx/datastore/migrations/SharedPreferencesMigration.kt

https://android-review.googlesource.com/1760672

ro...@google.com <ro...@google.com> #5Jul 9, 2021 04:14PM

Thanks for reporting this Florina. I've updated the code so that it will no longer crash when the key doesn't exist if it is constructed with MIGRATE_ALL_KEYS. The change will go out in rc2.

fl...@google.com <fl...@google.com> #6Jul 12, 2021 08:47AM

Thanks, Rohit!

ro...@google.com <ro...@google.com> Jul 12, 2021 07:48PM

Marked as fixed.

Issue 192824325