[shared_preferences] Fix Android type mismatch regression #8512
Conversation
`getStringList` should throw a `TypeError` if the stored value is of a different type, but the recent change to use JSON-encoded string lists regression that behavior if the stored type was a string, causing it to instead return null. This restores the previous behavior by passing extra information from Kotlin to Dart when attempting to get an enecoded string list, so that if a non-encoded-list string is found, a TypeError can be created on the Dart side. Since extra information is now being passed, the case of a legacy-encoded value is now communicated as well, so that we only have to request the legacy value if it's there, rather than always trying (which was not worth the complexity of adding extra data just for that initially, but now that we need extra data anyway, it's easy to distinguish that case). Fixes OOB regression in `shared_preferences` tests that has closed the tree.
|
I did consider an alternate version where I just always returned the raw string value, then did the checks for null vs one of the prefixes vs a raw string all on the Dart side, but that meant always sending the entire string (even the potentially large base-64-legacy-encoded string) over the platform channel for basically no reason, so I felt like this would be cleaner. (In a potential future world where we convert this to FFI, that won't be an issue and we could just have all the logic in Dart.) |
| } | ||
|
|
||
| @Test | ||
| fun testSetAndGetStringListWithSharedPreferencesRedirectsForLegacy() { |
There was a problem hiding this comment.
non blocking nit: would prefer "...DeprecatedStringList" over "Legacy" but this is a test so the consequence is small.
There was a problem hiding this comment.
Renamed to the more specific "PlatformEncoded".
| /// - If false, an unexpected string (one without any encoding prefix) was | ||
| /// found. This will happen if a client uses getStringList with a key that | ||
| /// was used with setString. | ||
| bool foundPlatformEncodedValue; |
There was a problem hiding this comment.
This seems like the type of decision we will regret later when there is yet another way to store lists of strings. Consider an enum.
There was a problem hiding this comment.
This seems like the type of decision we will regret later when there is yet another way to store lists of strings.
I did consider an enum originally, but the two-value enum seemed like overkill once I started writing it. I'll go ahead and switch back, but this is an internal implementation detail, so if ($DIETY forbid) we add a third way to encode string lists we wouldn't regret the bool, we would just change it then.
(Also, ideally we'd refactor more logic into Dart before then so we wouldn't even need to.)
There was a problem hiding this comment.
Fixed; it's now a three-state enum. One of the states is redundant with the string value being non-null, but that made the logic flow easier to understand than it was before.
|
This looks good to go, so let's land on red to fix the packages tree! |
camsim99
added
emergency
|
The label the bot is looking for changed to |
stuartmorgan-g
removed
the
emergency
|
autosubmit label was removed for flutter/packages/8512, because emergency label processing failed |
auto-submit
bot
removed
the
autosubmit
stuartmorgan-g
added
emergency
|
Oops; I removed and re-added it because I wasn't sure the rename would get picked up, but it looks like maybe my timing on removing it was terrible. |
|
autosubmit label was removed for flutter/packages/8512, because emergency label processing failed |
auto-submit
bot
removed
the
autosubmit
|
No, I think it's broken. Landing manually, and I'll file an issue. |
flutter/packages@258f6dc...02c6fef 2025-01-28 [email protected] Revert "[shared_preferences] Add shared preferences devtool" (flutter/packages#8515) 2025-01-28 [email protected] [webview_flutter_wkwebview] Updates the internal wrapper to use `@ProxyApi` from pigeon (flutter/packages#8311) 2025-01-28 [email protected] update the `const` in StatefulShellRouteData generation (flutter/packages#8502) 2025-01-27 [email protected] [video_player_platform_interface] Platform view support (flutter/packages#8453) 2025-01-27 [email protected] [multicast_dns] Rewrite `_readFQDN` to avoid recursion (flutter/packages#8390) 2025-01-27 [email protected] [shared_preferences] Fix Android type mismatch regression (flutter/packages#8512) If this roll has caused a breakage, revert this CL and stop the roller using the controls here: https://autoroll.skia.org/r/flutter-packages-flutter-autoroll Please CC [email protected] on the revert to ensure that a human is aware of the problem. To file a bug in Flutter: https://github.com/flutter/flutter/issues/new/choose To report a problem with the AutoRoller itself, please file a bug: https://issues.skia.org/issues/new?component=1389291&template=1850622 Documentation for the AutoRoller is here: https://skia.googlesource.com/buildbot/+doc/main/autoroll/README.md
getStringListshould throw aTypeErrorif the stored value is of a different type, but the recent change to use JSON-encoded string lists regression that behavior if the stored type was a string, causing it to instead return null.This restores the previous behavior by passing extra information from Kotlin to Dart when attempting to get an enecoded string list, so that if a non-encoded-list string is found, a TypeError can be created on the Dart side.
Since extra information is now being passed, the case of a legacy-encoded value is now communicated as well, so that we only have to request the legacy value if it's there, rather than always trying (which was not worth the complexity of adding extra data just for that initially, but now that we need extra data anyway, it's easy to distinguish that case).
Fixes OOB regression in
shared_preferencestests that has closed the tree.Pre-launch Checklist
dart format.)[shared_preferences]pubspec.yamlwith an appropriate new version according to the pub versioning philosophy, or this PR is exempt from version changes.CHANGELOG.mdto add a description of the change, following repository CHANGELOG style, or this PR is exempt from CHANGELOG changes.///).