[shared_preferences] update List<String> encode/decode #8335

tarrinneal · 2024-12-20T03:02:04Z

Follow up to #8187

Updates Android List<String> encoding/decoding to use JSON to avoid potential future breakages from restricted list types.

…code

tarrinneal · 2025-01-03T12:26:14Z

@stuartmorgan ready for review here as well

...ndroid/src/main/java/io/flutter/plugins/sharedpreferences/LegacySharedPreferencesPlugin.java

reidbaker · 2025-01-06T16:16:50Z

...ndroid/src/main/java/io/flutter/plugins/sharedpreferences/LegacySharedPreferencesPlugin.java

    for (String key : allPrefs.keySet()) {
      if (key.startsWith(prefix) && (allowList == null || allowList.contains(key))) {
-        filteredPrefs.put(key, transformPref(key, allPrefs.get(key)));
+        filteredPrefs.put(key, transformPref(key, Objects.requireNonNull(allPrefs.get(key))));


What was requireNonNull needed here? Also this code is getting a bit difficult to parse. (A loop with a conditional that sets some transformed filtered data) Consider breaking parts out into methods or adding comments.

It's a change suggested by a warning I was getting in Android Studio. Since the method being called requires nonnull arguments and getting a value from a map doesn't programmatically guarantee a value. In this context, there would always be a value though.

reidbaker · 2025-01-06T16:22:40Z

...ndroid/src/main/java/io/flutter/plugins/sharedpreferences/LegacySharedPreferencesPlugin.java

    if (value instanceof String) {
      String stringValue = (String) value;
-      if (stringValue.startsWith(LIST_IDENTIFIER)) {
+      if (stringValue.startsWith(JSON_LIST_IDENTIFIER)) {


I think this is where having the keys be similar starts to be important. That said from reading the code the reader does not know the keys are overlapping and it would seem safe in code review to swap the order of these if statements.

Consider making the keys independent so that you are not relying on the contents of the keys. If that is too much code change then consider making a "LIST_PREFIX" that can be checked so that it is at least clear in code what the relationship is.

If you want further reading see
https://www.16elt.com/2024/09/25/first-book-of-byte-sized-tech/

I think the first example explains the principal I am applying.

Consider making the keys independent so that you are not relying on the contents of the keys.

This is something Tarrin and I had discussed offline; making it a prefix avoids this being a potentially (very unlikely, but still possible) breaking change, by using our already-reserved space.

I would strongly prefer we keep that, and adjust the code to make it explicit to readers.

Ok I can understand that fear and why it might fail. Thank you for being open to structuring the code differently so that the overlap is easier to follow for future maintainers.

...ndroid/src/main/java/io/flutter/plugins/sharedpreferences/LegacySharedPreferencesPlugin.java

...preferences/shared_preferences_android/example/integration_test/shared_preferences_test.dart

reidbaker · 2025-01-06T16:33:11Z

packages/shared_preferences/shared_preferences_android/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 3.0.0


Are the breaking changes required? if not consider fixing/mitigating the security issue independent of the breaking changes. It makes it more likely for them to be adopted.

Breaking changes to shared_preferences have the potential to be quite disruptive to the ecosystem; I would strongly prefer we keep the legacy type support around to making a breaking change.

Fair enough. I originally expected this change was going to end up being "technically" breaking due to the new prefix (before we discussed the prefix extension plan) so I wanted to wrap these long awaited breaking changes in as well. I'll revert them.

Breaking changes to shared_preferences have the potential to be quite disruptive to the ecosystem; I would strongly prefer we keep the legacy type support around to making a breaking change.

Should I revert the error handling changes as well?

Should I revert the error handling changes as well?

I did this.

packages/shared_preferences/shared_preferences_android/lib/src/strings.dart

packages/shared_preferences/shared_preferences_android/lib/src/shared_preferences_android.dart

reidbaker · 2025-01-06T16:37:09Z

packages/shared_preferences/shared_preferences_android/lib/src/shared_preferences_android.dart

      case 'String':
-        return _api.setString(key, value as String);
+        value as String;
+        if (value.startsWith(listPrefix) ||


Will this list grow? if so consider making a set of prefixes to check.

We don't generally add types to shared_preferences since anything that all platforms don't support ends up with unpleasant complexity like the List<String> mess we are currently dealing with.

stuartmorgan-g · 2025-01-06T17:49:26Z

packages/shared_preferences/shared_preferences_android/CHANGELOG.md

@@ -1,3 +1,9 @@
+## 3.0.0


Breaking changes to shared_preferences have the potential to be quite disruptive to the ecosystem; I would strongly prefer we keep the legacy type support around to making a breaking change.

...roid/android/src/main/kotlin/io/flutter/plugins/sharedpreferences/SharedPreferencesPlugin.kt

stuartmorgan-g · 2025-01-06T17:55:34Z

packages/shared_preferences/shared_preferences_android/lib/src/shared_preferences_android.dart

      case 'String':
-        return _api.setString(key, value as String);
+        value as String;
+        if (value.startsWith(listPrefix) ||


We don't generally add types to shared_preferences since anything that all platforms don't support ends up with unpleasant complexity like the List<String> mess we are currently dealing with.

.../shared_preferences/shared_preferences_android/lib/src/shared_preferences_async_android.dart

…code

packages/shared_preferences/shared_preferences_android/CHANGELOG.md

...ndroid/src/main/java/io/flutter/plugins/sharedpreferences/LegacySharedPreferencesPlugin.java

...roid/android/src/main/kotlin/io/flutter/plugins/sharedpreferences/SharedPreferencesPlugin.kt

...nces_android/android/src/main/kotlin/io/flutter/plugins/sharedpreferences/MessagesAsync.g.kt

...roid/android/src/main/kotlin/io/flutter/plugins/sharedpreferences/SharedPreferencesPlugin.kt

.../shared_preferences/shared_preferences_android/lib/src/shared_preferences_async_android.dart

…code

packages/shared_preferences/shared_preferences_android/pigeons/messages.dart

…code

stuartmorgan-g

LGTM

auto-submit · 2025-01-24T19:25:57Z

autosubmit label was removed for flutter/packages/8335, because - The status or check suite Linux_android android_platform_tests_shard_1 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

auto-submit · 2025-01-24T19:29:23Z

autosubmit label was removed for flutter/packages/8335, because - The status or check suite Linux_android android_platform_tests_shard_1 master has failed. Please fix the issues identified (or deflake) before re-applying this label.

stuartmorgan-g · 2025-01-25T16:38:48Z

It looks like this may have caused OOB failure in shared_preferences tests when it was published.

…tter/packages#8335)

flutter/packages@3d3ab7b...258f6dc 2025-01-24 [email protected] [shared_preferences] Add shared preferences devtool (flutter/packages#8494) 2025-01-24 [email protected] [shared_preferences] update List<String> encode/decode (flutter/packages#8335) 2025-01-24 [email protected] Manual roll Flutter from c1561a4 to c1ffaa9 (21 revisions) (flutter/packages#8498) 2025-01-24 [email protected] [ios_platform_images] Switch to `loadImage` (flutter/packages#8216) 2025-01-24 [email protected] [camera] Remove OCMock from CameraExposureTests and CameraFocusTests (flutter/packages#8351) 2025-01-24 [email protected] [shared_preferences] Tool for migrating from legacy shared_preferences to shared_preferences_async (flutter/packages#8229) 2025-01-23 [email protected] Revert "[shared_preferences] Add shared preferences devtool" (flutter/packages#8493) 2025-01-23 [email protected] [go_router] Fix return type of current state getter to be non-nullable (flutter/packages#8173) 2025-01-23 [email protected] Manual roll Flutter from b2f515f to c1561a4 (18 revisions) (flutter/packages#8491) 2025-01-23 [email protected] [pigeon] fixes event channel dart instance name usage and adds test (flutter/packages#8483) 2025-01-23 [email protected] [go _route] fragment parameter added (flutter/packages#8232) 2025-01-23 [email protected] [in_app_purchase] Update in_app_purchase_android version in in_app_purchase (flutter/packages#8463) 2025-01-23 [email protected] [image_picker] Reference alternate macOS implementations (flutter/packages#8487) 2025-01-23 [email protected] [rfw] Activate leak testing (flutter/packages#8370) 2025-01-23 [email protected] [video_player] Activate leak testing (flutter/packages#8379) 2025-01-23 [email protected] Manual roll Flutter from b9e86a5 to b2f515f (42 revisions) (flutter/packages#8482) 2025-01-23 [email protected] [camera] Add API support query for image streaming (app-facing) (flutter/packages#8422) 2025-01-23 [email protected] Manual roll Flutter from b9e86a5 to eb6af3d (13 revisions) (flutter/packages#8473) 2025-01-23 [email protected] [shared_preferences] Add shared preferences devtool (flutter/packages#8322) 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

camsim99 · 2025-01-27T17:30:13Z

This is causing shared_preferences/shared_preferences/example/integration_test/shared_preferences_test.dart: shared_preferences_async Async throws TypeError when returned getStringList type is incorrect to fail with what looks like an OOB issue and is blocking the tree currently:

══╡ EXCEPTION CAUGHT BY FLUTTER TEST FRAMEWORK ╞════════════════════════════════════════════════════
The following TestFailure was thrown running a test (but after the test had completed):
Expected: throws <Instance of 'TypeError'>
  Actual: <Closure: () => Future<Null>>
   Which: returned a Future that emitted <null>

When the exception was thrown, this was the stack:
#0      fail (package:matcher/src/expect/expect.dart:149:31)
#1      _expect.<anonymous closure> (package:matcher/src/expect/expect.dart:125:9)
<asynchronous suspension>
<asynchronous suspension>
(elided 6 frames from dart:async and package:stack_trace)
════════════════════════════════════════════════════════════════════════════════════════════════════

The plan is to revert.

auto-submit · 2025-01-27T17:33:07Z

Time to revert pull request flutter/packages/8335 has elapsed.
You need to open the revert manually and process as a regular pull request.

…er#8335)" This reverts commit f7efc2b.

This reverts commit f7efc2b.

Follow up to flutter#8187 Updates Android `List<String>` encoding/decoding to use JSON to avoid potential future breakages from restricted list types.

update List<String> encode/decode and other simple breaking changes

e772224

tarrinneal requested a review from stuartmorgan-g December 20, 2024 03:02

github-actions bot added platform-android platform-ios platform-macos p: shared_preferences labels Dec 20, 2024

tarrinneal added 3 commits January 2, 2025 11:36

fix test set up and legacy android system

089d11d

Merge branch 'main' of github.com:flutter/packages into listString-en…

29f3662

…code

format analyze update to latest pigeon

c0c08df

tarrinneal marked this pull request as ready for review January 3, 2025 12:25

tarrinneal requested a review from reidbaker as a code owner January 3, 2025 12:25

remove unneeded braces from switch statement

2ef0a01

reidbaker reviewed Jan 6, 2025

View reviewed changes

stuartmorgan-g reviewed Jan 6, 2025

View reviewed changes

tarrinneal added 2 commits January 6, 2025 14:39

revert all breaking changes

bad5268

Merge branch 'main' of github.com:flutter/packages into listString-en…

7a013b1

…code

github-actions bot removed platform-ios platform-macos labels Jan 6, 2025

tarrinneal added 4 commits January 6, 2025 15:23

rest of the comments

2c10fb7

update test getter to match setter

614cbe4

fix tests that I broke fixing tests

e622f04

fix android code for error handling changes reversion

d317354

tarrinneal requested a review from stuartmorgan-g January 7, 2025 17:02

tarrinneal changed the title ~~[shared_preferences] update List<String> encode/decode and other simple breaking changes~~ [shared_preferences] update List<String> encode/decode Jan 7, 2025

stuartmorgan-g reviewed Jan 8, 2025

View reviewed changes

tarrinneal added 4 commits January 14, 2025 13:04

split getList and other nits

79ebe43

Merge branch 'main' of github.com:flutter/packages into listString-en…

3a45c25

…code

add new method to kotlin class

a49fb9f

fix test

e1f8fab

stuartmorgan-g reviewed Jan 24, 2025

View reviewed changes

packages/shared_preferences/shared_preferences_android/pigeons/messages.dart Outdated Show resolved Hide resolved

tarrinneal added 2 commits January 24, 2025 10:11

rename setStringList methods

e89421c

Merge branch 'main' of github.com:flutter/packages into listString-en…

88ce597

…code

stuartmorgan-g approved these changes Jan 24, 2025

View reviewed changes

fix kotlin tests and format

7e28547

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 24, 2025

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 24, 2025

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 24, 2025

auto-submit bot removed the autosubmit Merge PR when tree becomes green via auto submit App label Jan 24, 2025

fix java tests too

1cdce8d

tarrinneal force-pushed the listString-encode branch from 24aefc2 to 1cdce8d Compare January 24, 2025 19:31

tarrinneal added the autosubmit Merge PR when tree becomes green via auto submit App label Jan 24, 2025

auto-submit bot merged commit f7efc2b into flutter:main Jan 24, 2025
77 checks passed

stuartmorgan-g mentioned this pull request Jan 27, 2025

[shared_preferences] Add shared preferences devtool #8494

Merged

11 tasks

engine-flutter-autoroll mentioned this pull request Jan 27, 2025

Roll Packages from 3d3ab7bcb07e to 258f6dce6a49 (19 revisions) flutter/flutter#162254

Merged

engine-flutter-autoroll added a commit to engine-flutter-autoroll/flutter that referenced this pull request Jan 27, 2025

f7efc2b67 [shared_preferences] update List<String> encode/decode (flu…

3dca84a

…tter/packages#8335)

camsim99 added the revert Label used to revert changes in a closed and merged pull request. label Jan 27, 2025

auto-submit bot removed the revert Label used to revert changes in a closed and merged pull request. label Jan 27, 2025

camsim99 added a commit to camsim99/packages that referenced this pull request Jan 27, 2025

Revert "[shared_preferences] update List<String> encode/decode (flutt…

c2e3d9d

…er#8335)" This reverts commit f7efc2b.

camsim99 added a commit that referenced this pull request Jan 27, 2025

Revert "[shared_preferences] update List<String> encode/decode (#8335)"

aeb7ab6

This reverts commit f7efc2b.

This was referenced Jan 27, 2025

Revert "[shared_preferences] update List<String> encode/decode" #8510

Closed

Unresolved reference: StringListObjectInputStream flutter/flutter#162044

Open

[shared_preferences] update List<String> encode/decode #8335

[shared_preferences] update List<String> encode/decode #8335

Uh oh!

Conversation

tarrinneal commented Dec 20, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

tarrinneal commented Jan 3, 2025

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

stuartmorgan-g Jan 6, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

stuartmorgan-g left a comment

Choose a reason for hiding this comment

Uh oh!

auto-submit bot commented Jan 24, 2025

Uh oh!

auto-submit bot commented Jan 24, 2025

Uh oh!

Uh oh!

stuartmorgan-g commented Jan 25, 2025

Uh oh!

camsim99 commented Jan 27, 2025

Uh oh!

auto-submit bot commented Jan 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

tarrinneal commented Dec 20, 2024 •

edited

Loading

stuartmorgan-g Jan 6, 2025 •

edited

Loading