Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix ignoringFields with different map size #3003

Open
wants to merge 1 commit into
base: 3.x
Choose a base branch
from

Conversation

OlivierCavadenti
Copy link

Fix ignoringFields behavior that doesn't work properly when using maps of different sizes with usingRecursiveComparison and isEqualTo for example.

Fix #2988

Fix ignoringFields behavior that doesn't work properly when using
maps of different sizes with usingRecursiveComparison and isEqualTo for example.

Fix assertj#2988
@joel-costigliola joel-costigliola added this to the 3.25.0 milestone Mar 27, 2023
@@ -557,6 +559,15 @@ private static void compareUnorderedMap(DualValue dualValue, ComparisonState com
}
}

private static Map<?, ?> filterIgnoringFields(Map<?, ?> dualValue, ComparisonState comparisonState) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's rename dualValue to map since this is not a dualvalue

return dualValue;
}
return dualValue.entrySet()
.stream().filter(e -> !ignoredFields.contains(e.getKey()))
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename e to entry

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we are comparing the map key interpreted as field against the ignored fields, this is not entirely correct because we need to compare the field location (FieldLocation) to the ignored field.

Let's say we want to ignore the person.friends.address field, the code would only compare address so if it turns out that the root object as an address field then this will be ignored when it should not.

Moreover the recursive comparison supports ignoring fields by regex.

Have a look at matchesAnIgnoredFieldRegex and matchesAnIgnoredField in AbstractRecursiveOperationConfiguration

We should also add test cases in where we ignore for maps:

  • fields by regex
  • nested fields

@joel-costigliola
Copy link
Member

thanks @OlivierCavadenti, first round of review done!

@joel-costigliola
Copy link
Member

@OlivierCavadenti have you got time to finish this PR, if not, that's ok I can finish (you'll get full credit for it anyway 😉)

@scordio scordio added the theme: recursive comparison An issue related to the recursive comparison label Nov 12, 2023
@joel-costigliola
Copy link
Member

joel-costigliola commented Nov 12, 2023

I'm taking over this one @OlivierCavadenti as the fix is more complex than I thought, but you'll get credit 😉 .

The fix should check if the key is to be ignored according to the configuration, that includes:

  • ignored fields
  • ignored fields of type
  • compared types (if the value is not a compared type, it must be ignored)
  • compared fields (if the value is not a compared type, it must be ignored)

It is likely that we would need to compute the differences as if fields were not ignored but discard them when registering the diff as we do with the regular fields

@OlivierCavadenti
Copy link
Author

I'm taking over this one @OlivierCavadenti as the fix is more complex than I thought, but you'll get credit 😉 .

The fix should check if the key is to be ignored according to the configuration, that includes:

  • ignored fields
  • ignored fields of type
  • compared types (if the value is not a compared type, it must be ignored)
  • compared fields (if the value is not a compared type, it must be ignored)

Hi !
Thanks and sorry for not finish this one... !

@joel-costigliola joel-costigliola modified the milestones: 3.25.0, 4.0.0 Nov 26, 2023
@scordio scordio force-pushed the 3.x branch 2 times, most recently from 301ca01 to c730d18 Compare June 1, 2024 16:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
theme: recursive comparison An issue related to the recursive comparison
Projects
None yet
Development

Successfully merging this pull request may close these issues.

usingRecursiveComparison().ignoringFields("fieldName") is not working properly when maps sizes are different
3 participants