Do not create new map when testing for containsOnly and containsOnlyKeys for maps - #2160

comparing only the values is not enough, the following test should fail but succeeds with this implementation because all the keys have the same value:

Map<String, String> actual = new LinkedHashMap<>(2);
actual.put("a", "1");
actual.put("b", "1");
maps.assertContainsExactly(INFO, actual, entry("b", "1"), entry("a", "1"));

We could solve the problem if we were able to use the actual map way to compare keys to compare actualKeys[i] to entries[i].key but I'm not sure this is possible.

To be pragmatic I have created #2165 and I will remove the containsExactly changes from this PR when integrating (don't bother pushing a commit for that)

You are right, I didn't think about that use case. Thanks for removing the changes in the contains exactly method.

btw, it would be good to add the test case you shared in this commit to the codebase :)

good idea, I'll add it to the other tests

I'm not sure to see why we need to compare values when the assertion is only checking keys, am I missing something here?

I think we have the same issue as with containsExactly since we have to compare keys and we can't access actual's map key comparison strategy.

This test fails as it should but it does not report the correct error:

    actual = new CaseInsensitiveMap<>();
    actual.put("NAME", "green");
    actual.put("cool", "green");
    // THEN
    maps.assertContainsOnlyKeys(someInfo(), actual, "Name", "Color");

error is:

Expecting actual:
  {"cool"="green", "name"="green"}
to contain only following keys:
  ["Name", "Color"]
keys not found:
  ["Color"]
and keys not expected:
  ["name"]

"keys not found" is correctly reported but "keys not expected" should have shown "cool", it reports "name" instead which actually is a map key.

I guess the only way forward is to find a way to clone actual's map which might be possible using introspection to call a a no arg constructor and then populate the cloned map. Question is now: is there always a no arg constructor ...

You are right. What we can also do is to support sime well known case insensitive maps behind the scenes, and for everything else create a normal copy. Reusing constructors might lead to using unmodifiable / immutable maps. I can give it another go on Sunday afternoon, no access to a laptop before that

I guess the only way forward is to find a way to clone actual's map which might be possible using introspection to call a a no arg constructor and then populate the cloned map. Question is now: is there always a no arg constructor ...

Objenesis could deal with it and I think it would be fine to have it as a shaded dependency. However, unmodifiable / immutable maps might still be an issue as @filiphr mentioned.

I was thinking we could try to instantiate the map (I'm fine using Objenesis) and if it fails for whatever reason use a simple LinkedHashMap as in the existing implementation, that won't completely solve the issue but will work for a vast majority of use cases, finally we would mention this limitation in the javadoc.

I have tried Objenesis and that did not go well with CaseInsensitiveMap, creating the instance worked but would not initialize transient HashEntry<K, V>[] data; which is accessed later on when putting actual entries in the new map.

Interestingly it works with ConstructorInvoker (some old internal class in our codebase) and it worked.

Here's the stack trace for the record:

java.lang.NullPointerException
	at org.apache.commons.collections4.map.AbstractHashedMap.ensureCapacity(AbstractHashedMap.java:627)
	at org.apache.commons.collections4.map.AbstractHashedMap._putAll(AbstractHashedMap.java:325)
	at org.apache.commons.collections4.map.AbstractHashedMap.putAll(AbstractHashedMap.java:304)
	at org.assertj.core.internal.Maps.compareActualMapAndExpectedKeys(Maps.java:823)