search

eXsio / nestedj

NestedJ - a NestedSet implementation for Java
MIT License
95 stars 24 forks source link

Cannot insertAsLastRoot if node already is a last root #5

Closed Smartel1 closed 4 years ago

Smartel1 commented 4 years ago

QueryBasedNestedNodeMover, line 54 (v4.1.0) checks whether node can be a parent of itself. Looks like a bug

eXsio commented 4 years ago

If node is already a last root then a good thing would be not to do anything at all. I need to think this through as setting a node as a sibling to itself does not seem reasonable.

I need to create a test to see how the logic will behave and what will be the left and right values.

Smartel1 commented 4 years ago

I call insertAsLastRoot() not only when creating node, but also when updating it. Just the same approach as we call repository.save()

eXsio commented 4 years ago

I understand, but wouldn't it be more efficient to just catch the InvalidNodesHierarchyException and do nothing (maybe just log something)? Even if everything works correctly (and I'm not sure it will) the result will be an unchanged tree structure.

Smartel1 commented 4 years ago

In this case I would write more code on client side

eXsio commented 4 years ago

I think the try/catch could be added to the insertAsLastRoot and insertAsFirstRoot method as they shoul not throw that exception

Smartel1 commented 4 years ago

Sounds good. Can you please explain one thing. move( nodeInfo, parentInfo, mode) - parentInfo is a parent node when mode == FIRST_CHILD or LAST_CHILD, and is a sibling when mode == PREV_SIBLING or NEXT_SIBLING?

eXsio commented 4 years ago

Yes, but the logic assumes that the node is not moved to itself. The MODE is used to determine the logic to use when adjusting the left, right and level values. I created it as all the four variants have the same sequence of performing the tree modification, just with slight difference when it comes to calculation of left, right and level.

Smartel1 commented 4 years ago

Its a little bit confusing as 'parent' is narrow notion. You should probably rename this argument to something like 'relativeNodeInfo' or make describing doc block

Smartel1 commented 4 years ago

I've added some more commits to PR

eXsio commented 4 years ago

Found a better way of fixing the issue. Fixed in 4.1.1.

Smartel1 commented 4 years ago

Thank you for this fix! But I've found one little fault. In case of node is already last (first) node: entityManager.persist() will never be called. Otherwise it will. Its not a big problem because I can call it manually..

eXsio commented 4 years ago

Well that will make a difference only when you are passing a detached Entity that is at the same time already a last/first node.

That is a pretty edgy case, not to mention that calling persist() on a detached node can cause Hibernate to throw exception if there are any uncascaded relationships passed alongside the root Entity :)

Smartel1 commented 4 years ago

Ok, lets close this iss. Thanks!