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

CustomPropertyComparator should be used compare Collection items #747

Closed
bartoszwalacik opened this issue Nov 25, 2018 · 5 comments
Closed

CustomPropertyComparator should be used compare Collection items #747

bartoszwalacik opened this issue Nov 25, 2018 · 5 comments

Comments

@bartoszwalacik
Copy link
Member

@bartoszwalacik bartoszwalacik commented Nov 25, 2018

Currently, CustomPropertyComparator is used only for comparing object properties, for example, it can be used to compare Person objects in boss property:

class Company {
        private Person boss
        private List<Person> partners
}

But it is not used by JaVers to compare Person objects when they are collection items in the partners List.

Possible solution is a new method in CustomPropertyComparator with default impl:

   /**
     * This comparator is called by JaVers to calculate collection-to-collection diff.
     * <br/>
     * For example, when objects of type T are List items.
     * <br/><br/>
     * 
     * Both equals() and compare() should return consistent results. When compare() returns null,
     * equals() should return false.
     * 
     */
    default boolean equals(T a, T b) {
        return Objects.equals(a, b);
    }
bartoszwalacik added a commit that referenced this issue Nov 25, 2018
bartoszwalacik added a commit that referenced this issue Nov 25, 2018
@bartoszwalacik
Copy link
Member Author

@bartoszwalacik bartoszwalacik commented Nov 27, 2018

released in 4.0.0-RC3

@baumgartner
Copy link

@baumgartner baumgartner commented Dec 3, 2018

Works with ListCompareAlgorithm.SIMPLE
Assert fails for ListCompareAlgorithm.AS_SET and LEVENSHTEIN_DISTANCE.
Test-Project available at https://github.com/baumgartner/javerstest/blob/master/src/test/java/javers/test/ListCompareTest.java

@bartoszwalacik
Copy link
Member Author

@bartoszwalacik bartoszwalacik commented Dec 3, 2018

omg, so we need a fix ...

bartoszwalacik added a commit that referenced this issue Dec 3, 2018
failing test
bartoszwalacik added a commit that referenced this issue Dec 5, 2018
#746
fixes
@bartoszwalacik
Copy link
Member Author

@bartoszwalacik bartoszwalacik commented Dec 5, 2018

Fixed in 5.0.1 when CustomPropertyComparator is used with LEVENSHTEIN.
CustomPropertyComparator can't be used with AS_SET because for comparing Sets, we need both equals() and hashCode() and CustomPropertyComparator implements only custom equals()

@dmitry-weirdo
Copy link

@dmitry-weirdo dmitry-weirdo commented Nov 25, 2020

@bartoszwalacik any perspective to also be able to use CustomPropertyComparator together with CustomPropertyComparator?

I described my case on #1038.

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
3 participants