Project

General

Profile

bug #7155

VersionableEntity.equals() should take updated timestamp into account

Added by Andreas Kohlbecker almost 2 years ago. Updated over 1 year ago.

Status:
Closed
Priority:
Highest
Category:
cdmlib
Target version:
-
Start date:
01/08/2018
Due date:
% Done:

0%

Severity:
normal
Found in Version:
Tags:

Description

The equals() as implemented in VersionableEntity is just a copy of the equals in CdmBase. It does not implement further functionality except of a null check of the UUID. That is, it completely misses comparing the updated timestamps. This however is crucial, since otherwise for example an older entity derived from the auditing tables would be equal to the current version of the entity.

I stumbled over this in the context of the vaadin com.vaadin.ui.AbstractField.setValue(T newFieldValue, boolean repaintIsNotNeeded, boolean ignoreReadOnly) method in which the equals comparison is used to determine if the supplied newFieldValue is different from the current field value. Since VersionableEntity.equals() is ignoring the updated timestamp updated cdm entities can not be set as field value in order to update the UI with the new persistence state. The obvious workaround to first set the field value to null is awkward and not without nasty side effects.

Most lines of the VersionableEntity.equals() implementation can furthermore be replaced by a super.equals() call which would make this method much more clear.
If we won't change the VersionableEntity.equals() we should remove it, since it does exactly the same as the method it overrides.


Related issues

Related to Edit - feature request #7198: Implement equals() forCategoricalData Rejected 01/17/2018
Related to Edit - feature request #7199: Implement equals() for QuantitativeData Rejected 01/17/2018
Related to Edit - task #7201: [DISCUSS] Should we remove created comparison from CdmBase.equals? Closed 01/18/2018
Copied to Edit - task #7202: [DISCUSS] Can we remove equals override from some classes directly inheriting from CdmBase New 01/18/2018

Associated revisions

Revision 51a26cec (diff)
Added by Andreas Müller over 1 year ago

ref #7155, ref #7198, ref #7199 make Cdm.equals final

  • remove sematically equal method from VersionableEntity
  • add javadoc to explain why it should be final
  • open issue: do we really need to compare created in equals ?

Revision c9dfe0eb (diff)
Added by Andreas Müller over 1 year ago

ref #7155 move final to VersionableEntity as some non-versionable classes still do have there own equals implementation

Revision 579a4a81 (diff)
Added by Andreas Müller over 1 year ago

ref #7201 remove created from equals and remove equals override in TermBase

  • the new CdmBase.equals implementation should now be equivalent to old TermBase.equals

Revision 5069790f (diff)
Added by Andreas Müller over 1 year ago

ref #7201, ref #7155 remove equals override also from OrderedTermBase

Revision bef37262 (diff)
Added by Andreas Müller over 1 year ago

ref #7201, ref #7155 javadoc

Revision 04af63c0 (diff)
Added by Andreas Müller over 1 year ago

ref #7201, ref #7155 revert setting to final in CdmBase (was not on purpose)

History

#1 Updated by Andreas Kohlbecker almost 2 years ago

  • Status changed from New to In Progress
  • Assignee changed from Andreas Müller to Andreas Kohlbecker

I changed the method to


/**
* Is true if UUID created and updated timestamp are the same for the passed Object and this one.
* @see eu.etaxonomy.cdm.model.common.CdmBase#equals(java.lang.Object)
* See {@link http://www.hibernate.org/109.html hibernate109}, {@link http://www.geocities.com/technofundo/tech/java/equalhash.html geocities}
* or {@link http://www.ibm.com/developerworks/java/library/j-jtp05273.html ibm}
* for more information about equals and hashcode.
*/
@Override
public boolean equals(Object obj) {

    if(!super.equals(obj)){
        return false;
    }

    if(!VersionableEntity.class.isAssignableFrom(obj.getClass())){
        return false;
    }

    return CdmUtils.nullSafeEqual(((VersionableEntity)obj).getUpdated(), this.getUpdated());
}

all tests in cdmlib are running successful with this. So I will commit this change if no one objects.

#2 Updated by Andreas Müller almost 2 years ago

I do not agree with this solution. The up to now implementation is essential for almost everything we have done until now. If we soften it we do not know which algorithms do not work correct anymore even if no tests fail. Also it makes the correct understanding of equals more difficult.
Also I do not understand exactly why the updated timestamp should make a difference. For volatile objects this timestamp may not change if the object has been changed, so in the next step someone will say that we have to check each single value for equality.

Generally this is a highly sensitive issue that we shouldn't change without further discussion. It is also highly related to a correct transaction model (e.g. optimistic locking uses versioning (@Version, https://www.intertech.com/Blog/versioning-optimistic-locking-in-hibernate/ ) . A change in the equals method should take a correct implementation for a better transaction strategy into account (currently we use "last commit wins" which is definitely not the best). This can't be done in a minute and definitely needs further research.

The current contract is defined as "there is only one instance of the given database object per session and it is not possible to compare objects of 2 different sessions except for the question if they represent the same record in the database.

Certainly more arguing is needed but I commit this for now to stop the preliminary new implementation.

#3 Updated by Andreas Müller almost 2 years ago

Andreas Kohlbecker wrote:

I changed the method to


/**
* Is true if UUID created and updated timestamp are the same for the passed Object and this one.
* @see eu.etaxonomy.cdm.model.common.CdmBase#equals(java.lang.Object)
* See {@link http://www.hibernate.org/109.html hibernate109}, {@link http://www.geocities.com/technofundo/tech/java/equalhash.html geocities}
* or {@link http://www.ibm.com/developerworks/java/library/j-jtp05273.html ibm}
* for more information about equals and hashcode.
*/
@Override
public boolean equals(Object obj) {

    if(!super.equals(obj)){
        return false;
    }

    if(!VersionableEntity.class.isAssignableFrom(obj.getClass())){
        return false;
    }

    return CdmUtils.nullSafeEqual(((VersionableEntity)obj).getUpdated(), this.getUpdated());
}

