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

Issue with Map<String, List<String>> where list is modifiable #768

Open
fericit-bostan opened this issue Apr 28, 2018 · 6 comments
Open
Labels

Comments

@fericit-bostan
Copy link

I have the following definition in my abstract class:

@Value.Parameter public abstract Map<String, List<String>> getDefaultPropertyValues();

After generating my immutable object I expect that the map returned from this method to be immutable nut I would also expect the list contained within to be immutable as well, but it is not. I am able to write the following expression and have it execute successfully.

myType.getMap().get("key1").add("some value")

I would expect this to raise an UnsupportedOperationException but instead I found the value was successfully added to the list.

How can I prevent this?
Thanks...

@elucash
Copy link
Member

elucash commented Apr 28, 2018

@fericit-bostan thank you for raising thing! I don't want to disappoint you, but our tool is not all-powerful and has quite a few limitations: we mostly handle base(outer) types, but not their type arguments, so we do some magic about outer Map, but not for nested List. I guess if we do some smart handling for some types (like java.util.Map) and not for others we would be on the slippery slope of a lot more inconsistency.

Having that said, I think that you would grossly benefit from using Guava's Multimap which is supported by Immutables and have all the guarantees of immutable implementation.

Moreover, If you're a hardcore user, you can try to use encodings to change the way certain attribute types are handled: http://immutables.github.io/encoding.html so you can control almost any aspect of type conversion/validation/sanitization

@fericit-bostan
Copy link
Author

@elucash - Thanks for responding so quickly. I gave the Multimap a go but found the same issue. I was able to modify the underlying ArrayList by adding new values to it.
I changed my implementation to:
@Value.Parameter public abstract Multimap<String, List<String>> getDefaultPropertyValues();

myType.getMap().get("key1").asList().get(0).add("some value");

Too bad, as it looked promising and would have been an easy win.

I'll read through the documentation on encodings, but do those only apply to custom types? Can I provide an encoding for a Map , for example?

@Stephan202
Copy link
Contributor

Stephan202 commented Apr 28, 2018

@fericit-bostan I think @elucash tried to suggest one of the following the following:

@Value.Parameter public abstract Multimap<String, String> getDefaultPropertyValues();
@Value.Parameter public abstract ListMultimap<String, String> getDefaultPropertyValues();
@Value.Parameter public abstract ImmutableListMultimap<String, String> getDefaultPropertyValues();

(The first version is discouraged by the Guava team itself, while the third even signals immutability at the level of the type system. So I'd go with option 3 if possible.)


The wiki states:

You rarely use the Multimap interface directly, however; more often you'll use ListMultimap or SetMultimap, which map keys to a List or a Set respectively.

@elucash
Copy link
Member

elucash commented Apr 28, 2018

@fericit-bostan just to clarify, multimap does not need using a list, so it will be ListMultimap<String, String> but not ListMultimap<String, List<String>>.

By using an encoding, you can override how particular matched type is handled, so you can target Map<K, List<V>> combination, for example, and specify all the conversion/construction/builder routines involving this type, so instances you construct will always normalize to immutable lists

@fericit-bostan
Copy link
Author

@Stephan202 & @elucash - Thank you for the clarifications. I was getting a bit lost with the encoding sample projects as they depend upon Java 9 and I am locked into Java 8 at this point.

I'll retry using MultiMap, or rather ListMultiMap, as an immediate solution to the issue but I am very much interested in working with the encoding since it would not require me to change the existing interface. Along those lines, is is a requirement to have the encodings in a separate project or can I get away with placing them into the existing project?

Thanks for the help. It is truly appreciated.

@elucash
Copy link
Member

elucash commented Apr 29, 2018

strange, I've never fully updated to java9 (or just don't remember doing so)
I think all of this (encoding stuff) should work with Java 8 as it was designed for Java 7 and 8 in the first place. Please, let me know of specific stuff which doesn't work.

I would really suggest separate out encodings to a module. There can be bad ordering issues. The processor of encodings takes an encoding definition and generate special activation annotations with "compiled" metadata. Then you use activation annotation to include/discover encoding for classes which use it. If you will have everything under a single compilation module, the activation annotation may be not generated yet at the time when immutable classes using it will be generated, resulting in all sorts of errors or silent failure to apply encoding.
Having encoding activation annotations precompiled and coming from a separate module makes things clear, reusable and compilation order handled by a build tool.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

3 participants