Project

General

Profile

Actions

bug #10101

open

Correct mapping and handling of sorted tree structures

Added by Andreas Müller almost 2 years ago. Updated almost 2 years ago.

Status:
In Progress
Priority:
Priority14
Category:
cdmlib
Target version:
Start date:
Due date:
% Done:

20%

Estimated time:
Severity:
normal
Found in Version:

Description

We currently have 3 sorted tree structures in the CDM: Classifications, TermTrees and PolytomousKeys.

The correct mapping and handling of sorted tree structures is difficult. Children are modelled as Lists which usually is best handled with an @OrderColumn annotation (#3722) and a bidirectional mapping with orphanRemoval. If orphanRemoval is not used orphaned (deleted) children need to be deleted manually, otherwise they stay in the database and, in the worst case, have the same parent_id and sortindex if parent_id is not correctly removed by bidirectionality imlementation. This leads to kind of duplicates and, even worse, to gaps in the sortindex when the tree structure is persisted via merge. Gaps in the sortindexes (e.g. 0 followed by 3) lead to null values when the list is reloaded (#5536, #8127).

However, though orphanRemoval handles this best for Lists it is not good for (recursive) trees because moving a subtree to another parent then requires to clone the full subtree as not only the root but also all its children will be automatically deleted by hibernate via orphanRemoval. For large trees this is unwanted behavior and also for smaller trees cloning is not a great solution.

So a better solution is to manually remove the nodes if they are not used anymore. We tried to do this automatically for merge via the PostMergeEntityListener but still without success. See comments in #note-7.

Therefore a new merge method was created to merge together with the entities that should be deleted. This seems to work.

Note: To avoid the sortindex still has a gap for some reason we have to touch the children list somehow before commit. This is currently done in the PostMergeEntityListener.

Related tests are: PolytomousKeyNodeServiceTest.testMergeXXX() (maybe will move to PolytomousKeyServiceTest partly)


Related issues

Related to EDIT - task #3722: Using @OrderColumn does not work in treesClosedAndreas Müller05/23/202205/27/2022

Actions
Related to EDIT - task #8127: [Reminder] to check if Hibernate Null Value bug still existsDiscussedAndreas Müller05/23/202205/27/2022

Actions
Related to EDIT - bug #5211: Error when creating new taxon /problem with sortindex in taxon treeIn ProgressAndreas Müller

Actions
Actions #1

Updated by Andreas Müller almost 2 years ago

  • Related to task #3722: Using @OrderColumn does not work in trees added
Actions #3

Updated by Andreas Müller almost 2 years ago

  • Related to task #8127: [Reminder] to check if Hibernate Null Value bug still exists added
Actions #4

Updated by Andreas Müller almost 2 years ago

  • Related to bug #5211: Error when creating new taxon /problem with sortindex in taxon tree added
Actions #6

Updated by Andreas Müller almost 2 years ago

  • Description updated (diff)
Actions #7

Updated by Andreas Müller almost 2 years ago

Code comment from PostMergeEntityListener.java:

// #10101 the following code tried to handle orphanRemoval for key nodes that were
// really removed from the graph. Generally the removal worked but it was not possible at this
// place to guarantee that the node which was removed from the parent was not used elsewhere
// (has a new parent). For this one needs to retrieve the new state of the node (or of its new parent).
// But this is not difficult or impossible at this point as the node is not part of the graph to
// to be merged or if it is because its new parent is part of the graph also, it is not guaranteed
// that it has already treated so far.
// Maybe it can better be handled by another type of listener therefore I keep this code here.
// See #10101 for further information on this issue and how it was solved.
// The implementation was partly copied from https://stackoverflow.com/questions/812364/how-to-determine-collection-changes-in-a-hibernate-postupdateeventlistener

https://dev.e-taxonomy.eu/gitweb/cdmlib.git/blob/e1284d9aace334813f3bee700c1aa1127033f1ef:/cdmlib-persistence/src/main/java/eu/etaxonomy/cdm/persistence/hibernate/PostMergeEntityListener.java

Actions #8

Updated by Andreas Müller almost 2 years ago

  • Description updated (diff)
Actions #9

Updated by Andreas Müller almost 2 years ago

  • Status changed from New to In Progress
  • Priority changed from New to Priority14
  • Target version changed from Release 5.32 to Release 5.33
  • % Done changed from 0 to 20

I added tests for mergeDetached and saveDetached on nodes and trees and with removal and move of nodes to PolytomousKeyNodeServiceTest, TermNodeServiceTest and TaxonNodeServiceTest.

  • for PolytomousKeys all tests run well and also the TaxEditor does not seem to have problems anymore.
  • for TermNodes the mergeDetached on node with removal does not work for some reason, though the configuration is very similar to the one for PolytomousKeys, however, in TaxEditor it seems to work, maybe because we use DTOs there and finde graine operations (either delete or create)
  • for TaxonNodes the tests still need to be adapted as TaxonNodes, on purpuse, do not cascade. However, TaxEditor seems to work with (probably same reason as for TermNodes)

All failing tests set to @Ignore, so there are open issues. Especially the TermNodes issue needs to be further investigated.

Also I keep #8127 open as the current implementation works only with the workaround in PostMergeEntityListener and does not work at all yet for TermNodes and is not tested for TaxonNodes

Actions #10

Updated by Andreas Müller almost 2 years ago

I removed the HHH_9751_Util class as it is currently not used anymore, please recreate from cb923587ce746 and taxeditor commit 09abc6569 if it is used in future again.

Actions #11

Updated by Andreas Müller almost 2 years ago

Comment from onMerge in PostMergeEntityListener:

Added in context of working on #10101, it was unclear to me
why this method was not implemented before. Debugging the merge process
revealed that this method was primarily called on the first entity
to merge (as copied already is not necessary then).
So setting the ID and adding to the newEntitiesMap seems
not to be implemented for entities handle only here.
Needs intensive testing if there is not maybe a reason why the method
was not implemented.
Actions #12

Updated by Andreas Müller almost 2 years ago

Finding duplicates:

SELECT pkn.key_id, pkn.parent_id, pkn.sortindex, COUNT(*) AS n
FROM PolytomousKeyNode pkn
GROUP BY pkn.key_id, pkn.parent_id, pkn.sortindex
HAVING n > 1;

SELECT tn.classification_id, tn.parent_id, tn.sortindex, COUNT(*) AS n
FROM TaxonNode tn
GROUP BY tn.classification_id, tn.parent_id, tn.sortindex
HAVING n > 1;

SELECT tr.graph_id, tr.parent_id, tr.sortindex, COUNT(*) AS n
FROM TermRelation tr
GROUP BY tr.graph_id, tr.parent_id, tr.sortindex
HAVING n > 1;

A helpful SQL for checking the correctnes of the sortindex (here for PKNode):

SELECT pkn.id, pkn.created, pkn.updated, nodenumber, key_id, parent_id, sortindex 
FROM PolytomousKeyNode pkn
 WHERE key_id = 24
ORDER BY key_id, parent_id, id, sortindex;

SELECT pkn.id, pkn.created, pkn.updated, nodenumber, KEY_id, parent_id, sortindex, pkn.revtype
FROM PolytomousKeyNode_AUD pkn
 WHERE key_id = 24 OR id = 394
ORDER BY key_id, parent_id, id, sortindex
Actions #13

Updated by Andreas Müller almost 2 years ago

  • Tags set to 5.32-Remaining
  • Target version changed from Release 5.33 to Release 5.51
Actions

Also available in: Atom PDF