Thanks for reporting this @tkruse!

I was looking for something easy to do, and this looks pretty easy. Can I take this one?

Thanks @etellman, it should not be to complicated since it is only related to maps.

Cheers

I looked at this a bit, and am now questioning whether it is worth doing. I'm pretty confident that a null entry doesn't make sense and the only time it would come up is if there is a bug in the map implementation, which seems like a pretty obscure case.

Here's what I looked

The change to StandardRepresentation is straightforward:

  protected String toStringOf(Map<?, ?> map) {
    ...

      if (entry == null) {
        builder.append("null");
      } else {
        builder.append(format(map, entry.getKey())).append('=').append(format(map, entry.getValue()));
      }

    ...
  }

To get "containsKey" to work, internal/Maps.java needs a change as well:

  private static <K> boolean containsKey(Map<K, ?> actual, K key) {
    try {
      return actual.containsKey(key);
    } catch (NullPointerException e) {
      return false;
//      if (key == null) return false; // null keys not permitted
//      throw e;
    }
  }

AbstractMap throws a NPE when looking through the entry set for the key, when the entry set contains null.

Without this, this test still throws a NPE:

  @Test
  public void testContainsKeyWithNullEntry() {
    Map<String, Boolean> actual = new NullEntryMap();

    AssertionError error = catchThrowableOfType(() -> assertThat(actual).containsKey("notPresent"), AssertionError.class);
    assertThat(error).hasMessageContaining("null");
  }

But even with this change, this test fails (with the key "present" hard coded to be in the entry set):

  @Test
  public void testDoesNotConainKeyWithNullEntry() {
    Map<String, Boolean> actual = new NullEntryMap();

    AssertionError error = catchThrowableOfType(() -> assertThat(actual).doesNotContainKey("present"), AssertionError.class);
    assertThat(error).hasMessageContaining("null");
  }

That key is actually in the entry set, but entry sets containing null entries are now basically the same as empty entry sets, so this one fails because no AssertionError gets thrown.

Given that this change seems to be just trying to cover up a bug in the map implementation, I'm thinking that it would be better to leave things as they are now. The way it is now, you'll get a NPE and can go fix your custom map.

What do you think?

We only want to fix the NPE in the StandardRepresentation, I would not change the behavior of containsKey as part of this issue, this could be discussed in a different ticket if it is based on an actual use case.

I probably didn't explain this clearly, but I don't think there should make any change for this. The code as written now seems fine to me.

It's plausible that for a particular pair in the entry set, the key and/or value is null. I think these cases are already handled by AssertJ. It isn't plausible for the entry set to contain a null key/value pair, as there isn't anything reasonable that this would mean. If there's a null value in the entry set, the user's map implementation has a bug and they should fix it.

When I created a trivial map implementation with this problem, containsKey() threw a NPE. AbstractMap assumes that the entry set will never contain a null value. A map that extends AbstractMap will probably throw NPE from a various places if it has an entry set that contains null. We could add a work-around for null values in entry sets in StandardRepresentation but the test may get a NPE before AssertJ tries to produce the string representation.

I thought that instead of changing anything, we should just leave the code as it is now. If the user's map implementation returns entry sets with null in them, they'll probably get a NPE from one of the Map methods or, if the test makes it that far, from StandardRepresentation trying to create a string representation of the map. This is good, because then the user will find out that their map has a problem and can go fix it.

I definitely wouldn't change the behavior of containsKey. That would hide any NPE from a map, converting them all into "key not present".

An alternative would be to have StandardRepresentation throw an IllegalStateException (or similar) with a helpful error message if there's a null entry in the entry set. That might be more useful than masking the issue by adding "null" to the string representation of the entry set.

Let me know what you think. If you still want to change StandardRepresentation to include "null" in the string description of the map or throw an exception with a more helpful message, they're both easy changes to make. I was just questioning whether either was the right thing to do.

Here's a pull request for handling null entries when formatting the map, since this seems likely to be the preferred solution: #3110.

This doesn't try to make anything else handle null values in entries, so it's still possible that a map with this behavior will result in an exception being thrown before it's converted to a string.