all tests in cdmlib are running successful with this. So I will commit this change if no one objects.

By the way, if we implement it finally in this way we should definitely update the javadoc as the links are not related to any updated field or so but only a copy of CdmBase.equals().

#4 Updated by Andreas Kohlbecker almost 2 years ago

Hi Andreas,

I posted this provoking code as a suggestion and as an entry into the discussion. I never would have committed this crucial change without further research on the implications.

Your swift answer, for which I am grateful, contains all the concerns that I actually expected but also the point that drove me in starting this discussion:

The current contract is defined as "there is only one instance of the given database object per session and it is not possible to compare objects of 2 different sessions except for the question if they represent the same record in the database.

In my eyes, the assumption on which the contract is based exactly is the problem. Most client application code runs outside of the hibernate session context. That is, entities will stem from different session and thus are no longer guaranteed to be equal when VersionableEntity.equals() returns true.

On the other hand, using hibernate entities outside the session context, that is in the client application code, is an known antipattern and bad practice. Maybe we rather should guide the discussion in this directions and start thinking about using DTOs in general in clients.

Andreas

#5 Updated by Andreas Kohlbecker almost 2 years ago

  • Description updated (diff)

#6 Updated by Andreas Kohlbecker over 1 year ago

  • Status changed from In Progress to Feedback
  • Assignee changed from Andreas Kohlbecker to Andreas Müller
  • Severity changed from critical to normal

Hi Andreas, what shall we do with this issue? It is a topic for further discussion, therefore I think we should move it to another milestone.

#7 Updated by Andreas Müller over 1 year ago

  • Assignee changed from Andreas Müller to Andreas Kohlbecker

According to our telco we will not implement it as described in the title. As far as I remember we agreed that hibernate entities should better not be used directly in user interfaces. If used nevertheless, the UI should be implemented such that it accepts the current equals() contract.

However, we may discuss different handling of updated etc. e.g. by using the @Version annotation. But this is more for improving transaction strategies than for testing equalness, and therefore another ticket IMO.

#8 Updated by Andreas Kohlbecker over 1 year ago

  • Status changed from Feedback to Rejected
  • Target version deleted (Release 4.13)

Thank you for the additional points that I missed in my comment above.

So we can close this ticket as rejected, but for now i would rather abstain from creating two new tickets for the points that we might discuss later on.

#9 Updated by Andreas Müller over 1 year ago

#10 Updated by Andreas Müller over 1 year ago

#11 Updated by Andreas Müller over 1 year ago

There is another possibility to compare instances of a given CdmBase class by using IMatchStrategy (see https://dev.e-taxonomy.eu/redmine/issues/7199#note-1)

#12 Updated by Andreas Kohlbecker over 1 year ago

  • Status changed from Rejected to Feedback
  • Assignee changed from Andreas Kohlbecker to Andreas Müller

I am re-opening this issue because there is one point raised in here which is still open:

... VersionableEntity.equals() we should remove it, since it does exactly the same as the method it overrides.

Do you agree with removing this method override?

#13 Updated by Andreas Müller over 1 year ago

  • Related to task #7201: [DISCUSS] Should we remove created comparison from CdmBase.equals? added

#14 Updated by Andreas Müller over 1 year ago

Andreas Kohlbecker wrote:

I am re-opening this issue because there is one point raised in here which is still open:

... VersionableEntity.equals() we should remove it, since it does exactly the same as the method it overrides.

Do you agree with removing this method override?

Agreed and done: 51a26cec

#15 Updated by Andreas Müller over 1 year ago

  • Status changed from Feedback to Closed

I think we can close this ticket, or?

#16 Updated by Andreas Kohlbecker over 1 year ago

great!

#17 Updated by Andreas Müller over 1 year ago

AM:

in letzter Zeit wurde von verschiedentlich nachgefragt, ob die equals Methode von CdmBase Klassen überschrieben werden kann.

Diskussion z.B. in https://dev.e-taxonomy.eu/redmine/issues/7155 und related tickets.

Ich habe diese Methode jetzt auf final gesetzt, so dass überschreiben nicht mehr möglich ist, mit entsprechendem javadoc.
In VersionableEntity habe ich die semantisch quasi gleiche override Methode entfernt. (Ohh ich sehe gerade, dass es noch mehr overrides gibt, das kläre ich gleich noch, compile Fehler).

Für Vergleiche stehen die Matching Klassen zur Verfügung oder eigene Implementierungen.

Eine Frage habe ich aber noch: In der derzeitigen Implementierung wird aus irgendeinem Grund noch das created Attribut verglichen.
Ich habe leider überhaupt keine Erinnerung mehr, warum wir das mal eingefügt haben. Falls da jemand eine Idee hat, würde mich das sehr interessieren.
Ansonsten sollten wir mal schauen, ob wir das experimentell mal rausnehmen und ob trotzdem alles noch funktioniert. Möglichst direkt nach einem Release.
Diskussion bitte in https://dev.e-taxonomy.eu/redmine/issues/7201 .

#18 Updated by Andreas Müller over 1 year ago

  • Copied to task #7202: [DISCUSS] Can we remove equals override from some classes directly inheriting from CdmBase added

#19 Updated by Andreas Müller over 1 year ago

I moved the discussion on making CdmBase.equals final to #7202 as some classes directly inheriting from CdmBase still override equals().

Also available in: Atom PDF

Add picture from clipboard (Maximum size: 40 MB)