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

Problem with inheritance? #127

Closed
bfeater opened this Issue Mar 16, 2015 · 19 comments

Comments

Projects
None yet
4 participants
@bfeater

bfeater commented Mar 16, 2015

I tried out JaVers (version 1.1.0), but encountered a problem. Comparing a property with a value of different subclasses leads to an IllegalArgumentException. Is this intended behaviour?

Example to reproduce the exception:

   class Example {
      class Holder {
         A a;
      }

      class A {}

      class B0 extends A {
         String b0 = "asdf";
      }

      class B1 extends A {
         String b1 = "qwer";
      }

      public void doesNotWork() {
         Javers javers = JaversBuilder.javers().build();

         Holder first = new Holder();
         first.a = new B0();
         Holder second = new Holder();
         second.a = new B1();

         Diff diff = javers.compare(first, second);
      }
   }

@bartoszwalacik bartoszwalacik self-assigned this Mar 16, 2015

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 16, 2015

Looks like a bug, will check it out

@bartoszwalacik bartoszwalacik added bug question and removed bug labels Mar 16, 2015

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 20, 2015

There is no bug here, but indeed, error message is misleading. It will be fixed in the next release to TYPE_MISMATCH error

What you are doing here is comparing objects with different types.
You cannot compare B0 instance with B1 instance because they have different fields.

Field String b0 is not connected to String b1 despite the same type (String).
It will work only if you abstract the field definition to A class

bartoszwalacik added a commit that referenced this issue Mar 21, 2015

Merge pull request #134 from javers/#127
better handling of TypeMismatch Exception
@bfeater

This comment has been minimized.

bfeater commented Mar 22, 2015

Do you really mean that? Perhaps another example will make the problem clear

class Store {
List bicycles;
}

class Bicycle {
int speed;
}

class Mountenbike {
int seatHeight;
}

...
Store store0 = new Store();
store0.bicycles = new ArrayList<>();
store0.bicycles.add(new Bicycle());

Store store1 = new Store();
store1.bicycles = new ArrayList<>();
store0.bicycles.add(new Mountenbike());

Comparing store0 with store1, I would expect a ListChange with a removal
of a Bicycle and an addition of a Mountenbike. But in the current
implementation of JaVers I get an exception.

Am 20.03.2015 um 23:08 schrieb Bartosz Walacik:

There is no bug here, but indeed, error message is misleading. It will be fixed in the next release to TYPE_MISMATCH error

What you are doing here is comparing objects with different types.
You cannot compare B0 instance with B1 instance because they have different fields.

Field String b0 is not connected to String b1 despite the same type (String).
It will work only if you abstract the field definition to A class


Reply to this email directly or view it on GitHub:
#127 (comment)

Sebastian Gebhardt
Email: sebastian.gebhardt@bfeater.de
PGP-Public Key: http://www.bfeater.de/bfeater_pubkey.asc

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 22, 2015

Could you please push a mini-project to github with a failing test?

@bfeater

This comment has been minimized.

bfeater commented Mar 22, 2015

I have created a small example. In the test I have added a comment with my expected result.

https://github.com/bfeater/javers_inheritance_test

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 23, 2015

ok, will take a look

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 23, 2015

in the 1.1.2-SNAPSHOT, you will get better exception desc:

JaversException: TYPE_MISMATCH JaVers runtime error - Looks like you are comparing two objects with different types. There is no property 'seatHeight' in type 'Bicycle'.

the clue is the same, we don't know how to compare Bicycle with Mountainbike.
If the number of gears (common field) is different, that is clear that objects are different.
But what if the number of gears would be equal? Does it mean that objects are equal? Not sure ...

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 23, 2015

In fact, when you start adding fields to a subclass, it's not possible to write valid equals(), without breaking Liskov Principle. Try to write a valid equals() for both classes (Bicycle with Mountainbike), I mean reflexive, symmetric, transitive and consistent. Imho it's not possible.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Mar 29, 2015

I can think about simple solution, when comparing two ValueObjects, Javers would say not equals if these two objects have different number of fields. What do you think about that? Would it solve your issue?

@bfeater

This comment has been minimized.

bfeater commented Mar 31, 2015

In my opionion both equals() methods (Bicycle and Mountainbike) are correct. When the objects are not of the same class, equals() returns false. And I think Javers should behave in the same way. Objects of different classes - also they have equals global ids - cannot be equals.

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Apr 2, 2015

In fact, JaVers doesn't use users' equals() methods for ValueObjects. JaVers compares ValueObjects field-by-field. Personally I prefer using instanceof instead of getClass(), as it does less harm to Liskov Principle. But it does realy matter here

I can change ValueObject comparing algorithm in that way, that it would retrun false when both object have different set of properties. This would solve your case as Bicycle and Mountainbike have different fields. What do you think about it?

@bfeater

This comment has been minimized.

bfeater commented Apr 2, 2015

Sure, this would solve my problem.

But in case someone wants to compare two objects having different
classes but same set of properties, he would ran into the same problem.
So I would really suggest to compare the classes of two value objects.
When the classes are equal, it's guaranteed that both objects have the
same set of properties.

In fact, JaVers doesn't use users' equals() methods for ValueObjects. JaVers compares ValueObjects field-by-field. Personally I prefer using instanceof instead of getClass(), as it does less harm to Liskov Principle. But it does realy matter here

I can change ValueObject comparing algorithm in that way, that it would retrun false when both object have different set of properties. This would solve your case as Bicycle and Mountainbike have different fields. What do you think about it?


Reply to this email directly or view it on GitHub:
#127 (comment)

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented Apr 2, 2015

dont worry, new impl would be type-safe

concrete class should not matter, think of a simple example:
new ArrayList().equals( Collections.emptyList() )
gives true, despite that both object have different classes

@bgalek

This comment has been minimized.

Contributor

bgalek commented May 8, 2015

our plan:

  1. chcek if both objcets are from the same hierarchy (instance of)
  2. check if field count is equal - then it's a valid comparison in other case - return false
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented May 15, 2015

finally I did sth different.
In fact VO are compared property-by-property, ant not by any kind of equals() method.
So if a property is present in the left VO and missing in the right VO, ValueChange is created with null on the right.

see #153

bartoszwalacik added a commit that referenced this issue May 17, 2015

Merge pull request #153 from javers/#127
#127 "should compare ValueObject with its subclass even if subclass has more fields"
@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented May 18, 2015

done & released, see v 1.2.1

@heroInCommunity

This comment has been minimized.

heroInCommunity commented May 29, 2018

@bgalek please tell, if second option will work when field count is different, however @DiffIgnore was used?
See my question https://stackoverflow.com/questions/50527145/compare-objects-of-child-and-parent-classes-in-javers .

@bgalek

This comment has been minimized.

Contributor

bgalek commented May 29, 2018

@bartoszwalacik will know the implementation details ;)

@bartoszwalacik

This comment has been minimized.

Member

bartoszwalacik commented May 30, 2018

I will answer this question at stack

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment