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

ListCompareAlgorithm.AS_SET not working in combination with CustomComparator #832

Closed
buderjoh opened this issue May 7, 2019 · 7 comments
Closed

Comments

@buderjoh
Copy link

@buderjoh buderjoh commented May 7, 2019

If you use

JaversBuilder.javers().withListCompareAlgorithm(ListCompareAlgorithm.AS_SET)
.registerCustomComparator(new CustomBigDecimalComparator(2), BigDecimal.class);

The CustomBigDecimalComparator is not invoked.

JaversBuilder.javers().registerCustomComparator(new CustomBigDecimalComparator(2), BigDecimal.class);

is working and CustomBigDecimalComparator is invoked.

    Vehicle v = new Vehicle();
    VehicleCharacteristics c = new VehicleCharacteristics();
    c.setCo2Emission(BigDecimal.valueOf(0));
    v.setCharacteristics(c);

    Vehicle v2 = new Vehicle();
    VehicleCharacteristics c2 = new VehicleCharacteristics();
    c2.setCo2Emission(new BigDecimal("0.00"));
    v2.setCharacteristics(c2);

    Property p2 = ((Building) new Building().setSelf("B111")).setFloors(
            Arrays.asList(new Floor().setArea(BigDecimal.valueOf(3)), new Floor().setArea(BigDecimal.valueOf(7))));
    Property p4 = ((Building) new Building().setSelf("B111")).setFloors(Arrays
            .asList(new Floor().setArea(new BigDecimal("3.00")), new Floor().setArea(new BigDecimal("7.00"))));

    assertFalse(javers.compare(p2, p4))
bartoszwalacik added a commit that referenced this issue Sep 18, 2019
simple but slow implementation (n-squared)
bartoszwalacik added a commit that referenced this issue Sep 18, 2019
@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Sep 19, 2019

Fixed in 5.7.4

Added possibility to use CustomPropertyComparatortogether with ListCompareAlgorithm.AS_SET. A custom equals() methods is used to compare two Lists without paying attention to ordering and duplicates.

Warning! The list comparing algorithm would be slow in this case for large lists because it has n2 complexity

@buderjoh
Copy link
Author

@buderjoh buderjoh commented Sep 20, 2019

Thanks for fixing this issue.

Unfortunately it is not working in my case with Javers Version 5.7.4

I have my test case attached.
It should work independently so you can try it.

I am using the org.javers.core.diff.custom.CustomBigDecimalComparator.

The testWithSet is not working.
testWithoutSet is working just fine :)

testcase.txt

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Sep 21, 2019

@buderjoh
Copy link
Author

@buderjoh buderjoh commented Sep 27, 2019

The testcase is included in the branch -> https://github.com/buderjoh/javers/tree/javers-832

https://github.com/buderjoh/javers/blob/javers-832/javers-core/src/test/java/org/javers/core/examples/ListCompare.java

Both testcases should work...

The testWithSet is not working.
testWithoutSet works.

@bartoszwalacik bartoszwalacik reopened this Oct 4, 2019
@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Oct 4, 2019

Your second case is different. Issue happens when you compare List of ValueObjects using AS_SET and one of fields in this ValueObject is a CustomType.

Remember that CustomTypes don't have valid hashCode() so also your ValueObject (Floor) dosn't have a valid hashCode(). Comparing Sets without hashCode() would be very slow for large Sets (n-squared complexity)

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Oct 21, 2019

In 5.8.1, Custom comparators are reinvented, now your case should work. See https://javers.org/release-notes

@buderjoh
Copy link
Author

@buderjoh buderjoh commented Oct 22, 2019

It works thanks :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
2 participants