OAK-12213 Tree store: child node list maybe be incorrect if an intermediate node is missing#2902
OAK-12213 Tree store: child node list maybe be incorrect if an intermediate node is missing#2902thomasmueller wants to merge 4 commits into
Conversation
…ediate node is missing
…ediate node is missing
|
| throw new IllegalArgumentException(key); | ||
| } | ||
| current = key.substring(index + 1); | ||
| if (!key.startsWith(path + "\t")) { |
There was a problem hiding this comment.
If I understand correctly this is a stop condition because we've reached the sibling following the current node ? So treeStore.getSession().iterator(path) returns all the paths starting a path depth first ? So the method should really be named getDescendantNodeNamesIterator(String path), or do I miss understand something ?
There was a problem hiding this comment.
If I understand correctly this is a stop condition because we've reached the sibling following the current node ?
Yes. The bug is that a node was returned that is not a child node.
So treeStore.getSession().iterator(path) returns all the paths starting a path depth first ?
It doesn't use depth-first; it returns the entries in key order. I have now updated the documentation.
So the method should really be named getDescendantNodeNamesIterator(String path)
The methods is supposed to return only the direct children. But it returned non-children in some specific cases. This PR is supposed to fix that, so it only returns direct children.
| Iterator<Entry<String, String>> it = treeStore.getSession().iterator(path); | ||
| return new Iterator<String>() { |
There was a problem hiding this comment.
To follow the sonar point, this could indeed moved to an explicit TreeStorePathIterator class. that you could just instanciate here.
There was a problem hiding this comment.
Sure, this could be done, but I don't think it would make the code easier to understand. I think whether or not a separate class should be used here is an issue that is unrelated to his PR.
Cognitive Complexity
For this case, I don't agree with Sonar that this method is too complex and should be split into multiple methods.
…ediate node is missing
| assertEquals("/test", e.getPath()); | ||
| e = it.next(); | ||
| assertEquals("/test-node", e.getPath()); | ||
| Iterator<String> it2 = e.getNodeState().getChildNodeNames().iterator(); |
There was a problem hiding this comment.
With these 2 iterators this test is hard to follow. Would there be any way to make it clearer to the reader ? I am missing the intent of the test, to be able to validate that the test is correctly checking that intent (which I am sure it is, knowing the author ;) )
…ediate node is missing
3 participants
No description provided.