rombert commented on a change in pull request #24:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/24#discussion_r518739404
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java
##########
@@ -149,6 +149,12 @@
" and on the alias update time if the number of aliases
is huge (over 10000).")
boolean resource_resolver_optimize_alias_resolution() default true;
+ @AttributeDefinition(name = "Allowed Optimize alias path",
Review comment:
Nit: I think "Allowed optimized alias paths" communicated the intent
better.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
##########
@@ -128,6 +129,9 @@
private volatile ResourceResolverFactoryConfig config = DEFAULT_CONFIG;
+ /** Alias path whitelist */
+ private AtomicReferenceArray<String> aliasPathAllowList;
Review comment:
I think it's simpler if you have a `volatile` list here and for thread
safety either make them immutable , e.g. `Collections.unmodifiableList(...)` or
use a thread-safe list like `CopyOnWriteArrayList`.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1047,13 +1047,40 @@ private boolean addEntry(final Map<String,
List<MapEntry>> entryMap, final Strin
return map;
}
- /**
+ private boolean isValidAliasPath(final String path){
+ if (path == null) {
+ throw new IllegalArgumentException("Unexpected null path");
+ }
+
+ // ignore system tree
+ if (path.startsWith(JCR_SYSTEM_PREFIX)) {
+ log.debug("loadAliases: Ignoring {}", path);
+ return false;
+ }
+
+ // check white list
+ if ( this.factory.getAliasPath() != null &&
!this.factory.getAliasPath().isEmpty()) {
Review comment:
getAliasPath() cannot return null, right?
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1047,13 +1047,40 @@ private boolean addEntry(final Map<String,
List<MapEntry>> entryMap, final Strin
return map;
}
- /**
+ private boolean isValidAliasPath(final String path){
Review comment:
Indentation looks inconsistent in this method, please check.
##########
File path:
src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -2074,4 +2102,28 @@ public void run() {
}
}
}
+
+ @Test
+ public void testIsValidAliasPath() throws Exception {
+ Method method = MapEntries.class.getDeclaredMethod("isValidAliasPath",
String.class);
+ method.setAccessible(true);
+
+ // ignore system tree - path should not start with /jcr:system -
+ assertFalse((Boolean)method.invoke(mapEntries, "/jcr:system/node"));
+ //valid alias path
+ assertTrue((Boolean)method.invoke(mapEntries, "/parent"));
+ // notallowedparent is not valid configured alias path
+ assertFalse((Boolean)method.invoke(mapEntries, "/notallowedparent"));
+ }
+
+ @Test
+ public void testNullAliasPath() throws NoSuchMethodException,
IllegalAccessException {
Review comment:
This method does not fail if an exception is not thrown. You probably
want either `@Test(expected=...)` or to use the `ExpectedException` JUnit rule.
I you remove the usage of reflection (see comment on previous method) you can
use the first option.
##########
File path:
src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -2074,4 +2102,28 @@ public void run() {
}
}
}
+
+ @Test
+ public void testIsValidAliasPath() throws Exception {
Review comment:
Have you considered making that method have `package` level access by
removing the `private` modifier? It might make the code simpler with no ill
side-effects.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java
##########
@@ -78,4 +78,6 @@ public int compareTo(VanityPathConfig o2) {
* If <code>null</code> is returned, all paths are allowed.
*/
List<VanityPathConfig> getVanityPathConfig();
+
+ List<String> getAliasPath();
Review comment:
I think getAllowedAliasPaths would be better.Also please document what
this returns, if returns is nullable and what that means, etc.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java
##########
@@ -149,6 +149,12 @@
" and on the alias update time if the number of aliases
is huge (over 10000).")
boolean resource_resolver_optimize_alias_resolution() default true;
+ @AttributeDefinition(name = "Allowed Optimize alias path",
+ description = "This setting can contain a list of path prefixes, e.g.
/libs/, /content/. If " +
+ "such a list is configured, for alias optimization, only
paths from resources starting with this prefix " +
+ "are considered. If the list is empty, all paths are used.)")
+ String[] resource_resolver_optimize_alias_allowedlist();
Review comment:
Nit: I think "allowlist" is cleared than "allowedlist"
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
##########
@@ -304,6 +312,25 @@ protected void activate(final BundleContext bundleContext,
final ResourceResolve
this.observationPaths[i] = new Path(paths[i]);
}
+ // optimize alias path allow list
+ this.aliasPathAllowList = null;
Review comment:
I think you should swap/null out the field at the end, otherwise there
is a window of opportunity when this field is null during recalculation.
----------------------------------------------------------------
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:
[email protected]
