jsedding commented on a change in pull request #24:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/24#discussion_r518791275
##########
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:
+1 - either compute the new data-structure and swap it out with a single
assingment, or use a concurrent data-structure that could be updated
incrementally. I would prefer the former, as it reflects the atomic nature of a
config update better.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/CommonResourceResolverFactoryImpl.java
##########
@@ -472,6 +473,22 @@ public boolean hasVanityPathPrecedence() {
return configs;
}
+ @Override
+ public List<String> getAliasPath() {
+ final AtomicReferenceArray<String> includes =
this.activator.getOptimizedAliasResolutionAllowList();
+ if (includes == null) {
+ return Collections.emptyList();
+ }
+
+ final List<String> configs = new ArrayList<>();
Review comment:
I recommend using a `TreeSet`. It provides sorting and eliminates
duplicates. Also, I would avoid manual iteration. Instead if
`this.activator.getOptimizedAliasResolutionAllowList()` returns a `String[]`
you could write `new TreeSet<>(Arrays.asList(includes))`; or if it returns a
`Collection` you could just copy it into a `TreeSet` using the constructor.
##########
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:
The equivalent property for vanity paths is called
`resource_resolver_vanitypath_whitelist`. For consistency's sake I think this
should be called `resource_resolver_alias_whitelist` or maybe slightly more
descriptive `resource_resolver_alias_location_whitelist`.
##########
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 strongly agree with @rombert. `AtomicReferenceArray` is not the
correct data-structure for how you use it. It allows atomic updates to
individual array entries, but not for the entire array. You need an atomic
update of the entire array (or collection). Indeed, you reassign this field
later, which is not thread-safe because the field is not volatile.
If you want to atomically swap out the entire configuration, you need a
volatile field and make a local copy of the field before you work with it. Or
you use a thread-safe data-structure, like e.g. `CopyOnWriteArrayList` as
Robert suggests.
##########
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()) {
+ boolean allowed = false;
+ for(final String prefix : this.factory.getAliasPath()) {
Review comment:
Nit: this could be much nicer with streams:
`boolean allowed =
this.factory.getAliasPath().stream().anyMatch(path::startsWith)`
(Disclaimer: the above was typed in this comment box, so most likely an
error is hiding somewhere)
##########
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:
+1 to `getAllowedAliasPaths()` or `getAllowedAliasLocations()`
##########
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:
Or similar to the vanity equivalent "Allowed Alias Locations".
----------------------------------------------------------------
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:
us...@infra.apache.org
