rombert commented on code in PR #89:
URL:
https://github.com/apache/sling-org-apache-sling-resourceresolver/pull/89#discussion_r1066899199
##########
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java:
##########
@@ -163,15 +163,27 @@
@AttributeDefinition(name = "Allowed Vanity Path Location",
description ="This setting can contain a list of path prefixes, e.g.
/libs/, /content/. If " +
"such a list is configured, only vanity paths from
resources starting with this prefix " +
- " are considered. If the list is empty, all vanity paths
are used.")
+ " are considered. If the list is empty, we fallback to
resource_resolver_vanitypath_allowedlist.")
String[] resource_resolver_vanitypath_whitelist();
+ @AttributeDefinition(name = "Allowed Vanity Path Location",
+ description ="This setting can contain a list of path prefixes, e.g.
/libs/, /content/. If " +
+ "such a list is configured, only vanity paths from resources
starting with this prefix " +
+ " are considered. If the list is empty, all vanity paths are
used.")
+ String[] resource_resolver_vanitypath_allowedlist();
+
@AttributeDefinition(name = "Denied Vanity Path Location",
description ="This setting can contain a list of path prefixes, e.g.
/misc/. If " +
"such a list is configured,vanity paths from resources
starting with this prefix " +
- " are not considered. If the list is empty, all vanity
paths are used.")
+ " are not considered. If the list is empty, we fallback to
resource_resolver_vanitypath_deniedlist.")
String[] resource_resolver_vanitypath_blacklist();
+ @AttributeDefinition(name = "Denied Vanity Path Location",
+ description ="This setting can contain a list of path prefixes, e.g.
/misc/. If " +
+ "such a list is configured,vanity paths from resources starting
with this prefix " +
+ " are not considered. If the list is empty, all vanity paths are
used.")
+ String[] resource_resolver_vanitypath_deniedlist();
Review Comment:
Please use 'denylist' instead of 'deniedlist'
##########
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java:
##########
@@ -163,15 +163,27 @@
@AttributeDefinition(name = "Allowed Vanity Path Location",
description ="This setting can contain a list of path prefixes, e.g.
/libs/, /content/. If " +
"such a list is configured, only vanity paths from
resources starting with this prefix " +
- " are considered. If the list is empty, all vanity paths
are used.")
+ " are considered. If the list is empty, we fallback to
resource_resolver_vanitypath_allowedlist.")
String[] resource_resolver_vanitypath_whitelist();
+ @AttributeDefinition(name = "Allowed Vanity Path Location",
+ description ="This setting can contain a list of path prefixes, e.g.
/libs/, /content/. If " +
+ "such a list is configured, only vanity paths from resources
starting with this prefix " +
+ " are considered. If the list is empty, all vanity paths are
used.")
+ String[] resource_resolver_vanitypath_allowedlist();
Review Comment:
Please use 'allowlist' instead of 'allowedlist' . See also
https://cwiki.apache.org/confluence/display/SLING/Removal+of+problematic+language
##########
src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java:
##########
@@ -195,6 +195,11 @@ public String[] resource_resolver_virtual() {
@Override
public String[] resource_resolver_vanitypath_whitelist() {
+ return resource_resolver_vanitypath_allowedlist();
Review Comment:
This can get confusing, please return just `null`.
##########
src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java:
##########
@@ -215,6 +220,11 @@ public int
resource_resolver_vanitypath_bloomfilter_maxBytes() {
@Override
public String[] resource_resolver_vanitypath_blacklist() {
+ return resource_resolver_vanitypath_deniedlist();
Review Comment:
This can get confusing, please return just `null`.
##########
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java:
##########
@@ -79,6 +81,45 @@ private static final class FactoryRegistration {
public volatile CommonResourceResolverFactoryImpl commonFactory;
}
+ private static final class VanityPathConfigurer {
Review Comment:
Please extract a test for this class, having the logic separated makes it
easy.
##########
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java:
##########
@@ -79,6 +81,45 @@ private static final class FactoryRegistration {
public volatile CommonResourceResolverFactoryImpl commonFactory;
}
+ private static final class VanityPathConfigurer {
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ void configureVanityPathPrefixes(String[] pathPrefixes, String[]
pathPrefixesFallback,
+ String pathPrefixesPropertyName,
String pathPrefixesFallbackPropertyName,
+ Consumer<String[]>
filteredPathPrefixesConsumer) {
+ if (pathPrefixes != null) {
+ configureVanityPathPrefixes(pathPrefixes,
filteredPathPrefixesConsumer);
Review Comment:
Please log a WARN in the case where both the regular prefixes and the
fallback ones are defined; this is probably a customer error. We should rely on
the old behaviour in this case and only take information from
`config.resource_resolver_vanitypath_whitelist()`.
##########
src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java:
##########
@@ -79,6 +81,45 @@ private static final class FactoryRegistration {
public volatile CommonResourceResolverFactoryImpl commonFactory;
}
+ private static final class VanityPathConfigurer {
+ private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+ void configureVanityPathPrefixes(String[] pathPrefixes, String[]
pathPrefixesFallback,
Review Comment:
I wonder if it makes sense to pass in the whole config object, so simplify
the signature ; now we pass to fields and their names, passing in the object
might make things nicer.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
