wimsymons commented on a change in pull request #24:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/24#discussion_r528702798
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -111,6 +88,8 @@
static final String ALIAS_QUERY_DEFAULT = "SELECT sling:alias FROM nt:base
WHERE sling:alias IS NOT NULL";
+ static final String ALIAS_BASE_QUERY_DEFAULT = "SELECT sling:alias FROM
nt:base As page";
Review comment:
Upper-case `AS` for sake of readability?
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java
##########
@@ -1038,22 +1017,67 @@ private boolean addEntry(final Map<String,
List<MapEntry>> entryMap, final Strin
*/
private Map<String, Map<String, String>> loadAliases(final
ResourceResolver resolver) {
final Map<String, Map<String, String>> map = new ConcurrentHashMap<>();
- final String queryString = ALIAS_QUERY_DEFAULT;
- final Iterator<Resource> i =
resolver.findResources(queryString, "sql");
- while (i.hasNext()) {
+ final String queryString = updateAliasQuery();
+ final Iterator<Resource> i = resolver.findResources(queryString,
"sql");
+ while (i.hasNext()) {
final Resource resource = i.next();
loadAlias(resource, map);
}
return map;
}
- /**
+
+ /*
+ * Update alias query based on configured alias locations
+ */
+ private String updateAliasQuery(){
+ CopyOnWriteArrayList<String> allowedPaths =
this.factory.getAllowedAliasPaths();
+
+ StringBuilder baseQuery = new StringBuilder(ALIAS_BASE_QUERY_DEFAULT);
+ baseQuery.append(" ").append("WHERE");
+
+ if(!allowedPaths.isEmpty()){
Review comment:
You could drop the `!` by swapping the if {} else {} blocks.
##########
File path:
src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapConfigurationProvider.java
##########
@@ -85,5 +84,5 @@ public int compareTo(VanityPathConfig o2) {
* If empty set is returned, all paths are allowed.
* @return
*/
- Set<String> getAllowedAliasPaths();
+ CopyOnWriteArrayList<String> getAllowedAliasPaths();
Review comment:
You should just expose the interface, not the implementation. So
`List<String>` should suffice.
##########
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:
Still not 100% happy with the re-assignment of the aliasPathAllowList.
A `CopyOnWriteArrayList` should only be created/assigned once. I would do it
in the field declaration. After that there should be no re-assignments, only
add/remove/clear operations on the existing list.
----------------------------------------------------------------
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
