Assertj-core: FieldUtils.readField returns static fields

[henri-tremblay]

Summary

The issue came from hasFieldOrPropertyWithValue. FieldUtils.readField will consider values from static fields to be what we are looking for. I think static fields should be ignored.

This behavior is new. This test used to work in 3.14 and doesn't in 3.17.

Example

public class A {
  public static final String KEY = "KEY"

  Map<String, String> values = new HashMap<>();

  public String getKey() { // Not called because we will look for getKEY and not getKey. That's fine
     return values.get(KEY);
  }
}

A a = new A();
a.values.put(KEY, "x");

assertThat(a).hasFieldOrPropertyWithValue(A.KEY, "x"); // A.KEY is matched instead of the value in the map.

[scordio]

Hi @henri-tremblay, I would not expect assertThat(a).hasFieldOrPropertyWithValue(A.KEY, "x") to match the value in the map because values is a field of A. The behavior you mentioned can happen if A implements Map, see:
https://github.com/joel-costigliola/assertj-core/blob/3c6bb8a5d4f452c60cf3c8e336a03feba5e9b207/src/main/java/org/assertj/core/util/introspection/PropertyOrFieldSupport.java#L74-L78

Or did I misunderstand your example?

I tried to run the example on 3.14 and 3.17.2, both fail with:

  <com.example.A@18bc90ba>
to have a property or a field named <"KEY"> with value
  <"x">
but value was:
  <"KEY">

Ignoring static fields could be a separate topic, but right now I cannot see how the assertion would verify the value of values.get(KEY).

[henri-tremblay]

A does implement map in my case. But it goes out before that.

Let me get back with a full example. I apologize, I did it a bit too quickly from an actual issue I had in my code.

[scordio]

I was able to reproduce the case you mentioned, the root cause is probably #1763.

I am not sure we should entirely ignore static fields as this could prevent the verification of non public values.
We currently verify values in the following order:

1. input name as property
2. input name as field
3. input name as map key, if the object is an instance of Map

Maybe it could be reasonable to ignore static fields during 2 and try them only if 3 failed. @joel-costigliola thoughts?

[joel-costigliola]

I think we should ignore static fields by default, hasFieldOrPropertyWithValue was meant to check instance's fields or properties.
If people want to check static fields we could introduce hasStaticFieldWithValue in class assertions.

[henri-tremblay]

I agree with @joel-costigliola.

[scordio]

My only concern is that FieldUtils.readField is a core element and the change of the behavior will affect other features (extracting and recursive comparison configuration, for example).

Or we would need to do a bigger refactoring to make sure only hasFieldOrPropertyWithValue is changed.

[joel-costigliola]

For recursive comparison, the intent was not to check static fields, for extracting I would argue the same.
It is a breaking change for sure but I feel it's a reasonable one as the initial goals was not to check static fields.
@scordio WDYT?

[scordio]

I agree, static field verification is really a corner case. Ok, I can drop the bad 👮‍♂️ hat!

[joel-costigliola]

Fixed in https://github.com/assertj/assertj-core/commit/2f3eeb9f597dd162b06283345a07f083fcf5b3fa.
I also ignored synthetic fields