Jackrabbit Oak
  1. Jackrabbit Oak
  2. OAK-343

Session.getNodeByUUID requires save call

    Details

    • Type: Bug Bug
    • Status: Closed
    • Priority: Major Major
    • Resolution: Fixed
    • Affects Version/s: None
    • Fix Version/s: 0.7
    • Component/s: core, jcr
    • Labels:
      None

      Description

      while adding mix:referenceable to a new node immediately assigns a
      uuid and makes the node referenceable, session.getNodeByUUID only works
      after saving the changes.

      the following test illustrates this behavior:

      @Test
      public void getNodeByUUID() throws RepositoryException

      { Node node = getNode("/foo").addNode("boo"); node.addMixin(JcrConstants.MIX_REFERENCEABLE); assertTrue(node.isNodeType(JcrConstants.MIX_REFERENCEABLE)); String uuid = node.getUUID(); assertNotNull(uuid); assertEquals(uuid, node.getIdentifier()); Node nAgain = node.getSession().getNodeByUUID(uuid); assertTrue(nAgain.isSame(node)); assertTrue(nAgain.isSame(node.getSession().getNodeByIdentifier(uuid))); }
      1. oak-343-QueryIndex.patch
        11 kB
        Manfred Baedke
      2. OAK-343-v2.patch
        17 kB
        Alex Parvulescu

        Activity

        Hide
        angela added a comment -

        the same applies to IdentifierManager#getTree and main issue behind is that uuid/identifier resolutions uses
        the query which relies on an index that is only populated after changes have been saved.

        Show
        angela added a comment - the same applies to IdentifierManager#getTree and main issue behind is that uuid/identifier resolutions uses the query which relies on an index that is only populated after changes have been saved.
        Hide
        Jukka Zitting added a comment -

        Do we need the UUIDs to be present and usable before a save()? The relevant part of the JCR 2.0 spec says:

        The identifier must be assigned at the latest when the node is first persisted, though it may be assigned earlier, when the node is first created in transient storage in the session.

        Show
        Jukka Zitting added a comment - Do we need the UUIDs to be present and usable before a save()? The relevant part of the JCR 2.0 spec says: The identifier must be assigned at the latest when the node is first persisted, though it may be assigned earlier, when the node is first created in transient storage in the session.
        Hide
        angela added a comment -

        well, since the uuid is created immediately and the node is referenceable as well,
        i would expect that i could also get it by uuid.

        second we have an issue with a backwards compatible user mgt implementation:
        a NEW user used to be accessible by for the editing session even before commiting
        it. second, since the tree object isn't stable it needs to be re-accessed during
        the life-time of a user object. searching isn't possible but i would expect that
        lookup be id was possible.

        Show
        angela added a comment - well, since the uuid is created immediately and the node is referenceable as well, i would expect that i could also get it by uuid. second we have an issue with a backwards compatible user mgt implementation: a NEW user used to be accessible by for the editing session even before commiting it. second, since the tree object isn't stable it needs to be re-accessed during the life-time of a user object. searching isn't possible but i would expect that lookup be id was possible.
        Hide
        angela added a comment -

        regarding the user mgt: i realized that when looking at failing test
        that were originally written for jackrabbit and which i would expect
        to pass (as long as the tests are not obviously buggy)

        Show
        angela added a comment - regarding the user mgt: i realized that when looking at failing test that were originally written for jackrabbit and which i would expect to pass (as long as the tests are not obviously buggy)
        Hide
        Jukka Zitting added a comment -

        Marcel brought up the good idea that there's no need for us to wait for the save() or the commit hook until the jcr:uuid index used by the getNodeByUUID() method gets updated. Instead we could update that index already in the transient space as soon as the UUID is assigned to a new, transient node. The index lookup then just needs to be done against the transient state of the tree instead of the persisted one.

        Show
        Jukka Zitting added a comment - Marcel brought up the good idea that there's no need for us to wait for the save() or the commit hook until the jcr:uuid index used by the getNodeByUUID() method gets updated. Instead we could update that index already in the transient space as soon as the UUID is assigned to a new, transient node. The index lookup then just needs to be done against the transient state of the tree instead of the persisted one.
        Hide
        Julian Reschke added a comment -

        Spec lawyering question: does the JCR spec allow an implementation to assign the identifier early but then fail getNodeByIdentifier()? It it's silent on that, we may want to clarify that in JSR 333.

        Show
        Julian Reschke added a comment - Spec lawyering question: does the JCR spec allow an implementation to assign the identifier early but then fail getNodeByIdentifier()? It it's silent on that, we may want to clarify that in JSR 333.
        Hide
        Manfred Baedke added a comment -

        As suggested by Thomas and Angela, this can be solved by keeping track of the UUIDs of newly created nodes using a mapping in RootImpl. If noone objects, I'm going to implement this.

        Show
        Manfred Baedke added a comment - As suggested by Thomas and Angela, this can be solved by keeping track of the UUIDs of newly created nodes using a mapping in RootImpl. If noone objects, I'm going to implement this.
        Hide
        Manfred Baedke added a comment -

        I looked into it and found that it should be possible to wrap the RootImpl.indexProvider in order to add a QueryIndex that contains our transient map and takes care of the UUID lookup.
        Straightforward approaches using a Map that is aggregated by RootImpl would require a change to the Root interface, and I suppose that we do not want that.

        Show
        Manfred Baedke added a comment - I looked into it and found that it should be possible to wrap the RootImpl.indexProvider in order to add a QueryIndex that contains our transient map and takes care of the UUID lookup. Straightforward approaches using a Map that is aggregated by RootImpl would require a change to the Root interface, and I suppose that we do not want that.
        Hide
        Michael Dürig added a comment -

        wrap the RootImpl.indexProvider [...]

        I'd prefer this approach.

        Show
        Michael Dürig added a comment - wrap the RootImpl.indexProvider [...] I'd prefer this approach.
        Hide
        Manfred Baedke added a comment - - edited

        Following Jukkas hint to use NodeStateDiffs instead of a Map to lookup the transient nodes, I created a solution proposal using an additional QueryIndex (see attached oak-343-patch).

        Show
        Manfred Baedke added a comment - - edited Following Jukkas hint to use NodeStateDiffs instead of a Map to lookup the transient nodes, I created a solution proposal using an additional QueryIndex (see attached oak-343-patch).
        Hide
        Manfred Baedke added a comment -

        As expected, immediately after uploading the patch I realized that it's not going to work properly, because descendants of transient nodes will not be found.
        I will uploaded a better version soon.

        Show
        Manfred Baedke added a comment - As expected, immediately after uploading the patch I realized that it's not going to work properly, because descendants of transient nodes will not be found. I will uploaded a better version soon.
        Hide
        Thomas Mueller added a comment -

        This is a very interesting approach. It should work and is quite elegant. The query is actually executed in the getCost method, that's a bit outside of what I thought getCost should do, but I'm sure we can solve that (either change documentation or change the behavior a bit).

        Maybe we could use a similar mechanism for other problems as well, for example to allow other queries to see the latest (transient) state of the repository, even if the index implementation itself doesn't support that directly. This is a feature quite commonly requested, but is relatively hard to solve in another way. But there is a cost to it (even if it's only in-memory operation), so I guess it shouldn't be done by default. Maybe with an extension in the query syntax? ("select ... including transient" or so) Instead of 'only' using this special transient index, I wonder if the normal index and the transient index should be combined, so that the query doesn't need to be executed in getCost. A similar mechanism to overlay / combine indexes is already used in the NodeType index, and one is planned to speed up queries of type "a=1 and b=2". I will propose a solution for that, but I guess it's not urgent.

        Show
        Thomas Mueller added a comment - This is a very interesting approach. It should work and is quite elegant. The query is actually executed in the getCost method, that's a bit outside of what I thought getCost should do, but I'm sure we can solve that (either change documentation or change the behavior a bit). Maybe we could use a similar mechanism for other problems as well, for example to allow other queries to see the latest (transient) state of the repository, even if the index implementation itself doesn't support that directly. This is a feature quite commonly requested, but is relatively hard to solve in another way. But there is a cost to it (even if it's only in-memory operation), so I guess it shouldn't be done by default. Maybe with an extension in the query syntax? ("select ... including transient" or so) Instead of 'only' using this special transient index, I wonder if the normal index and the transient index should be combined, so that the query doesn't need to be executed in getCost. A similar mechanism to overlay / combine indexes is already used in the NodeType index, and one is planned to speed up queries of type "a=1 and b=2". I will propose a solution for that, but I guess it's not urgent.
        Hide
        Michael Dürig added a comment -

        Thanks for the patch. A few comments

        • Please use Guava instead of commons collections. See Oak-200.
        • Don't use start imports, import each class/member explicitly.
        • I'd rename InternalIndexProvider to UUIDIndexProvider and move the implementation to the end of the file.
        • UUIDQueryIndex.query() relies on the side effects of a prior call to UUIDQueryIndex.getCost(). Can we really rely of getCost() having been called at the point when query() is called? If so QueryIndexProvider should clearly state this in its contract. Otherwise it might be better to use a memoisation pattern for UUIDQueryIndex.path and explicitly calculate it if it wasn't calculated before.
        Show
        Michael Dürig added a comment - Thanks for the patch. A few comments Please use Guava instead of commons collections. See Oak-200. Don't use start imports, import each class/member explicitly. I'd rename InternalIndexProvider to UUIDIndexProvider and move the implementation to the end of the file. UUIDQueryIndex.query() relies on the side effects of a prior call to UUIDQueryIndex.getCost() . Can we really rely of getCost() having been called at the point when query() is called? If so QueryIndexProvider should clearly state this in its contract. Otherwise it might be better to use a memoisation pattern for UUIDQueryIndex.path and explicitly calculate it if it wasn't calculated before.
        Hide
        Manfred Baedke added a comment -

        Added recursion, works better now.
        @Michael: Thank you, I will make the according changes tomorrow and ask Thomas about the reliability of the getCost()-call.

        Show
        Manfred Baedke added a comment - Added recursion, works better now. @Michael: Thank you, I will make the according changes tomorrow and ask Thomas about the reliability of the getCost()-call.
        Hide
        Thomas Mueller added a comment - - edited

        For me it's not actually about that getCost() gets called first, but in general that getCost() is supposed to just calculate the cost, without side effects. Even if this side effect is within the specification, it's just unusual and unexpected. I would rather have a solution where getCost() only calculates the cost and the lookup is done within query(). I think we can find a solution. It's also problematic that getCost() returns 0 so that the cost is smaller than the cost of the regular jcr:uuid index. It does solve the problem, true, but it's not what I had in mind originally.

        About naming: I would probably call it "TransientUUIDIndex" because it operates on the transient space. Later on, if we want to make it more generic, we could call it "TransientIndex".

        What about making it explicit in the query that the result should include transient changes? So the query would be changed to "select ... from ... where [jcr:uuid] = $id including transient". Then the QueryIndex interface would be extended with a new method "boolean includesTransient". That way, if the query is "including transient", the query engine would only use indexes that do support transient changes.

        I'm OK to all those changes myself later on, as I don't think they are urgent.

        I wonder if both the NodeState and Root given to the query actually include the node?

        Show
        Thomas Mueller added a comment - - edited For me it's not actually about that getCost() gets called first, but in general that getCost() is supposed to just calculate the cost, without side effects. Even if this side effect is within the specification, it's just unusual and unexpected. I would rather have a solution where getCost() only calculates the cost and the lookup is done within query(). I think we can find a solution. It's also problematic that getCost() returns 0 so that the cost is smaller than the cost of the regular jcr:uuid index. It does solve the problem, true, but it's not what I had in mind originally. About naming: I would probably call it "TransientUUIDIndex" because it operates on the transient space. Later on, if we want to make it more generic, we could call it "TransientIndex". What about making it explicit in the query that the result should include transient changes? So the query would be changed to "select ... from ... where [jcr:uuid] = $id including transient". Then the QueryIndex interface would be extended with a new method "boolean includesTransient". That way, if the query is "including transient", the query engine would only use indexes that do support transient changes. I'm OK to all those changes myself later on, as I don't think they are urgent. I wonder if both the NodeState and Root given to the query actually include the node?
        Hide
        Jukka Zitting added a comment -

        I'm not too excited about using the QueryIndex mechanism for this, as it modifies the contract as outlined by Thomas. What I had in mind originally was for IdentifierManager.resolveUUID() to do the transient diff directly before falling back to a query.

        Show
        Jukka Zitting added a comment - I'm not too excited about using the QueryIndex mechanism for this, as it modifies the contract as outlined by Thomas. What I had in mind originally was for IdentifierManager.resolveUUID() to do the transient diff directly before falling back to a query.
        Hide
        Thomas Mueller added a comment -

        > What I had in mind originally was for IdentifierManager.resolveUUID() to do the transient diff directly

        If that's simpler, then we could do that.

        We can extend the query engine to support transient changes later on, if it turns out it's a useful feature in other areas.

        Show
        Thomas Mueller added a comment - > What I had in mind originally was for IdentifierManager.resolveUUID() to do the transient diff directly If that's simpler, then we could do that. We can extend the query engine to support transient changes later on, if it turns out it's a useful feature in other areas.
        Hide
        Manfred Baedke added a comment -

        We can do that, but then we have to change the Root interface, what's exactly what I tried to avoid.

        Show
        Manfred Baedke added a comment - We can do that, but then we have to change the Root interface, what's exactly what I tried to avoid.
        Hide
        Manfred Baedke added a comment -

        Btw: I admittedly overstretched the contract of the QueryIndex API here, but in a way that might turn out useful if formalized - as Thomas already pointed out.
        Currently the QueryIndexProvider has one single collection of QueryIndexes, choosing one of them depending on the costs. Alternatively we could have multiple collections of QueryIndexes which could be combined by logical operations (As Thomas mentioned, we could also write container QueryIndexes that combine other QueryIndexes by logical operations. That could be done without changing the API).

        Show
        Manfred Baedke added a comment - Btw: I admittedly overstretched the contract of the QueryIndex API here, but in a way that might turn out useful if formalized - as Thomas already pointed out. Currently the QueryIndexProvider has one single collection of QueryIndexes, choosing one of them depending on the costs. Alternatively we could have multiple collections of QueryIndexes which could be combined by logical operations (As Thomas mentioned, we could also write container QueryIndexes that combine other QueryIndexes by logical operations. That could be done without changing the API).
        Hide
        Michael Dürig added a comment -

        but then we have to change the Root interface

        On a second thought, this might not be a bad thing: retrieving nodes by their id is an integral part of JCR. So exposing the corresponding functionality directly in the Oak API seems like a good idea and Root seems like a good place. OTOH the indexing approach - while very elegant - kind of exposes implementation details of a certain way to resolve ids to nodes.

        Show
        Michael Dürig added a comment - but then we have to change the Root interface On a second thought, this might not be a bad thing: retrieving nodes by their id is an integral part of JCR. So exposing the corresponding functionality directly in the Oak API seems like a good idea and Root seems like a good place. OTOH the indexing approach - while very elegant - kind of exposes implementation details of a certain way to resolve ids to nodes.
        Hide
        Jukka Zitting added a comment -

        Yeah, having this exposed in Root is a bit troublesome as we've tried to avoid pushing too many JCR semantics down to the Oak API. Though in this case I think doing so is the lesser evil compared to changing the query semantics.

        Could we do this in a generic manner by extending the Root.getTree() semantics to allow also identifier paths, and by adding an extension interface for pluggable identifier lookups?

        Show
        Jukka Zitting added a comment - Yeah, having this exposed in Root is a bit troublesome as we've tried to avoid pushing too many JCR semantics down to the Oak API. Though in this case I think doing so is the lesser evil compared to changing the query semantics. Could we do this in a generic manner by extending the Root.getTree() semantics to allow also identifier paths, and by adding an extension interface for pluggable identifier lookups?
        Hide
        Michael Dürig added a comment -

        Could we do this in a generic manner by extending the Root.getTree()

        This would go very well with the solution I proposed for OAK-426, which was vetoed though.

        Show
        Michael Dürig added a comment - Could we do this in a generic manner by extending the Root.getTree() This would go very well with the solution I proposed for OAK-426 , which was vetoed though.
        Hide
        Manfred Baedke added a comment -

        I uploaded a new version of the QueryIndex solution (oak-343-QueryIndex.patch) containing the changes suggested by Michael and Thomas. Later I will upload another patch which doesn't (ab)use a QueryIndex but instead changes the Root interface.
        Since the QueryIndex solution works and doesn't change any interfaces, maybe we can use it as a temporary solution until we have a final one. That would be nice because this issue is blocking stuff.

        Show
        Manfred Baedke added a comment - I uploaded a new version of the QueryIndex solution (oak-343-QueryIndex.patch) containing the changes suggested by Michael and Thomas. Later I will upload another patch which doesn't (ab)use a QueryIndex but instead changes the Root interface. Since the QueryIndex solution works and doesn't change any interfaces, maybe we can use it as a temporary solution until we have a final one. That would be nice because this issue is blocking stuff.
        Hide
        Manfred Baedke added a comment - - edited

        @Angela: Note that I had to change a test case in UserManagementTest which fails after applying the patch. I think that indeed the test case was broken, but you might want to review it.

        Show
        Manfred Baedke added a comment - - edited @Angela: Note that I had to change a test case in UserManagementTest which fails after applying the patch. I think that indeed the test case was broken, but you might want to review it.
        Hide
        Jukka Zitting added a comment -

        Since the QueryIndex solution works and doesn't change any interfaces, maybe we can use it as a temporary solution until we have a final one.

        +1 Incremental improvements FTW.

        Show
        Jukka Zitting added a comment - Since the QueryIndex solution works and doesn't change any interfaces, maybe we can use it as a temporary solution until we have a final one. +1 Incremental improvements FTW.
        Hide
        Manfred Baedke added a comment -

        Updated patch to use Double.POSITIVE_INFINITY and Double.NEGATIVE_INFINITY as index cost values.

        Show
        Manfred Baedke added a comment - Updated patch to use Double.POSITIVE_INFINITY and Double.NEGATIVE_INFINITY as index cost values.
        Hide
        Thomas Mueller added a comment -

        The patch looks good to me. It's not the final solution, there are still a few things to be changed (lookup in getCost instead of query for example), but those can be solved later on as well.

        Show
        Thomas Mueller added a comment - The patch looks good to me. It's not the final solution, there are still a few things to be changed (lookup in getCost instead of query for example), but those can be solved later on as well.
        Hide
        Thomas Mueller added a comment -

        I will work on this. I will try to integrate (maybe slightly change) the patch from Manfred Baedke and commit it. Later on we might want to find a different solution if it turns out to be problematic.

        Show
        Thomas Mueller added a comment - I will work on this. I will try to integrate (maybe slightly change) the patch from Manfred Baedke and commit it. Later on we might want to find a different solution if it turns out to be problematic.
        Hide
        Manfred Baedke added a comment -

        Very good.

        Show
        Manfred Baedke added a comment - Very good.
        Hide
        Alex Parvulescu added a comment -

        I took the patch for a spin, here's an updated version.

        I took the liberty of extracting the diff mechanism into its dedicated class (and adding some tests to that), next the index just passes on the filter and based on that, the matching nodes are extracted from the diff.

        feedback welcome!

        Show
        Alex Parvulescu added a comment - I took the patch for a spin, here's an updated version. I took the liberty of extracting the diff mechanism into its dedicated class (and adding some tests to that), next the index just passes on the filter and based on that, the matching nodes are extracted from the diff. feedback welcome!
        Hide
        Alex Parvulescu added a comment -

        I pushed the patch with rev 1438403.

        There was a small bug, Thomas pointed it out which is now fixed - the UUIDDiffIndexProviderWrapper would always return an empty list.
        Interestingly enough, being an empty list the Query would fallback to the Traversing index so the test would still not fail (at least I think that was the reason)
        Anyway, the code is in, the tests pass.

        The score and the plan are still not properly put in place. That is not a big deal, but it is the reason I'm not marking this issue as resolved.
        I'll also have to add some documentation to the code

        Show
        Alex Parvulescu added a comment - I pushed the patch with rev 1438403. There was a small bug, Thomas pointed it out which is now fixed - the UUIDDiffIndexProviderWrapper would always return an empty list. Interestingly enough, being an empty list the Query would fallback to the Traversing index so the test would still not fail (at least I think that was the reason) Anyway, the code is in, the tests pass. The score and the plan are still not properly put in place. That is not a big deal, but it is the reason I'm not marking this issue as resolved. I'll also have to add some documentation to the code
        Hide
        Thomas Mueller added a comment -

        OK, great, thanks a lot! I will take over from here. There are a few changes I would like to implement, for example avoiding to read too much data in the getCost method.

        But this should no longer block other features.

        Show
        Thomas Mueller added a comment - OK, great, thanks a lot! I will take over from here. There are a few changes I would like to implement, for example avoiding to read too much data in the getCost method. But this should no longer block other features.
        Hide
        Thomas Mueller added a comment -

        Removed block for OAK-50

        Show
        Thomas Mueller added a comment - Removed block for OAK-50
        Hide
        Alex Parvulescu added a comment - - edited

        > I will take over from here. There are a few changes I would like to implement, for example avoiding to read too much data in the getCost method.

        perfect, thanks!

        There are still issues with this fix.
        The tests marked as failing because of OAK-343 in oak-jcr (pom.xml under "known.issues") are still failing unfortunately.
        The problem is they also fail using the original patch, so probably the problem goes deeper.

        It would seem that the UUID query is looking for a node that is neither in the transient space (in the diff between rootTree.getNodeState() -> getBaseState) nor the property index. (I am seeing the nodes in the base state NodeState, but not in the property index)
        I'll try to look into this a bit.

        Show
        Alex Parvulescu added a comment - - edited > I will take over from here. There are a few changes I would like to implement, for example avoiding to read too much data in the getCost method. perfect, thanks! There are still issues with this fix. The tests marked as failing because of OAK-343 in oak-jcr (pom.xml under "known.issues") are still failing unfortunately. The problem is they also fail using the original patch, so probably the problem goes deeper. It would seem that the UUID query is looking for a node that is neither in the transient space (in the diff between rootTree.getNodeState() -> getBaseState) nor the property index. (I am seeing the nodes in the base state NodeState, but not in the property index) I'll try to look into this a bit.
        Hide
        Alex Parvulescu added a comment -

        Interesting, I was looking at a different test
        For FindAuthorizablesTest#testFindAuthorizableByAddedProperty it seemes like the diff doesn't go deep enough.

        The node hierarchy looks like

        rep:security
          /rep:authorizables
            /rep:groups
              /t
                /te
                   /testPrincipal_4e6b704e <-- with the jcr:uuid I'm interested in
                   /testGroup_1c22a39f
        

        And everything under /rep:groups is new content that apparently doesn't get picked up by the diff at all.

        Show
        Alex Parvulescu added a comment - Interesting, I was looking at a different test For FindAuthorizablesTest#testFindAuthorizableByAddedProperty it seemes like the diff doesn't go deep enough. The node hierarchy looks like rep:security /rep:authorizables /rep:groups /t /te /testPrincipal_4e6b704e <-- with the jcr:uuid I'm interested in /testGroup_1c22a39f And everything under /rep:groups is new content that apparently doesn't get picked up by the diff at all.
        Hide
        Alex Parvulescu added a comment -

        UserManagerTest.testGetNewAuthorizable()

        right, I'm seeing the same thing. Apparently the query uses the property index, and it resolved to a (probably) non existent path.

        Show
        Alex Parvulescu added a comment - UserManagerTest.testGetNewAuthorizable() right, I'm seeing the same thing. Apparently the query uses the property index, and it resolved to a (probably) non existent path.
        Hide
        Alex Parvulescu added a comment -

        I took a closer look at UserManagerTest.testGetNewAuthorizable() and the test actually fails at

          assertNotNull(userMgr.getAuthorizable(uid));
        

        and then offcourse in the finally block wen it tries to cleanup.
        So the UUID resolution needs more work

        Show
        Alex Parvulescu added a comment - I took a closer look at UserManagerTest.testGetNewAuthorizable() and the test actually fails at assertNotNull(userMgr.getAuthorizable(uid)); and then offcourse in the finally block wen it tries to cleanup. So the UUID resolution needs more work
        Hide
        Manfred Baedke added a comment -

        Yes, just made the same observation. Sorry for the confusion

        Show
        Manfred Baedke added a comment - Yes, just made the same observation. Sorry for the confusion
        Hide
        Manfred Baedke added a comment -

        Found it. The childNodeChanged() implementation stops recursion if the childNodeCount did not change, which is of course nonsense.

        Show
        Manfred Baedke added a comment - Found it. The childNodeChanged() implementation stops recursion if the childNodeCount did not change, which is of course nonsense.
        Hide
        Alex Parvulescu added a comment -

        Found it. The childNodeChanged() implementation stops recursion if the childNodeCount did not change, which is of course nonsense.

        Right, I've pushed a fix for this issue. The diff acts now like the other diffs used in indexing (see Property2IndexDiff).

        Show
        Alex Parvulescu added a comment - Found it. The childNodeChanged() implementation stops recursion if the childNodeCount did not change, which is of course nonsense. Right, I've pushed a fix for this issue. The diff acts now like the other diffs used in indexing (see Property2IndexDiff).
        Hide
        Alex Parvulescu added a comment -

        first good news: UserManagerTest.testGetNewAuthorizable() passes!
        Manfred, do you see the same thing?

        I'll look at the other ones next

        Show
        Alex Parvulescu added a comment - first good news: UserManagerTest.testGetNewAuthorizable() passes! Manfred, do you see the same thing? I'll look at the other ones next
        Hide
        Manfred Baedke added a comment -

        Only two remaining failures: UserManagerTest.testGroupCreateWithExistingPrincipal2() and UserManagerTest.testGroupCreateWithExistingPrincipal3().
        I suspect that these fail for a bug that has nothing to do with this issue.

        Show
        Manfred Baedke added a comment - Only two remaining failures: UserManagerTest.testGroupCreateWithExistingPrincipal2() and UserManagerTest.testGroupCreateWithExistingPrincipal3(). I suspect that these fail for a bug that has nothing to do with this issue.
        Hide
        Alex Parvulescu added a comment -

        Thanks for the help Manfred.

        I think we need now Angela's input on the failing tests to see if there are still issues in the fix or not.

        Show
        Alex Parvulescu added a comment - Thanks for the help Manfred. I think we need now Angela's input on the failing tests to see if there are still issues in the fix or not.
        Hide
        Manfred Baedke added a comment -

        And GroupTest.testCyclicGroups() and FindAuthorizables.testFindAuthorizableByRelativePath() are also failing.
        I'll take a quick look at these.

        Show
        Manfred Baedke added a comment - And GroupTest.testCyclicGroups() and FindAuthorizables.testFindAuthorizableByRelativePath() are also failing. I'll take a quick look at these.
        Hide
        Manfred Baedke added a comment -

        So far, I tested on my own fork with the initial patch. On the public repository, I now see additional failures in RepositoryTest.

        Show
        Manfred Baedke added a comment - So far, I tested on my own fork with the initial patch. On the public repository, I now see additional failures in RepositoryTest.
        Hide
        Alex Parvulescu added a comment -

        yes, I fixed it again
        The diff mechanism is not obvious sometimes, (this time I forgot to include the new nodes).

        Remaining failing auth related tests:

        testFindAuthorizableByRelativePath(org.apache.jackrabbit.oak.jcr.security.user.FindAuthorizablesTest): expected result
          testCreateGroupWithExistingPrincipal2(org.apache.jackrabbit.oak.jcr.security.user.UserManagerTest): Principal testPrincipal_dabdf591-e632-4d8a-ba20-387d7171f419 is already in use -> must throw AuthorizableExistsException.
          testCreateGroupWithExistingPrincipal3(org.apache.jackrabbit.oak.jcr.security.user.UserManagerTest): Principal testPrincipal_6f98fd44-854c-47e4-b1a1-e0a2f7808a18 is already in use -> must throw AuthorizableExistsException.
          testCyclicGroups(org.apache.jackrabbit.oak.jcr.security.user.GroupTest)
        
        Show
        Alex Parvulescu added a comment - yes, I fixed it again The diff mechanism is not obvious sometimes, (this time I forgot to include the new nodes). Remaining failing auth related tests: testFindAuthorizableByRelativePath(org.apache.jackrabbit.oak.jcr.security.user.FindAuthorizablesTest): expected result testCreateGroupWithExistingPrincipal2(org.apache.jackrabbit.oak.jcr.security.user.UserManagerTest): Principal testPrincipal_dabdf591-e632-4d8a-ba20-387d7171f419 is already in use -> must throw AuthorizableExistsException. testCreateGroupWithExistingPrincipal3(org.apache.jackrabbit.oak.jcr.security.user.UserManagerTest): Principal testPrincipal_6f98fd44-854c-47e4-b1a1-e0a2f7808a18 is already in use -> must throw AuthorizableExistsException. testCyclicGroups(org.apache.jackrabbit.oak.jcr.security.user.GroupTest)
        Hide
        Manfred Baedke added a comment - - edited

        FindAuthorizablesTest.testFindAuthorizableByRelativePath() fails because it expects the XPATH query

        /jcr:root/rep:security/rep:authorizables//element(*)[@prop1='v1']

        to match transient nodes, so that also seems to be a different issue (just a broken test case?).
        I think you're right and Angela should take a look, if she finds the time.

        Show
        Manfred Baedke added a comment - - edited FindAuthorizablesTest.testFindAuthorizableByRelativePath() fails because it expects the XPATH query /jcr:root/rep:security/rep:authorizables //element(*)[@prop1='v1'] to match transient nodes, so that also seems to be a different issue (just a broken test case?). I think you're right and Angela should take a look, if she finds the time.
        Hide
        Manfred Baedke added a comment -

        Finally, GroupTest.testCyclicGroups() relies on

        SELECT * FROM [nt:base] WHERE PROPERTY([rep:members], 'WeakReference') = $uuid

        to match transient nodes.

        Show
        Manfred Baedke added a comment - Finally, GroupTest.testCyclicGroups() relies on SELECT * FROM [nt:base] WHERE PROPERTY([rep:members], 'WeakReference') = $uuid to match transient nodes.
        Hide
        angela added a comment -

        i resolved most remaining TODOs related to OAK-343 and fixed the testCreateGroupWithExistingPrincipal* test.
        regarding testFindAuthorizableByRelativePath and testCyclicGroups i need to have a closer look again... as manfred suspected there are save() calls missing but adding those doesn't seems to solve the issue.

        Show
        angela added a comment - i resolved most remaining TODOs related to OAK-343 and fixed the testCreateGroupWithExistingPrincipal* test. regarding testFindAuthorizableByRelativePath and testCyclicGroups i need to have a closer look again... as manfred suspected there are save() calls missing but adding those doesn't seems to solve the issue.
        Hide
        angela added a comment -

        the testCyclicGroups is fine and just needed the save calls to be placed property (see OAK-615 for another issue that i detected by fixing)

        Show
        angela added a comment - the testCyclicGroups is fine and just needed the save calls to be placed property (see OAK-615 for another issue that i detected by fixing)
        Hide
        angela added a comment -

        testFindAuthorizableByRelativePath is caused by a bug in the code
        so, i hope that answers all the remaining questions.

        Show
        angela added a comment - testFindAuthorizableByRelativePath is caused by a bug in the code so, i hope that answers all the remaining questions.

          People

          • Assignee:
            Thomas Mueller
            Reporter:
            angela
          • Votes:
            0 Vote for this issue
            Watchers:
            7 Start watching this issue

            Dates

            • Created:
              Updated:
              Resolved:

              Development