jsedding commented on a change in pull request #24:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/24#discussion_r534130787
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
##########
@@ -313,21 +306,21 @@ protected void activate(final BundleContext
bundleContext, final ResourceResolve
}
// optimize alias path allow list
- this.aliasPathAllowList = null;
- String[] aliasPathPrefix =
config.resource_resolver_optimize_alias_allowlist();
+ String[] aliasPathPrefix =
config.resource_resolver_allowed_alias_locations();
if ( aliasPathPrefix != null ) {
- final List<String> prefixList = new ArrayList<>();
- for(final String value : aliasPathPrefix) {
- if ( value.trim().length() > 0 ) {
- if ( value.trim().endsWith("/") ) {
- prefixList.add(value.trim());
+ final Set<String> prefixSet = new HashSet<>();
+ for(final String prefix : aliasPathPrefix) {
+ String value = prefix.trim();
+ if (!value.isEmpty()) {
+ if ( value.endsWith("/") ) {
+ prefixSet.add(value);
} else {
- prefixList.add(value.trim() + "/");
+ prefixSet.add(value + "/");
}
}
}
- if ( !prefixList.isEmpty()) {
- this.aliasPathAllowList = new AtomicReferenceArray<>(
prefixList.toArray(new String[prefixList.size()]));
+ if ( !prefixSet.isEmpty()) {
+ this.aliasPathAllowList = new
CopyOnWriteArrayList<>(prefixSet);
Review comment:
As I wrote earlier:
> You want to swap out one configuration for another, so updating a volatile
field with an immutable collection sounds most correct to me.
Let the field `this.aliasPathAllowedList =
Collections.unmodifiableSet(prefixSet)`. You are creating a snapshot (cache) of
the configuration values at the time of activation. The contents of the set
should never be modified thereafter. On a new configuration update, a new
immutable set should be created and atomically assigned (i.e. volatile).
I am not even 100% sure that the `volatile` keyword is required. It probably
is, because we have a method annotated with `@Modified`, implying that
configuration updates can affect the existing service and thus may happen
concurrently.
And yes, @wymsymons is right, only the interface type (in this case `Set` or
`Collection`) should be used for fields and return types. That makes reading
and refactoring the code easier.
----------------------------------------------------------------
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]
