Project

General

Profile

Actions

bug #7155

closed

VersionableEntity.equals() should take updated timestamp into account

Added by Andreas Kohlbecker about 5 years ago. Updated about 5 years ago.

Status:
Closed
Priority:
Highest
Category:
cdmlib
Target version:
-
Start date:
Due date:
% Done:

0%

Estimated time:
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() forCategoricalDataRejectedPatrick Plitzner

Actions
Related to EDIT - feature request #7199: Implement equals() for QuantitativeDataRejectedPatrick Plitzner

Actions
Related to EDIT - task #7201: [DISCUSS] Should we remove created comparison from CdmBase.equals?ClosedAndreas Müller

Actions
Copied to EDIT - task #7202: [DISCUSS] Can we remove equals override from some classes directly inheriting from CdmBase NewAndreas Kohlbecker

Actions
Actions #1

Updated by Andreas Kohlbecker about 5 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.

Actions #2

Updated by Andreas Müller about 5 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.

Actions #3

Updated by Andreas Müller about 5 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().

Actions #4

Updated by Andreas Kohlbecker about 5 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

Actions #5

Updated by Andreas Kohlbecker about 5 years ago

  • Description updated (diff)
Actions #6

Updated by Andreas Kohlbecker about 5 years 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.

Actions #7

Updated by Andreas Müller about 5 years 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.

Actions #8

Updated by Andreas Kohlbecker about 5 years 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.

Actions #9

Updated by Andreas Müller about 5 years ago

Actions #10

Updated by Andreas Müller about 5 years ago

Actions #11

Updated by Andreas Müller about 5 years 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)

Actions #12

Updated by Andreas Kohlbecker about 5 years 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?

Actions #13

Updated by Andreas Müller about 5 years ago

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

Updated by Andreas Müller about 5 years 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

Actions #15

Updated by Andreas Müller about 5 years ago

  • Status changed from Feedback to Closed

I think we can close this ticket, or?

Actions #16

Updated by Andreas Kohlbecker about 5 years ago

great!

Actions #17

Updated by Andreas Müller about 5 years 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 .

Actions #18

Updated by Andreas Müller about 5 years ago

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

Updated by Andreas Müller about 5 years ago

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

Actions

Also available in: Atom PDF