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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
