jsedding commented on a change in pull request #24:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/24#discussion_r545711843
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
##########
@@ -123,7 +123,8 @@
private volatile ResourceResolverFactoryConfig config = DEFAULT_CONFIG;
/** Alias path whitelist */
- private final List<String> aliasPathAllowList = new
CopyOnWriteArrayList<>();
+ @SuppressWarnings("java:S3077")
+ private volatile Set<String> aliasPathAllowList =
Collections.unmodifiableSet(Collections.emptySet());
Review comment:
`Collections.emptySet()` is already immutable. No need to wrap it with
`Collections.unmodifiableSet()`.
##########
File path:
src/test/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryTest.java
##########
@@ -108,7 +109,7 @@
@Test public void testGetAllowedAliasPaths() throws NoSuchMethodException {
assertTrue(this.commonFactory.getAllowedAliasPaths().isEmpty());
String[] allowPaths = {"/parent", "/parent0"};
- setInaccessibleField("aliasPathAllowList", activator,
Collections.unmodifiableList(Arrays.asList(allowPaths)));
+ setInaccessibleField("aliasPathAllowList", activator,
Collections.unmodifiableSet(new HashSet<>(Arrays.asList(allowPaths))));
Review comment:
I would use `TreeSet` again instead of `HashSet`, even in the test. Just
in case some logic (now or in the future) relies on the allowed alias locations
to be sorted.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
##########
@@ -123,7 +123,8 @@
private volatile ResourceResolverFactoryConfig config = DEFAULT_CONFIG;
/** Alias path whitelist */
Review comment:
Comment still contains the term "whitelist". I would argue the comment
is not needed, because the field's name describes its purpose sufficiently.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
##########
@@ -306,27 +307,29 @@ protected void activate(final BundleContext
bundleContext, final ResourceResolve
}
// optimize alias path allow list
- this.aliasPathAllowList.clear();
String[] aliasPathPrefix =
config.resource_resolver_allowed_alias_locations();
if ( aliasPathPrefix != null ) {
+ final Set<String> prefixSet = new TreeSet<>();
for(final String prefix : aliasPathPrefix) {
String value = prefix.trim();
- if (!value.isEmpty()&&value.startsWith("/")) { // absolute
path should be given
- if (value.endsWith("/")) {
- this.aliasPathAllowList.add(value);
- } else {
- this.aliasPathAllowList.add(value + "/");
- }
+ if (!value.isEmpty()) {
+ if ( value.endsWith("/") ) {
+ prefixSet.add(value);
+ } else {
+ prefixSet.add(value + "/");
}
+ }
+ }
+ if ( !prefixSet.isEmpty()) {
+ this.aliasPathAllowList =
Collections.unmodifiableSet(prefixSet);
}
- Collections.sort(this.aliasPathAllowList);
}
// vanity path white list
this.vanityPathWhiteList = null;
String[] vanityPathPrefixes =
config.resource_resolver_vanitypath_whitelist();
if ( vanityPathPrefixes != null ) {
- final List<String> prefixList = new ArrayList<>();
+ List<String> prefixList = new ArrayList<>();
Review comment:
I like `final` variables. It tells me I don't have to scan the code for
unexpected re-assignments. Also, this line is outside the scope of this PR if I
understand correctly.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
##########
@@ -306,27 +307,29 @@ protected void activate(final BundleContext
bundleContext, final ResourceResolve
}
// optimize alias path allow list
- this.aliasPathAllowList.clear();
String[] aliasPathPrefix =
config.resource_resolver_allowed_alias_locations();
if ( aliasPathPrefix != null ) {
+ final Set<String> prefixSet = new TreeSet<>();
for(final String prefix : aliasPathPrefix) {
String value = prefix.trim();
- if (!value.isEmpty()&&value.startsWith("/")) { // absolute
path should be given
- if (value.endsWith("/")) {
- this.aliasPathAllowList.add(value);
- } else {
- this.aliasPathAllowList.add(value + "/");
- }
+ if (!value.isEmpty()) {
Review comment:
What happened to the check for absolute path (starts with "/")? Is it
not needed anymore? Why?
If you do re-introduce the check, I would log a warning if it is not an
absolute path (but keep skipping empty values silently).
##########
File path:
src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java
##########
@@ -116,9 +116,9 @@ public void setup() throws Exception {
Collections.<Resource> emptySet().iterator());
//when(resourceResolverFactory.getAliasPath()).thenReturn(Arrays.asList("/child"));
- CopyOnWriteArrayList<String> aliasPath = new CopyOnWriteArrayList<>();
+ Set<String> aliasPath = new TreeSet<>();
aliasPath.add("/parent");
- for(int i = 1;i<testSize;i++){
+ for(int i = 1;i<testSize;i++){
Review comment:
Indentation looks wrong. Also the Sling code-base normally uses
white-space after the semicolons and around the comparator.
----------------------------------------------------------------
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]
