You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@sling.apache.org by GitBox <gi...@apache.org> on 2023/01/11 11:52:55 UTC
[GitHub] [sling-org-apache-sling-resourceresolver] rombert commented on a diff in pull request #89: [SLING-11742] Provide alternative equitable terminology for properties
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: dev-unsubscribe@sling.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org