Skip to content
Merged
Show file tree
Hide file tree
Changes from 5 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
e772224
update List<String> encode/decode and other simple breaking changes
tarrinneal Dec 20, 2024
089d11d
fix test set up and legacy android system
tarrinneal Jan 2, 2025
29f3662
Merge branch 'main' of github.com:flutter/packages into listString-en…
tarrinneal Jan 2, 2025
c0c08df
format analyze update to latest pigeon
tarrinneal Jan 3, 2025
2ef0a01
remove unneeded braces from switch statement
tarrinneal Jan 3, 2025
bad5268
revert all breaking changes
tarrinneal Jan 6, 2025
7a013b1
Merge branch 'main' of github.com:flutter/packages into listString-en…
tarrinneal Jan 6, 2025
2c10fb7
rest of the comments
tarrinneal Jan 6, 2025
614cbe4
update test getter to match setter
tarrinneal Jan 6, 2025
e622f04
fix tests that I broke fixing tests
tarrinneal Jan 7, 2025
d317354
fix android code for error handling changes reversion
tarrinneal Jan 7, 2025
79ebe43
split getList and other nits
tarrinneal Jan 14, 2025
3a45c25
Merge branch 'main' of github.com:flutter/packages into listString-en…
tarrinneal Jan 14, 2025
a49fb9f
add new method to kotlin class
tarrinneal Jan 16, 2025
e1f8fab
fix test
tarrinneal Jan 17, 2025
96d129b
comments and test fixes
tarrinneal Jan 24, 2025
e89421c
rename setStringList methods
tarrinneal Jan 24, 2025
88ce597
Merge branch 'main' of github.com:flutter/packages into listString-en…
tarrinneal Jan 24, 2025
7e28547
fix kotlin tests and format
tarrinneal Jan 24, 2025
1cdce8d
fix java tests too
tarrinneal Jan 24, 2025
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,3 +1,9 @@
## 3.0.0
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should I revert the error handling changes as well?

I did this.


* Updates non-supported type errors to `ArgumentError`.
* Removes support for reading `BigInt` and `Set<String>`.
* Moves `List<String>` encoding to dart `JSON` package.

## 2.4.0

* Adds `SharedPreferences` support within `SharedPreferencesAsyncAndroid` API.
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,24 +19,24 @@
import java.io.IOException;
import java.io.ObjectInputStream;
import java.io.ObjectOutputStream;
import java.math.BigInteger;
import java.util.ArrayList;
import java.util.HashMap;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

/** LegacySharedPreferencesPlugin */
public class LegacySharedPreferencesPlugin implements FlutterPlugin, SharedPreferencesApi {
private static final String TAG = "SharedPreferencesPlugin";
private static final String SHARED_PREFERENCES_NAME = "FlutterSharedPreferences";
private static final String JSON_LIST_IDENTIFIER = "VGhpcyBpcyB0aGUgcHJlZml4IGZvciBhIGxpc3Qu!";
private static final String LIST_IDENTIFIER = "VGhpcyBpcyB0aGUgcHJlZml4IGZvciBhIGxpc3Qu";
private static final String BIG_INTEGER_PREFIX = "VGhpcyBpcyB0aGUgcHJlZml4IGZvciBCaWdJbnRlZ2Vy";
private static final String DOUBLE_PREFIX = "VGhpcyBpcyB0aGUgcHJlZml4IGZvciBEb3VibGUu";

private SharedPreferences preferences;
private SharedPreferencesListEncoder listEncoder;
private final SharedPreferencesListEncoder listEncoder;

public LegacySharedPreferencesPlugin() {
this(new ListEncoder());
Expand Down Expand Up @@ -73,13 +73,6 @@ public void onDetachedFromEngine(@NonNull FlutterPlugin.FlutterPluginBinding bin

@Override
public @NonNull Boolean setString(@NonNull String key, @NonNull String value) {
// TODO (tarrinneal): Move this string prefix checking logic to dart code and make it an Argument Error.
if (value.startsWith(LIST_IDENTIFIER)
|| value.startsWith(BIG_INTEGER_PREFIX)
|| value.startsWith(DOUBLE_PREFIX)) {
throw new RuntimeException(
"StorageError: This string cannot be stored as it clashes with special identifier prefixes");
}
return preferences.edit().putString(key, value).commit();
}

Expand All @@ -99,6 +92,7 @@ public void onDetachedFromEngine(@NonNull FlutterPlugin.FlutterPluginBinding bin
return preferences.edit().remove(key).commit();
}

// Deprecated, for testing purposes only.
@Override
public @NonNull Boolean setStringList(@NonNull String key, @NonNull List<String> value)
throws RuntimeException {
Expand Down Expand Up @@ -131,14 +125,13 @@ public void onDetachedFromEngine(@NonNull FlutterPlugin.FlutterPluginBinding bin

// Gets all shared preferences, filtered to only those set with the given prefix.
// Optionally filtered also to only those items in the optional [allowList].
@SuppressWarnings("unchecked")
private @NonNull Map<String, Object> getAllPrefs(
@NonNull String prefix, @Nullable Set<String> allowList) throws RuntimeException {
Map<String, ?> allPrefs = preferences.getAll();
Map<String, Object> filteredPrefs = new HashMap<>();
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))));
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

}
}

Expand All @@ -148,32 +141,14 @@ public void onDetachedFromEngine(@NonNull FlutterPlugin.FlutterPluginBinding bin
private Object transformPref(@NonNull String key, @NonNull Object value) {
if (value instanceof String) {
String stringValue = (String) value;
if (stringValue.startsWith(LIST_IDENTIFIER)) {
if (stringValue.startsWith(JSON_LIST_IDENTIFIER)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

@stuartmorgan-g stuartmorgan-g Jan 6, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

return value;
} else if (stringValue.startsWith(LIST_IDENTIFIER)) {
return listEncoder.decode(stringValue.substring(LIST_IDENTIFIER.length()));
} else if (stringValue.startsWith(BIG_INTEGER_PREFIX)) {
// TODO (tarrinneal): Remove all BigInt code.
// https://github.com/flutter/flutter/issues/124420
String encoded = stringValue.substring(BIG_INTEGER_PREFIX.length());
return new BigInteger(encoded, Character.MAX_RADIX);
} else if (stringValue.startsWith(DOUBLE_PREFIX)) {
String doubleStr = stringValue.substring(DOUBLE_PREFIX.length());
return Double.valueOf(doubleStr);
}
} else if (value instanceof Set) {
// TODO (tarrinneal): Remove Set code.
// https://github.com/flutter/flutter/issues/124420

// This only happens for previous usage of setStringSet. The app expects a list.
@SuppressWarnings("unchecked")
List<String> listValue = new ArrayList<>((Set<String>) value);
// Let's migrate the value too while we are at it.
preferences
.edit()
.remove(key)
.putString(key, LIST_IDENTIFIER + listEncoder.encode(listValue))
.apply();

return listValue;
}
return value;
}
Expand Down
Loading
Loading