https://dev.e-taxonomy.eu/redmine/https://dev.e-taxonomy.eu/redmine/redmine/favicon.ico?14691914852018-01-08T10:24:12ZEDIT Project ManagementEDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=338222018-01-08T10:24:12ZAndreas Kohlbecker
<ul><li><strong>Status</strong> changed from <i>New</i> to <i>In Progress</i></li><li><strong>Assignee</strong> changed from <i>Andreas Müller</i> to <i>Andreas Kohlbecker</i></li></ul><p>I changed the method to </p>
<pre><code class="java syntaxhl">
<span class="cm">/**
* 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.
*/</span>
<span class="nd">@Override</span>
<span class="kd">public</span> <span class="kt">boolean</span> <span class="nf">equals</span><span class="o">(</span><span class="nc">Object</span> <span class="n">obj</span><span class="o">)</span> <span class="o">{</span>
<span class="k">if</span><span class="o">(!</span><span class="kd">super</span><span class="o">.</span><span class="na">equals</span><span class="o">(</span><span class="n">obj</span><span class="o">)){</span>
<span class="k">return</span> <span class="kc">false</span><span class="o">;</span>
<span class="o">}</span>
<span class="k">if</span><span class="o">(!</span><span class="nc">VersionableEntity</span><span class="o">.</span><span class="na">class</span><span class="o">.</span><span class="na">isAssignableFrom</span><span class="o">(</span><span class="n">obj</span><span class="o">.</span><span class="na">getClass</span><span class="o">())){</span>
<span class="k">return</span> <span class="kc">false</span><span class="o">;</span>
<span class="o">}</span>
<span class="k">return</span> <span class="nc">CdmUtils</span><span class="o">.</span><span class="na">nullSafeEqual</span><span class="o">(((</span><span class="nc">VersionableEntity</span><span class="o">)</span><span class="n">obj</span><span class="o">).</span><span class="na">getUpdated</span><span class="o">(),</span> <span class="k">this</span><span class="o">.</span><span class="na">getUpdated</span><span class="o">());</span>
<span class="o">}</span>
</code></pre>
<p>all tests in cdmlib are running successful with this. So I will commit this change if no one objects.</p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=338252018-01-08T12:50:05ZAndreas Müller
<ul></ul><p>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.<br>
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.</p>
<p>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, <a href="https://www.intertech.com/Blog/versioning-optimistic-locking-in-hibernate/">https://www.intertech.com/Blog/versioning-optimistic-locking-in-hibernate/</a> ) . 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.</p>
<p>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.</p>
<p>Certainly more arguing is needed but I commit this for now to stop the preliminary new implementation. </p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=338262018-01-08T13:03:14ZAndreas Müller
<ul></ul><p>Andreas Kohlbecker wrote:</p>
<blockquote>
<p>I changed the method to </p>
<pre><code class="java syntaxhl">
<span class="cm">/**
* 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.
*/</span>
<span class="nd">@Override</span>
<span class="kd">public</span> <span class="kt">boolean</span> <span class="nf">equals</span><span class="o">(</span><span class="nc">Object</span> <span class="n">obj</span><span class="o">)</span> <span class="o">{</span>
<span class="k">if</span><span class="o">(!</span><span class="kd">super</span><span class="o">.</span><span class="na">equals</span><span class="o">(</span><span class="n">obj</span><span class="o">)){</span>
<span class="k">return</span> <span class="kc">false</span><span class="o">;</span>
<span class="o">}</span>
<span class="k">if</span><span class="o">(!</span><span class="nc">VersionableEntity</span><span class="o">.</span><span class="na">class</span><span class="o">.</span><span class="na">isAssignableFrom</span><span class="o">(</span><span class="n">obj</span><span class="o">.</span><span class="na">getClass</span><span class="o">())){</span>
<span class="k">return</span> <span class="kc">false</span><span class="o">;</span>
<span class="o">}</span>
<span class="k">return</span> <span class="nc">CdmUtils</span><span class="o">.</span><span class="na">nullSafeEqual</span><span class="o">(((</span><span class="nc">VersionableEntity</span><span class="o">)</span><span class="n">obj</span><span class="o">).</span><span class="na">getUpdated</span><span class="o">(),</span> <span class="k">this</span><span class="o">.</span><span class="na">getUpdated</span><span class="o">());</span>
<span class="o">}</span>
</code></pre>
<p>all tests in cdmlib are running successful with this. So I will commit this change if no one objects.</p>
</blockquote>
<p>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(). </p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=338272018-01-08T13:40:52ZAndreas Kohlbecker
<ul></ul><p>Hi Andreas,</p>
<p>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.</p>
<p>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: </p>
<blockquote>
<p>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.</p>
</blockquote>
<p>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 <code>VersionableEntity.equals()</code> returns true.</p>
<p>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.</p>
<p>Andreas</p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=338282018-01-08T14:03:42ZAndreas Kohlbecker
<ul><li><strong>Description</strong> updated (<a title="View differences" href="/redmine/journals/33828/diff?detail_id=43295">diff</a>)</li></ul> EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=339842018-01-16T15:00:24ZAndreas Kohlbecker
<ul><li><strong>Status</strong> changed from <i>In Progress</i> to <i>Feedback</i></li><li><strong>Assignee</strong> changed from <i>Andreas Kohlbecker</i> to <i>Andreas Müller</i></li><li><strong>Severity</strong> changed from <i>critical</i> to <i>normal</i></li></ul><p>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. </p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=339882018-01-16T15:14:22ZAndreas Müller
<ul><li><strong>Assignee</strong> changed from <i>Andreas Müller</i> to <i>Andreas Kohlbecker</i></li></ul><p>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.</p>
<p>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.</p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=340062018-01-17T07:08:24ZAndreas Kohlbecker
<ul><li><strong>Status</strong> changed from <i>Feedback</i> to <i>Rejected</i></li><li><strong>Target version</strong> deleted (<del><i>Release 4.13</i></del>)</li></ul><p>Thank you for the additional points that I missed in my comment above.</p>
<p>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.</p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=340142018-01-17T21:58:03ZAndreas Müller
<ul><li><strong>Related to</strong> <i><a class="issue tracker-5 status-6 priority-11 priority-default closed" href="/redmine/issues/7198">feature request #7198</a>: Implement equals() forCategoricalData</i> added</li></ul> EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=340182018-01-17T21:58:31ZAndreas Müller
<ul><li><strong>Related to</strong> <i><a class="issue tracker-5 status-6 priority-11 priority-default closed" href="/redmine/issues/7199">feature request #7199</a>: Implement equals() for QuantitativeData</i> added</li></ul> EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=340202018-01-17T22:00:06ZAndreas Müller
<ul></ul><p>There is another possibility to compare instances of a given CdmBase class by using IMatchStrategy (see <a href="https://dev.e-taxonomy.eu/redmine/issues/7199#note-1">https://dev.e-taxonomy.eu/redmine/issues/7199#note-1</a>)</p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=340212018-01-18T07:05:35ZAndreas Kohlbecker
<ul><li><strong>Status</strong> changed from <i>Rejected</i> to <i>Feedback</i></li><li><strong>Assignee</strong> changed from <i>Andreas Kohlbecker</i> to <i>Andreas Müller</i></li></ul><p>I am re-opening this issue because there is one point raised in here which is still open:</p>
<blockquote>
<p>... VersionableEntity.equals() we should remove it, since it does exactly the same as the method it overrides.</p>
</blockquote>
<p>Do you agree with removing this method override?</p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=340232018-01-18T09:37:23ZAndreas Müller
<ul><li><strong>Related to</strong> <i><a class="issue tracker-6 status-5 priority-11 priority-default closed" href="/redmine/issues/7201">task #7201</a>: [DISCUSS] Should we remove created comparison from CdmBase.equals?</i> added</li></ul> EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=340282018-01-18T09:39:03ZAndreas Müller
<ul></ul><p>Andreas Kohlbecker wrote:</p>
<blockquote>
<p>I am re-opening this issue because there is one point raised in here which is still open:</p>
<blockquote>
<p>... VersionableEntity.equals() we should remove it, since it does exactly the same as the method it overrides.</p>
</blockquote>
<p>Do you agree with removing this method override?</p>
</blockquote>
<p>Agreed and done: 51a26cec </p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=340292018-01-18T09:39:44ZAndreas Müller
<ul><li><strong>Status</strong> changed from <i>Feedback</i> to <i>Closed</i></li></ul><p>I think we can close this ticket, or?</p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=340302018-01-18T09:43:53ZAndreas Kohlbecker
<ul></ul><p>great!</p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=340312018-01-18T09:52:59ZAndreas Müller
<ul></ul><p>AM:</p>
<p>in letzter Zeit wurde von verschiedentlich nachgefragt, ob die equals Methode von CdmBase Klassen überschrieben werden kann.</p>
<p>Diskussion z.B. in <a href="https://dev.e-taxonomy.eu/redmine/issues/7155">https://dev.e-taxonomy.eu/redmine/issues/7155</a> und related tickets.</p>
<p>Ich habe diese Methode jetzt auf final gesetzt, so dass überschreiben nicht mehr möglich ist, mit entsprechendem javadoc.<br>
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).</p>
<p>Für Vergleiche stehen die Matching Klassen zur Verfügung oder eigene Implementierungen.</p>
<p>Eine Frage habe ich aber noch: In der derzeitigen Implementierung wird aus irgendeinem Grund noch das created Attribut verglichen. <br>
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.<br>
Ansonsten sollten wir mal schauen, ob wir das experimentell mal rausnehmen und ob trotzdem alles noch funktioniert. Möglichst direkt nach einem Release.<br>
Diskussion bitte in <a href="https://dev.e-taxonomy.eu/redmine/issues/7201">https://dev.e-taxonomy.eu/redmine/issues/7201</a> .</p>
EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=340402018-01-18T11:07:40ZAndreas Müller
<ul><li><strong>Copied to</strong> <i><a class="issue tracker-6 status-1 priority-11 priority-default" href="/redmine/issues/7202">task #7202</a>: [DISCUSS] Can we remove equals override from some classes directly inheriting from CdmBase </i> added</li></ul> EDIT - bug #7155: VersionableEntity.equals() should take updated timestamp into accounthttps://dev.e-taxonomy.eu/redmine/issues/7155?journal_id=340432018-01-18T11:10:24ZAndreas Müller
<ul></ul><p>I moved the discussion on making CdmBase.equals final to <a class="issue tracker-6 status-1 priority-11 priority-default" title="task: [DISCUSS] Can we remove equals override from some classes directly inheriting from CdmBase (New)" href="https://dev.e-taxonomy.eu/redmine/issues/7202">#7202</a> as some classes directly inheriting from CdmBase still override equals().</p>