thomasmueller commented on code in PR #81: URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/81#discussion_r961648494
########## src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java: ########## @@ -1275,25 +1276,40 @@ private static boolean isAliasValid(String alias) { return invalid; } + private Iterator<Resource> queryAllVanityPaths(String query) { + log.debug("start vanityPath query: {}", query); + long queryStart = System.nanoTime(); + final Iterator<Resource> i = resolver.findResources(query, "JCR-SQL2"); + long queryElapsed = System.nanoTime() - queryStart; Review Comment: This will probably not do what you want, because JCR queries are processed lazily (while iterating, not when executed). So, I would not log at this stage. Instead, I would measure the time on a higher level, while iterating over the result. ########## src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java: ########## @@ -1303,7 +1319,13 @@ private Map<String, List<String>> loadVanityPaths(ResourceResolver resolver) { countInScope += 1; final boolean addToCache = isAllVanityPathEntriesCached() || vanityCounter.longValue() < this.factory.getMaxCachedVanityPathEntries(); - loadVanityPath(resource, resolveMapsMap, targetPaths, addToCache); + String firstVanityPath = loadVanityPath(resource, resolveMapsMap, targetPaths, addToCache); + if (supportsSort && firstVanityPath != null) { + if (previousVanityPath != null && firstVanityPath.compareTo(previousVanityPath) < 0) { + log.error("Sorting by first(vanityPath) does not appear to work; got " + firstVanityPath + " after " + previousVanityPath); Review Comment: It might be that sorting on the server and sorting on the client doesn't match for some edge cases (like, client locale doesn't match server locale). For this reason, I would recommend to log a warning, not an error. ########## src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java: ########## @@ -1275,25 +1276,40 @@ private static boolean isAliasValid(String alias) { return invalid; } + private Iterator<Resource> queryAllVanityPaths(String query) { + log.debug("start vanityPath query: {}", query); + long queryStart = System.nanoTime(); + final Iterator<Resource> i = resolver.findResources(query, "JCR-SQL2"); + long queryElapsed = System.nanoTime() - queryStart; + log.debug("end vanityPath query; elapsed {}ms", TimeUnit.NANOSECONDS.toMillis(queryElapsed)); + return i; + } + /** * Load vanity paths - search for all nodes (except under /jcr:system) * having a sling:vanityPath property */ private Map<String, List<String>> loadVanityPaths(ResourceResolver resolver) { final Map<String, List<String>> targetPaths = new ConcurrentHashMap<>(); - final String queryString = "SELECT [sling:vanityPath], [sling:redirect], [sling:redirectStatus]" + " FROM [nt:base]" + final String baseQueryString = "SELECT [sling:vanityPath], [sling:redirect], [sling:redirectStatus]" + " FROM [nt:base]" + " WHERE NOT isdescendantnode('" + queryLiteral(JCR_SYSTEM_PATH) + "')" + " AND [sling:vanityPath] IS NOT NULL"; + final String queryStringWithSort = baseQueryString + " ORDER BY FIRST([sling:vanityPath]), [jcr:path]"; - log.debug("start vanityPath query: {}", queryString); - long queryStart = System.nanoTime(); - final Iterator<Resource> i = resolver.findResources(queryString, "JCR-SQL2"); - long queryElapsed = System.nanoTime() - queryStart; - log.debug("end vanityPath query; elapsed {}ms", TimeUnit.NANOSECONDS.toMillis(queryElapsed)); + boolean supportsSort = true; + Iterator<Resource> i; Review Comment: Nitpicking: "i" is typically used for "int i"; I would use "it" for iterators. ########## src/test/java/org/apache/sling/resourceresolver/impl/mapping/InMemoryResourceProvider.java: ########## @@ -103,7 +103,7 @@ public Iterator<Resource> findResources(@NotNull ResolveContext<Void> ctx, Strin .iterator(); } - if ( "JCR-SQL2".equals(language) && "SELECT [sling:vanityPath], [sling:redirect], [sling:redirectStatus] FROM [nt:base] WHERE NOT isdescendantnode('/jcr:system') AND [sling:vanityPath] IS NOT NULL".equals(query) ) { + if ( "JCR-SQL2".equals(language) && "SELECT [sling:vanityPath], [sling:redirect], [sling:redirectStatus] FROM [nt:base] WHERE NOT isdescendantnode('/jcr:system') AND [sling:vanityPath] IS NOT NULL ORDER BY FIRST([sling:vanityPath]), [jcr:path]".equals(query) ) { Review Comment: This query doesn't use keyset pagination currently, so it can still fail when there are many entries. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: dev-unsubscr...@sling.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org