Merge pull request #528 from apache/OAK-9737

OAK-9737 : Avoid duplicate tree resolution by using ResultRow.getTree

oak-core/src/main/java/org/apache/jackrabbit/oak/plugins/identifier/IdentifierManager.java

@@ -133,13 +133,17 @@ public Tree getTree(String identifier) {
 
             checkArgument(UUIDUtils.isValidUUID(uuid), "Not a valid identifier '" + identifier + '\'');
 
-            String basePath = resolveUUID(uuid);
-            if (basePath == null) {
+            Tree tree = resolveUUIDToTree(createPropertyValue(uuid));
+            if (tree == null) {
                 return null;
             } else if (k == -1) {
-                return root.getTree(basePath);
+                return tree;
             } else {
-                return root.getTree(PathUtils.concat(basePath, identifier.substring(k + 1)));
+                Iterable<String> subpath = PathUtils.elements(identifier.substring(k + 1));
+                for (String element : subpath) {
+                    tree = tree.getChild(element);
+                }
+                return tree;
             }
         }
     }
@@ -171,7 +175,7 @@ public String getPath(String identifier) {
     public String getPath(PropertyState referenceValue) {
         int type = referenceValue.getType().tag();
         if (type == PropertyType.REFERENCE || type == PropertyType.WEAKREFERENCE) {
-            return resolveUUID(referenceValue);
+            return resolveUUID(PropertyValues.create(referenceValue));
         } else {
             throw new IllegalArgumentException("Invalid value type");
         }
@@ -338,14 +342,17 @@ public boolean apply(String path) {
 
     @Nullable
     public String resolveUUID(String uuid) {
-        return resolveUUID(StringPropertyState.stringProperty("", uuid));
+        return resolveUUID(createPropertyValue(uuid));
     }
-
-    private String resolveUUID(PropertyState uuid) {
-        return resolveUUID(PropertyValues.create(uuid));
+
+    @Nullable
+    private String resolveUUID(@NotNull PropertyValue uuid) {
+        Tree tree = resolveUUIDToTree(uuid);
+        return (tree == null) ? null : tree.getPath();
     }
 
-    private String resolveUUID(PropertyValue uuid) {
+    @Nullable
+    private Tree resolveUUIDToTree(@NotNull PropertyValue uuid) {
         try {
             Map<String, PropertyValue> bindings = Collections.singletonMap("id", uuid);
             Result result = root.getQueryEngine().executeQuery(
@@ -355,20 +362,24 @@ private String resolveUUID(PropertyValue uuid) {
                     Query.JCR_SQL2,
                     bindings, NO_MAPPINGS);
 
-            String path = null;
+            Tree tree = null;
             for (ResultRow rr : result.getRows()) {
-                if (path != null) {
-                    log.error("multiple results for identifier lookup: " + path + " vs. " + rr.getPath());
+                if (tree != null) {
+                    log.error("multiple results for identifier lookup: " + tree.getPath() + " vs. " + rr.getPath());
                     return null;
                 } else {
-                    path = rr.getPath();
+                    tree = rr.getTree(null);
                 }
             }
-            return path;
+            return tree;
         } catch (ParseException ex) {
             log.error("query failed", ex);
             return null;
         }
     }
-
+
+    @NotNull
+    private static PropertyValue createPropertyValue(@NotNull String uuid) {
+        return PropertyValues.create(StringPropertyState.stringProperty("", uuid));
+    }
 }

oak-core/src/main/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlManagerImpl.java

@@ -375,10 +375,11 @@ public AccessControlPolicy[] getEffectivePolicies(@NotNull Set<Principal> princi
         Set<String> processed = Sets.newHashSet();
         Predicate<Tree> predicate = new PrincipalPredicate(principals);
         for (ResultRow row : aceResult.getRows()) {
-            String acePath = row.getPath();
+            Tree aceTree = row.getTree(null);
+            String acePath = aceTree.getPath();
             String aclName = Text.getName(Text.getRelativeParent(acePath, 1));
 
-            Tree accessControlledTree = r.getTree(Text.getRelativeParent(acePath, 2));
+            Tree accessControlledTree = aceTree.getParent().getParent();
             if (!POLICY_NODE_NAMES.contains(aclName) || !accessControlledTree.exists()) {
                 log.debug("Isolated access control entry -> ignore query result at {}", acePath);
                 continue;
@@ -488,7 +489,7 @@ private JackrabbitAccessControlList createPrincipalACL(@Nullable String oakPath,
         List<ACE> entries = new ArrayList<>();
         Map<String, Principal> principalMap = new HashMap<>();
         for (ResultRow row : aceResult.getRows()) {
-            Tree aceTree = root.getTree(row.getPath());
+            Tree aceTree = row.getTree(null);
             if (Util.isACE(aceTree, ntMgr)) {
                 String aclPath = Text.getRelativeParent(aceTree.getPath(), 1);
                 String path;

oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/UserProvider.java

@@ -240,8 +240,7 @@ Tree getAuthorizableByPrincipal(@NotNull Principal principal) {
 
             Iterator<? extends ResultRow> rows = result.getRows().iterator();
             if (rows.hasNext()) {
-                String path = rows.next().getPath();
-                return root.getTree(path);
+                return rows.next().getTree(null);
             }
         } catch (ParseException ex) {
             log.error("Failed to retrieve authorizable by principal", ex);

oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/query/ResultRowToAuthorizable.java

@@ -16,8 +16,6 @@
  */
 package org.apache.jackrabbit.oak.security.user.query;
 
-import javax.jcr.RepositoryException;
-
 import com.google.common.base.Function;
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.oak.api.ResultRow;
@@ -33,6 +31,8 @@
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
+import javax.jcr.RepositoryException;
+
 /**
  * Function to convert query result rows {@link Authorizable}s of a given
  * target type.
@@ -44,12 +44,14 @@ class ResultRowToAuthorizable implements Function<ResultRow, Authorizable> {
     private final UserManagerImpl userManager;
     private final Root root;
     private final AuthorizableType targetType;
+    private final String selectorName;
 
     ResultRowToAuthorizable(@NotNull UserManagerImpl userManager, @NotNull Root root,
-                            @Nullable AuthorizableType targetType) {
+                            @Nullable AuthorizableType targetType, @NotNull String[] selectorNames) {
         this.userManager = userManager;
         this.root = root;
         this.targetType = (targetType == null || AuthorizableType.AUTHORIZABLE == targetType) ? null : targetType;
+        this.selectorName = (selectorNames.length == 0) ? null : selectorNames[0];
     }
 
     @Nullable
@@ -63,9 +65,11 @@ public Authorizable apply(@Nullable ResultRow row) {
     private Authorizable getAuthorizable(@Nullable ResultRow row) {
         Authorizable authorizable = null;
         if (row != null) {
-            String resultPath = row.getValue(QueryConstants.JCR_PATH).getValue(Type.STRING);
+            Tree tree = row.getTree(selectorName);
+            if (!tree.exists()) {
+                return null;
+            }
             try {
-                Tree tree = root.getTree(resultPath);
                 AuthorizableType type = UserUtil.getType(tree);
                 while (tree.exists() && !tree.isRoot() && type == null) {
                     tree = tree.getParent();
@@ -75,7 +79,7 @@ private Authorizable getAuthorizable(@Nullable ResultRow row) {
                     authorizable = userManager.getAuthorizable(tree);
                 }
             } catch (RepositoryException e) {
-                log.debug("Failed to access authorizable {}", resultPath);
+                log.debug("Failed to access authorizable {}", row.getPath());
             }
         }
         return authorizable;

oak-core/src/main/java/org/apache/jackrabbit/oak/security/user/query/UserQueryManager.java

@@ -273,7 +273,7 @@ private Iterator<Authorizable> findAuthorizables(@NotNull String statement,
                     statement, javax.jcr.query.Query.XPATH, limit, offset,
                     NO_BINDINGS, namePathMapper.getSessionLocalMappings());
             Iterable<? extends ResultRow> resultRows = query.getRows();
-            Iterator<Authorizable> authorizables = Iterators.transform(resultRows.iterator(), new ResultRowToAuthorizable(userManager, root, type));
+            Iterator<Authorizable> authorizables = Iterators.transform(resultRows.iterator(), new ResultRowToAuthorizable(userManager, root, type, query.getSelectorNames()));
             return Iterators.filter(authorizables, new UniqueResultPredicate());
         } catch (ParseException e) {
             throw new RepositoryException("Invalid user query "+statement, e);

oak-core/src/test/java/org/apache/jackrabbit/oak/security/authorization/accesscontrol/AccessControlManagerImplTest.java

@@ -1745,12 +1745,7 @@ public void testEffectivePolicyIsolatedAce() throws Exception {
         ace.setProperty(REP_PRINCIPAL_NAME, testPrincipal.getName());
         ace.setProperty(REP_PRIVILEGES, ImmutableList.of(JCR_READ), Type.NAMES);
 
-        ResultRow row = when(mock(ResultRow.class).getPath()).thenReturn(ace.getPath()).getMock();
-        Iterable rows = ImmutableList.of(row);
-        Result res = mock(Result.class);
-        when(res.getRows()).thenReturn(rows).getMock();
-        QueryEngine qe = mock(QueryEngine.class);
-        when(qe.executeQuery(anyString(), anyString(), any(Map.class), any(Map.class))).thenReturn(res);
+        QueryEngine qe = mockQueryEngine(ace);
         when(r.getQueryEngine()).thenReturn(qe);
 
         AccessControlManagerImpl mgr = createAccessControlManager(r, getNamePathMapper());
@@ -1770,18 +1765,23 @@ public void testEffectivePolicyWrongPrimaryType() throws Exception {
         ace.setProperty(REP_PRINCIPAL_NAME, testPrincipal.getName());
         ace.setProperty(REP_PRIVILEGES, ImmutableList.of(JCR_READ), Type.NAMES);
 
-        ResultRow row = when(mock(ResultRow.class).getPath()).thenReturn(ace.getPath()).getMock();
-        Iterable rows = ImmutableList.of(row);
-        Result res = mock(Result.class);
-        when(res.getRows()).thenReturn(rows).getMock();
-        QueryEngine qe = mock(QueryEngine.class);
-        when(qe.executeQuery(anyString(), anyString(), any(Map.class), any(Map.class))).thenReturn(res);
+        QueryEngine qe = mockQueryEngine(ace);
         when(r.getQueryEngine()).thenReturn(qe);
 
         AccessControlManagerImpl mgr = createAccessControlManager(r, getNamePathMapper());
         AccessControlPolicy[] policies = mgr.getEffectivePolicies(ImmutableSet.of(testPrincipal));
         assertPolicies(policies, 0);
     }
+
+    private static QueryEngine mockQueryEngine(@NotNull Tree aceTree) throws Exception {
+        ResultRow row = when(mock(ResultRow.class).getTree(null)).thenReturn(aceTree).getMock();
+        Iterable rows = ImmutableList.of(row);
+        Result res = mock(Result.class);
+        when(res.getRows()).thenReturn(rows).getMock();
+        QueryEngine qe = mock(QueryEngine.class);
+        when(qe.executeQuery(anyString(), anyString(), any(Map.class), any(Map.class))).thenReturn(res);
+        return qe;
+    }
 
     @Test(expected = RepositoryException.class)
     public void testEffectivePolicyFailingQuery() throws Exception {

oak-core/src/test/java/org/apache/jackrabbit/oak/security/user/query/ResultRowToAuthorizableTest.java

@@ -19,16 +19,13 @@
 import org.apache.jackrabbit.api.security.user.Authorizable;
 import org.apache.jackrabbit.api.security.user.User;
 import org.apache.jackrabbit.oak.api.ContentSession;
-import org.apache.jackrabbit.oak.api.PropertyValue;
 import org.apache.jackrabbit.oak.api.ResultRow;
 import org.apache.jackrabbit.oak.api.Root;
 import org.apache.jackrabbit.oak.api.Tree;
 import org.apache.jackrabbit.oak.commons.PathUtils;
-import org.apache.jackrabbit.oak.plugins.memory.PropertyValues;
 import org.apache.jackrabbit.oak.plugins.tree.TreeUtil;
 import org.apache.jackrabbit.oak.security.user.AbstractUserTest;
 import org.apache.jackrabbit.oak.security.user.UserManagerImpl;
-import org.apache.jackrabbit.oak.spi.query.QueryConstants;
 import org.apache.jackrabbit.oak.spi.security.user.AuthorizableType;
 import org.jetbrains.annotations.NotNull;
 import org.jetbrains.annotations.Nullable;
@@ -61,12 +58,11 @@ public void before() throws Exception {
     @NotNull
     private ResultRowToAuthorizable createResultRowToAuthorizable(@NotNull Root r, @Nullable AuthorizableType targetType) {
         UserManagerImpl umgr = createUserManagerImpl(r);
-        return new ResultRowToAuthorizable(umgr, r, targetType);
+        return new ResultRowToAuthorizable(umgr, r, targetType, new String[0]);
     }
 
-    private static ResultRow createResultRow(@NotNull String path) {
-        PropertyValue propValue = PropertyValues.newPath(path);
-        return when(mock(ResultRow.class).getValue(QueryConstants.JCR_PATH)).thenReturn(propValue).getMock();
+    private static ResultRow createResultRow(@NotNull Tree tree) {
+        return when(mock(ResultRow.class).getTree(null)).thenReturn(tree).getMock();
     }
 
     @Test
@@ -76,20 +72,18 @@ public void testApplyNullRow() {
 
     @Test
     public void testRowToNonExistingTree() {
-        PropertyValue propValue = PropertyValues.newPath("/path/to/nonExisting/tree");
-        ResultRow row = when(mock(ResultRow.class).getValue(QueryConstants.JCR_PATH)).thenReturn(propValue).getMock();
-        assertNull(groupRrta.apply(row));
+        assertNull(groupRrta.apply(createResultRow(root.getTree("/path/to/nonExisting/tree"))));
     }
 
     @Test
     public void testRowToRootTree() {
-        assertNull(groupRrta.apply(createResultRow(PathUtils.ROOT_PATH)));
+        assertNull(groupRrta.apply(createResultRow(root.getTree(PathUtils.ROOT_PATH))));
     }
 
     @Test
     public void testRowToUserTree() throws Exception {
         User user = getTestUser();
-        ResultRow row = createResultRow(user.getPath());
+        ResultRow row = createResultRow(root.getTree(user.getPath()));
 
         assertNull(groupRrta.apply(row));
 
@@ -103,7 +97,7 @@ public void testRowToUserSubTree() throws Exception {
         User user = getTestUser();
         Tree t = root.getTree(user.getPath());
         t = TreeUtil.addChild(t, "child", NT_OAK_UNSTRUCTURED);
-        ResultRow row = createResultRow(t.getPath());
+        ResultRow row = createResultRow(t);
 
         assertNull(groupRrta.apply(row));
 
@@ -115,7 +109,8 @@ public void testRowToUserSubTree() throws Exception {
     @Test
     public void testRowToNonExistingUserSubTree() throws Exception {
         User user = getTestUser();
-        ResultRow row = createResultRow(PathUtils.concat(user.getPath(), "child"));
+        Tree tree = root.getTree(user.getPath()).getChild("child");
+        ResultRow row = createResultRow(tree);
 
         assertNull(userRrta.apply(row));
     }
@@ -128,7 +123,7 @@ public void testRowNonAccessibleUserTree() throws Exception {
         try (ContentSession cs = login(new SimpleCredentials(user.getID(), user.getID().toCharArray()))) {
             Root r = cs.getLatestRoot();
             ResultRowToAuthorizable rrta = createResultRowToAuthorizable(r, null);
-            assertNull(rrta.apply(createResultRow(userPath)));
+            assertNull(rrta.apply(createResultRow(r.getTree(userPath))));
         }
     }
 }