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

Return unmodifiable string set from preferences #65

Merged
merged 1 commit into from
Mar 10, 2017

Conversation

NightlyNexus
Copy link
Contributor

https://developer.android.com/reference/android/content/SharedPreferences.html#getStringSet(java.lang.String,%20java.util.Set%3Cjava.lang.String%3E)

This can cause subtle errors and ConcurrentModificationExceptions if the set is mutated by the caller.
This is a breaking change, but I thought I'd propose it, anyway. :)
Also, it could just return a mutable copy (new LinkedHashSet<>() instead of unmodifiableSet()) to be a compatible change.

@@ -12,7 +13,7 @@
static final StringSetAdapter INSTANCE = new StringSetAdapter();

@Override public Set<String> get(@NonNull String key, @NonNull SharedPreferences preferences) {
return preferences.getStringSet(key, null);
return Collections.unmodifiableSet(preferences.getStringSet(key, null));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

oh, it's going to throw an npe. will fix.

@f2prateek
Copy link
Owner

f2prateek commented Jan 16, 2017

Thanks! Changes LGTM.

Behaviour wise, not 100% sure if it's the responsibility of the library to do this. Might be equally surprising for a user to see we're wrapping the set returned by the preferences.

@f2prateek
Copy link
Owner

(I'm not too worries about the breaking changes part since v2 is relatively new, and might need some breaking changes with Flowables and nulls anyway)

@NightlyNexus
Copy link
Contributor Author

NightlyNexus commented Jan 16, 2017

I think API calls for collections should normally return unmodifiable collections. Returning mutable collections is a surprising way to share state (and it's just an oversight/error for the Android sdk to do so).
That's my feeling haha.

@NightlyNexus
Copy link
Contributor Author

(I realized it's actually not a breaking change, since the default value is Collections.emptySet(), currently.)

@f2prateek
Copy link
Owner

@NightlyNexus sorry forgot to follow up here. What's behaviour if you call SharedPreferences#getStringSet(). It returns a mutable value, correct?

@NightlyNexus
Copy link
Contributor Author

NightlyNexus commented Mar 9, 2017

That's actually the whole of the problem.
It's "mutable," but it's not supposed to be mutable, leading to subtle bugs. (I eliminated ConcurrentModificationExceptions from the apps I worked on by fixing this in my fork.)

From the doc:

Note that you must not modify the set instance returned by this call.

Just sad legacy behavior in AOSP that it doesn't return an unmodifiable set, I believe.

@f2prateek
Copy link
Owner

Got it. Ok let's add a small note about this in javadoc and :shipit:

@NightlyNexus
Copy link
Contributor Author

NightlyNexus commented Mar 10, 2017

Done. I just made a quick edit to the javadoc and am never offended if you want to tweak any wording, etc. haha.

@f2prateek
Copy link
Owner

LGTM!

@f2prateek f2prateek merged commit 04c75c4 into f2prateek:master Mar 10, 2017
@NightlyNexus NightlyNexus deleted the patch-1 branch March 10, 2017 03:32
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.

2 participants