himanshudixit / google-collections

Automatically exported from code.google.com/p/google-collections
Apache License 2.0
0 stars 0 forks source link

ImmutableMap fails on null value #234

Closed GoogleCodeExporter closed 9 years ago

GoogleCodeExporter commented 9 years ago
Immutable map raises a strange exception when one tried to add an entry where 
key is not null and value is null.

It makes ImmutableMap unusable in most cases, where it can be made from other 
maps that allow null values (it seems all other implementation except 
ImmutableMap).

What so strange and insane restriction for?

Original issue reported on code.google.com by leonidos on 10 Sep 2009 at 3:35

GoogleCodeExporter commented 9 years ago
this will be explained fully in the faq when I get around to that.  But realize 
that 
this is in common with virtually all new collections added to the JDK after JDK 
1.4.  
It's not true at all that it makes ImmutableMap "unusable in most cases"; it's 
more 
like 5% of cases based on our study of the extensive codebase at Google.  In 
those 
cases, just do it the old-fashioned way and you're fine.

Original comment by kevin...@gmail.com on 10 Sep 2009 at 3:49

GoogleCodeExporter commented 9 years ago
May be to make it customizable?

Original comment by leonidos on 10 Sep 2009 at 4:06

GoogleCodeExporter commented 9 years ago
Could you please explain me what kind of problem can occur when a _value_ (not 
key) is 
null? As I know, map values are not compared with something else, ImmutableMap 
doesn't 
attempt to do something with it. What this restriction for?

Original comment by leonidos on 10 Sep 2009 at 4:11

GoogleCodeExporter commented 9 years ago
leonidos, here's one:

  Map<Integer, String> map = ...
  String value = map.get(5);
  if (value == null) {
    // this situation is ambiguous! We can't differentiate between
    //    A: no-mapping for 5
    //    B: the mapping for 5 is null
  }

ImmutableMap neatly avoids this problem.

Original comment by limpbizkit on 11 Sep 2009 at 3:20

GoogleCodeExporter commented 9 years ago
Yes, I agree with you that using null values in a map is a bad practice; in my 
code 
this never happens.

But some legacy code can assume that map allows nulls - so it's impossible to 
use 
ImmutableMap in legacy code refactorings.

If nulls not supporting behaviour is one of the major Google Collections lib 
properties 
- I think it should be documented - otherwise a user can be surprized very late.

Original comment by leonidos on 23 Sep 2009 at 2:02

GoogleCodeExporter commented 9 years ago
@limpbiskit

but couldn't you check with map.containsKey(5) if there's a mapping for 5 before
get()ting it?

Original comment by andre.ru...@gmail.com on 23 Sep 2009 at 2:15

GoogleCodeExporter commented 9 years ago
"But some legacy code can assume that map allows nulls - so it's impossible to 
use 
ImmutableMap in legacy code refactorings."

This doesn't bother me at all.  It's better to give the 95% case exactly what 
they 
want.

Original comment by kevin...@gmail.com on 23 Sep 2009 at 3:54

GoogleCodeExporter commented 9 years ago
May by it would be reasonable to introduce:
ImmutableMap.allowNullValues().<all nice methods of ImmutableMap>

Or introduce a kind of NULL_VALUE singleton, that could be put in map and later 
checked in client code.

Original comment by kua...@gmail.com on 6 Oct 2009 at 3:25

GoogleCodeExporter commented 9 years ago
I play around null problem and make a solution for my self:

In ImmutableMap I change:
1) add 
  static <K, V> Entry<K, V> entryAllowNullsOf(K key, V value) {
    return Maps.immutableEntry(checkNotNull(key), value);
  }
2)add 5 functions allowNullsOf(...) - just a copy of of(...), but inside used 
entryAllowNullsOf() instead of entryOf()

3) add final boolean allowNulls to Builder
   and make new constructor - Builder(boolean allowNulls)
   and modify default no-args constructor to call new one with false arg.
   modify Builder.put() to respect allowNulls flag

thats all

now if I really want allow nulls in immutableMap I can explicit call 
allowNullsOf() 
stuff.

If it can be accepted to be added to codebase, I can send a patch.

Original comment by kua...@gmail.com on 9 Oct 2009 at 5:21

GoogleCodeExporter commented 9 years ago
Hi limpbizkit,

I think whoever read the map should figure it out for themselves.
The Map cannot return 'i know there is such a key, and it's value is NULL'
and 'there is no such key' without throwing exception or 'undef'.

Original comment by jiaxiang...@gmail.com on 25 Jul 2014 at 3:28

GoogleCodeExporter commented 9 years ago
By electing to reject null values, you're saying, that you at Google know 
better everybody else.  Frankly, i don't really care what your code base at 
google looks like.  If you're going to publish open-source code, that you and 
your entry-level engineers at Google need to learn to play better with the rest 
of the world.

Original comment by emanuel....@gmail.com on 21 Jan 2015 at 10:23

GoogleCodeExporter commented 9 years ago
Here's a real-life example that's not legacy:
In our unit tests, we check that specific rows are present in a sql resultset.

assertContainsRow("select ID, FOO from BAR", 
ImmutableMap.<String,Object>builder()
  .put("ID", 5)
  .put("FOO", "qux").build());

If we want to test for null values we can't use the easy to read construction.

Original comment by ka...@vervaeke.info on 27 Feb 2015 at 12:10