[GitHub] [sling-org-apache-sling-resourceresolver] jsedding commented on a change in pull request #46: SLING-10447 Improve the querys that are used to load vanity paths, by specifying path restrictions

[GitBox <gi...@apache.org>]

jsedding commented on a change in pull request #46:
URL: https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/46#discussion_r644618310



##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1032,34 +1070,33 @@ private boolean addEntry(final Map<String, List<MapEntry>> entryMap, final Strin
     /*
     * Update alias query based on configured alias locations
     */
-    private String updateAliasQuery(){
+    private String createAliasQuery(){
         final Set<String> allowedLocations = this.factory.getAllowedAliasLocations();
 
-        StringBuilder baseQuery = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
-        baseQuery.append(" ").append("WHERE");
+        StringBuilder query = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
+        query.append(" ").append("WHERE");
 
         if(allowedLocations.isEmpty()){
-            String jcrSystemPath = StringUtils.removeEnd(JCR_SYSTEM_PREFIX, "/");
-            baseQuery.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,")
-                    .append("\"").append(jcrSystemPath).append("\"").append("))");
+            query.append(" ").append("(").append("NOT ISDESCENDANTNODE(page,")
+                    .append("\"").append(JCR_SYSTEM_PATH).append("\"").append("))");

Review comment:
       It would be nice to reformat this query to use single quotes instead of escaped double quotes. Then both queries share the same style, and it is more readable.

##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1255,6 +1279,33 @@ private boolean loadVanityPath(final Resource resource, final Map<String, List<M
         return hasVanityPath;
     }
 
+    private String createVanityPathQuery() {
+        final String query =
+            VANITY_PATH_BASE_QUERY_DEFAULT +
+                " WHERE sling:vanityPath IS NOT NULL" +
+                Stream
+                    .of(true, false)
+                    .map(this::createVanityPathQueryPathRestriction)

Review comment:
       After reading the code generating the restrictions several times, I believe it is actually correct. But it is not very readable or intuitive.
   
   Maybe it would become more readable if you separated out all "include" restrictions and all "exclude" restrictions, each into their own collection, and then join them with the appropriate "AND" or "OR" operators?
   
   Pseudo code:
   ```
   includesExcludes = Stream.of(new VanityPathConfig(JCR_SYSTEM_PATH, true), factory.getVanityPathConfig().stream())
       .collect(Collectors.groupingBy(VanityPathConfig::isExclude)
   
   excludes = includesExclude.get(Boolean.TRUE)
   includes =  includesExclude.get(Boolean.FALSE)
   
   excludesPart =excludes.stream().map("NOT ISDESCENDANTNODE(page, '%s')").join(" AND ")
   includesPart = includes.stream().map("ISDESCENDANTNODE(page, '%s')").join(" OR ")
   
   queryRestrictions = String.format("(%s) AND (%s)", excludesPart, includesPart)
   ```
   
   Beware, I may have gotten the "OR"s and "AND"s wrong ;)
   
   PS: maybe also move `new VanityPathConfig(JCR_SYSTEM_PATH, true)` into `factory.getVanityPathConfig()`?

##########
File path: src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java
##########
@@ -112,11 +112,6 @@
                       "the ResourceResolver mapping. The default value is /etc/map.")
     String resource_resolver_map_location() default MapEntries.DEFAULT_MAP_ROOT;
 
-    @AttributeDefinition(name = "Mapping Observation",

Review comment:
       I don't understand why you are removing this configuration option. Is this actually related to improving the vanity path query? Or is this a different change?




-- 
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.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org