allow nulls in EntryStream.toMap() and similar

[mmariotti]

Hello,
I understand the reason, but I think this is too limitating.
Even when I'm dealing with parallel streams, I'm not sure it's useful: ConcurrentHashMap already throws NullPointerException if any key or value is null (by doc).
When dealing with sequential streams instead, I see no straight way collecting to a null-tolerant map.

So, I want to propose some solution:

1. I'd prefer to have this limitation totally removed
2. remove the limitation only for EntryStream.toCustomMap()
3. create something like EntryStream.toNullTolerantMap()

Thank you!

[xiumeteo]

Why do you want to have a null key or value? Just curious about

[mmariotti]

Here you are some examples:

Map<String, String> eventMap = StreamEx.of(events)
	.mapToEntry(TimeEvent::getId, TimeEvent::getParameter)
	.mapValues(x -> context.resolve(x, true))
	.toMap(); // if param is resolved to null, boom
	
	
Map<String, Object> stateMap = StreamEx.ofTree(node, x -> StreamEx.of(x.getChildren()))
	.mapToEntry(Node::getPath, NodeState::new)
	.filterValues(NodeState::isLoaded)
	.toMap(); // root node has null path, boom

So, when I have full control on code, in particular on the functions that use the returned map, the problem can be workarounded:

Map<String, Optional<String>> eventMap = StreamEx.of(events)
	.mapToEntry(TimeEvent::getId, TimeEvent::getParameter)
	.mapValues(x -> Optional.ofNullable(context.resolve(x, true)))
	.toMap();

Map<Optional<String>, NodeState> stateMap = StreamEx.ofTree(node, x -> StreamEx.of(x.getChildren()))
	.mapToEntry(x -> Optional.ofNullable(x.getPath()), NodeState::new)
	.filterValues(NodeState::isLoaded)
	.toMap();

Nevertheless, most of times these maps are used for serialization/marshalling/persistence, so I can't use non-pojo objects (and Optional is an immutable non-serializable monad).
Then I'm forced to:

Map<String, String> eventMap = new HashMap<>();
StreamEx.of(events)
	.mapToEntry(TimeEvent::getId, TimeEvent::getParameter)
	.mapValues(x -> context.resolve(x, true))
	.forKeyValue(eventMap::put);
	
Map<String, NodeState> stateMap = new HashMap<>();
StreamEx.ofTree(node, x -> StreamEx.of(x.getChildren()))
	.mapToEntry(Node::getPath, NodeState::new)
	.filterValues(NodeState::isLoaded)
	.forKeyValue(stateMap::put);

Even if this is not a great issue, it's still annoying. EntryStream.toMap and EntryStream.into are essentially precluded.

I understand, theoretically speaking, the java 8 filosophy against nulls, but in practice nulls spread in every single piece of code, and in most cases are unavoidable just because null it's a language property.
Furthermore, most of Map implementations accept both null keys and values; IMHO the strict behavior of Collectors.toMap is overwhelming and a conceptual bug: null tolerance should depend on the map implementation, and should not be a restriction by itself.

[amaembo]

Note that toMap() does not guarantee the serializability of the returned Map. For toCustomMap it's probably reasonable to remove the limitation.

[mmariotti]

Right, you stated it in doc. But I suppose it's a preemptive statement, probably conservative for future changes, since toMap returns either HashMap or ConcurrentHashMap, which are both Serializable.

Removing the limitation on toCustomMap would be greatly appreciated!

[landawn]

@mmariotti I have read the answers on : Java 8 NullPointerException in Collectors.toMap
Ask and How should we manage jdk8 stream for null values. But those answers about how null should be handled in Stream APIs for this specific case don't make scenes to me. So I forked and made changes to totally remove the limitation at landawn/streamex

[mmariotti]

On a first glimpse, I see that EntryStream.toMapConsumer has not changed. This will prevent null values.

[amaembo]

After rethinking I doubt that modifying even toCustomMap is a great idea. In this case we cannot use Map.putIfAbsent to add a new value, because it will silently overwrite if repeating value appears. E.g. EntryStream.of(1,null,1,"bar").toCustomMap(HashMap::new) will write (1, null), then silently overwrite it with (1, "bar"). Instead we would need to make a containsKey test making the operation more expensive in general case (two key lookups per addition), just to support a corner case. Also this will not work for concurrent maps. I don't know whether there's a null-friendly concurrent map in the world (the ConcurrentMap interface does not explicitly disables it). However if it is, then Map interface certainly does not provide a way to atomically avoid an update of null-valued entry. So correct imlementation will be a) slower b) not parallel-friendly.

Another problem is the merging version when custom merger is provided. Merging typically is delegated to Map.merge which also treats nulls as absent values by specification. So if we allow null values, merging will be inconsistent (merge function will not be called if null and not-null values are about to be merged). To make merging consistent we will need to reimplement merge manually, which again violates atomicity for concurrent collection and makes the operation slower for non-concurrent.

An end-user alternative would be to create a Map<K, Optional<V>> if you need to represent an absent value. This could be done in very simple way adding mapValues(Optional::ofNullable) step. The resulting Map type declaration explicitly says that absent values are possible there and it will be much less error-prone to read such Map. I believe, there are tons of subtle errors in the Java code when people assume that null value is equal to an absent value (like (if(map.get(key) == null)), but this assumption is wrong.