extracting should throw a proper assertion error if actual is null

[scordio]

Summary

Based on #2392 (comment) and further comments, the implementation of extracting(Function, AssertFactory) and extracting(String, AssertFactory) in AbstractAssert should be improved to ensure that a preliminary isNotNull check is performed.

Example

The following assertions:

assertThatObject(null).extracting(Object::getClass);
assertThatObject(null).extracting("class");

should fail with:

java.lang.AssertionError: 
Expecting actual not to be null

instead of failing with:

java.lang.NullPointerException
	at org.assertj.core.api.AbstractAssert.extracting(AbstractAssert.java:1016)
	at org.assertj.core.api.AbstractObjectAssert.extracting(AbstractObjectAssert.java:953)
	...

and:

java.lang.IllegalArgumentException: The object to extract property/field from should not be null
	at org.assertj.core.util.Preconditions.checkArgument(Preconditions.java:129)
	at org.assertj.core.util.introspection.PropertyOrFieldSupport.getValueOf(PropertyOrFieldSupport.java:49)
	at org.assertj.core.extractor.ByNameSingleExtractor.apply(ByNameSingleExtractor.java:29)
	at org.assertj.core.api.AbstractAssert.extracting(AbstractAssert.java:990)
	at org.assertj.core.api.AbstractObjectAssert.extracting(AbstractObjectAssert.java:834)
	...

respectively.

[scordio]

@bjhargrave as you used extracting in the past for custom assertions (#1781 (comment)), do you see any issues if we add an isNotNull() call to the extracting methods of AbstractAssert?

[bjhargrave]

do you see any issues if we add an isNotNull() call to the extracting methods of AbstractAssert?

No. In all the cases were we use extracting in osgi-test, we have already called isNotNull directly or indirectly first.

For example: https://github.com/osgi/osgi-test/blob/659f5c7fe6c57cb194e3a511f1f4e726eceed166/org.osgi.test.assertj.framework/src/main/java/org/osgi/test/assertj/bundle/AbstractBundleAssert.java#L93-L96

cc: @kriegfrj

[scordio]

Thanks a lot for the feedback!

[kriegfrj]

I concur with @bjhargrave.

I think it makes good sense to do the null check as part of extracting() as you won't be able to extract anything useful if actual is null (I remember having the fleeting thought that this feature should be added but wasn't able to sustain the thought long enough to make an issue report or PR about it! 😄). As BJ said we are working around this in osgi-test by doing it manually, but it would some boilerplate for us (and for all users) if we didn't have to work around it.

While strictly speaking this is not a backward-compatible change, I think practically speaking it won't cause any compatibility issues because there aren't really any legitimate use cases for calling extracting() when you know that actual is null. Most code out there will either never test cases where actual might be null, or else will be (should be?) doing their own defensive checks like we are doing. So I think we are safe to make the change from a compatibility perspective.