Assertj-core: Assertions ignore missing entry on maps

[rotty3000]

Summary

A custom assertion extending MapAssert that performs a org.assertj.core.api.AbstractObjectAssert.hasFieldOrProperty(String) on an actual that neither has a getter or field, but _is_ a Map silently ignores missing map entries.

Example

Given JSON:

{
  "startLevel" : 1,
  "initialBundleStartLevel" : 1
}

The following will silently succeed

net.javacrumbs.jsonunit.assertj.JsonAssertions.assertThatJson(
    json
).isObject().hasFieldOrProperty(
    "lastModified"
);

Reason

The method org.assertj.core.util.introspection.PropertyOrFieldSupport.getSimpleValue(String, Object) throws in the case of propertySupport not having a getter method, and fieldSupport not finding a field. However in the case of the Map fallback it does not fail for a missing entry.

    // try to get name as a property, then try as a field, then try as a map key
    try {
      return propertySupport.propertyValueOf(name, Object.class, input);
    } catch (IntrospectionError propertyIntrospectionError) {
      // no luck as a property, let's try as a field
      try {
        return fieldSupport.fieldValue(name, Object.class, input);
      } catch (IntrospectionError fieldIntrospectionError) {
        // neither field nor property found with given name

        // if the input object is a map, try name as a map key
        if (input instanceof Map) {
          Map<?, ?> map = (Map<?, ?>) input;
          return map.get(name);
        }

        // no value found with given name, it is considered as an error
        String message = format("%nCan't find any field or property with name '%s'.%n" +
                                "Error when introspecting properties was :%n" +
                                "- %s %n" +
                                "Error when introspecting fields was :%n" +
                                "- %s",
                                name, propertyIntrospectionError.getMessage(),
                                fieldIntrospectionError.getMessage());
        throw new IntrospectionError(message, fieldIntrospectionError);
      }
    }

[scordio]

Thanks for the detailed report @rotty3000.

is there a particular reason why you are using hasFieldOrProperty instead of containsKey? The contains* API is also demonstrated in JsonUnit's examples.

Still, it is interesting that AbstractMapAssert is inheriting from AbstractObjectAssert, exposing assertions from the latter. I would imagine those assertions don't make much sense with maps.

Playing around:

assertThat(new Object()).hasFieldOrProperty("lastModified"); // fails

assertThat(new HashMap<>()).hasFieldOrProperty("lastModified"); // succeeds

assertThat(new HashMap<>()).containsKey("lastModified"); // fails

@assertj/core should we consider a review of the class hierarchy in this area? My current feeling would be that AbstractMapAssert should inherit from AbstractAssert only.

[rotty3000]

Thank you for the insight @scordio. I really appreciate it.

However, from a outsiders perspective looking at Json "objects" it's not clear that the underlying structure is a Map (even though that is the obvious implementation) and so the nomenclature that a JSON "object" has a _property_ of _field_ is far more natural than it having a "key". And so in this sense I feel that, if the API is there, it should work as expected. Don't you agree? I don't think the developer of JsonUnit's assertj support would disagree :)

[rotty3000]

I'd like to propose an implementation of MapSupport akin to FieldSupport and PropertySupport having a property allowMissingKey where the default is true and this is the default in the EXTRACTING use case. Meanwhile in the comparison use case it could be set to false.

[scordio]

Thank you for the insight @scordio. I really appreciate it.

However, from a outsiders perspective looking at Json "objects" it's not clear that the underlying structure is a Map (even though that is the obvious implementation) and so the nomenclature that a JSON "object" has a _property_ of _field_ is far more natural than it having a "key". And so in this sense I feel that, if the API is there, it should work as expected. Don't you agree? I don't think the developer of JsonUnit's assertj support would disagree :)

I agree that the external layer focuses on the JSON, hencehasFieldOrProperty might sound better here. However, assertj-core has no knowledge about the JSON type, that is just a map. The JSON type is the target of JsonUnit.

Having said that, if we would decide to change the API exposed by AbstractMapAssert, I would definitely brainstorm together with @lukas-krecan in order to keep a meaningful API for JsonUnit users.

Let's see what the rest of the team thinks about it.

[rotty3000]

So, I wanted to add that after further consideration I think I'm probably OK with using the contains(entry(name, value)), containsKey(name) nomenclature. They seems to handle the cases I need well enough. However, I do still find it strange that hasFieldOrPropertyWithValue(name, value) seems to work very well while hasFieldOrProperty(name) does not work reliably.

I suppose it just caught me off guard when I realized after having written several positive tests and then a negative test only to realize the negative case wasn't failing. That forced me to go back and reassess every test I had written to make sure I didn't accidentally have a false positive case hiding in the weeds.

[joel-costigliola]

hasFieldOrPropertyWithValue(name, value) and hasFieldOrProperty(name) should be consistent and fail if there is no field or property or key with the given name.

My current feeling would be that AbstractMapAssert should inherit from AbstractAssert only.

That would be a breaking change for sure, I'll try to find why we have it, I don't remember right now.

[filiphr]

@rotty3000 and the AssertJ team. Doesn't it make more sense to request the json-unit team to look into this. Calling that isObject actually returns a JsonMapAssert from json-unit. I think it would make more sense address and improve the API from json-unit here.

[joel-costigliola]

Fair point @filiphr, I did not realize it was returning a JsonMapAssert.
But still, we should be consistent between hasFieldOrPropertyWithValue and hasFieldOrProperty

[lukas-krecan]

There are two issues. One is that hasFieldOrPropertyWithValue(name, value) and hasFieldOrProperty(name) are inconsistent and their behavior is surprising for MapAssert.

The other is that JsonUnit may lead user to use them. I will not be able to help with the first, but I can take a look at JsonUnit issues. I may mark the methods as deprecated or fix their behavior.

[scordio]

Doesn't it make more sense to request the json-unit team to look into this.

That was my initial feeling, but we might also have a conceptual issue with the current AbstractMapAssert parent class.

That would be a breaking change for sure, I'll try to find why we have it, I don't remember right now.

Yes, definitely. If we realize this is the way to go, I would deprecate AbstractObjectAssert's methods in AbstractMapAssert, similarly to what @lukas-krecan proposed for JsonUnit, and we could change the inheritance in version 4.

we should be consistent between hasFieldOrPropertyWithValue and hasFieldOrProperty

I'll look into it.

[scordio]

I do still find it strange that hasFieldOrPropertyWithValue(name, value) seems to work very well while hasFieldOrProperty(name) does not work reliably.

@rotty3000 do you have an example where hasFieldOrPropertyWithValue(name, value) is working correctly with a Map?

I played a bit more and both assertions below are erroneous but "consistent" 🙂

assertThat(new HashMap<>()).hasFieldOrProperty("lastModified"); // succeeds
assertThat(new HashMap<>()).hasFieldOrPropertyWithValue("lastModified", null); // succeeds

Also, trying:

assertThat(new HashMap<>()).hasFieldOrPropertyWithValue("lastModified", "value"); // fails

yields:

java.lang.AssertionError: 
Expecting
  {}
to have a property or a field named "lastModified" with value
  "value"
but value was:
  null
(static and synthetic fields are ignored)

From my point of view, both hasFieldOrProperty and hasFieldOrPropertyWithValue when actual is a Map should be fixed.