Allow ComparingNormalizedFields instances to be reused across different assertions

[genuss]

Hi, team!

After upgrading to 3.25.3 I see that ComparingNormalizedFields RecursiveComparisonIntrospectionStrategy can't be between different assertions (see my example). I digged into the code and found that the change was introduced in commit 0d5b789, related to some performance improvements. The root cause is that originalFieldNamesByNormalizedFieldNameByNode is an identity hash map, although fieldNamesPerClass is keyed by class values. So, I found a fix for that, which I present here. I'm not sure that it's 100% correct, but I didn't find any failing tests. Hope you may find this helpful.

package test;

import org.assertj.core.api.Assertions;
import org.assertj.core.api.recursive.comparison.ComparingNormalizedFields;
import org.assertj.core.api.recursive.comparison.RecursiveComparisonConfiguration;
import org.junit.jupiter.api.Test;

class AssertjBugTest {

  @Test
  void test() {
    Assertions.assertThat(new Actual(0))
        .usingRecursiveComparison(
            RecursiveComparisonConfiguration.builder()
                .withIntrospectionStrategy(SomeIntrospectionStrategy.INSTANCE)
                .build())
        .isEqualTo(new Expected(0));
    Assertions.assertThat(new Actual(0))
        .usingRecursiveComparison(
            RecursiveComparisonConfiguration.builder()
                .withIntrospectionStrategy(SomeIntrospectionStrategy.INSTANCE)
                .build())
        .isEqualTo(new Expected(0));
  }

  record Actual(int a) {}

  record Expected(int A) {}
}

class SomeIntrospectionStrategy extends ComparingNormalizedFields {

  static final SomeIntrospectionStrategy INSTANCE = new SomeIntrospectionStrategy();

  @Override
  protected String normalizeFieldName(String fieldName) {
    return fieldName.toLowerCase();
  }
}

Check List:

• Unit tests : YES
• Javadoc with a code example (on API only) : NA

[scordio]

Thanks for reporting it and providing a fix, @genuss! We'll check it and get back to you.

[joel-costigliola]

thanks @genuss, I'll look at it next week

[joel-costigliola]

I had a quick look and there was indeed a bug in getChildrenNodeNamesOf which tried to be clever and not renormalize fields for the same class twice, but it turns out that the fields normalization also populates originalFieldNamesByNormalizedFieldNameByNode and this needs to be done for each nodes.

The code is a bit crap because getChildrenNodeNamesOf has some side effects and is not performant as it calls Objects.getFieldsNames(node.getClass()); which is expensive whereas normalizing fields names is not.

I have a fix but it needs a bit more love and I'll see if I want to cache Objects.getFieldsNames(node.getClass()) operation as it is expensive.

[joel-costigliola]

Integrated thanks @genuss !

[genuss]

Very good, thank you too!