Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[SLING-11742] Provide alternative equitable terminology for properties #89

Closed
wants to merge 3 commits into from
Closed

[SLING-11742] Provide alternative equitable terminology for properties #89

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
208e9d4
[SLING-11742] Provide alternative equitable terminology for properties
cioriia Jan 6, 2023
f988af2
Implement PR feedback.
cioriia Jan 11, 2023
463613f
Implement PR feedback.
cioriia Jan 20, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
85 changes: 53 additions & 32 deletions src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import java.util.*;


import java.util.function.Consumer;
import org.apache.commons.collections4.BidiMap;
import org.apache.commons.collections4.bidimap.TreeBidiMap;
import org.apache.commons.lang3.StringUtils;
Expand All @@ -39,6 +40,7 @@
import org.apache.sling.resourceresolver.impl.providers.ResourceProviderTracker.ChangeListener;
import org.apache.sling.resourceresolver.impl.providers.RuntimeServiceImpl;
import org.apache.sling.serviceusermapping.ServiceUserMapper;
import org.jetbrains.annotations.NotNull;
import org.osgi.framework.Bundle;
import org.osgi.framework.BundleContext;
import org.osgi.framework.Constants;
Expand Down Expand Up @@ -79,6 +81,45 @@ private static final class FactoryRegistration {
public volatile CommonResourceResolverFactoryImpl commonFactory;
}

private static final class VanityPathConfigurer {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please extract a test for this class, having the logic separated makes it easy.

private final Logger logger = LoggerFactory.getLogger(this.getClass());

void configureVanityPathPrefixes(String[] pathPrefixes, String[] pathPrefixesFallback,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Author

@cristic83 cristic83 Jan 11, 2023 •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I attempted to implement this suggestion and the result looks like below. I don't think it's all that nicer,what do you think?

void configureVanityPAthPrefix(ResourceResolverFactoryConfig config, Consumer<String[]> allowVanityPathConsumer, Consumer<String[]> denyVanityPathConsumer) { configureVanityPathPrefixes(config.resource_resolver_vanitypath_whitelist(), config.resource_resolver_vanitypath_allowlist(), "resource_resolver_vanitypath_whitelist", "resource_resolver_vanitypath_allowlist", allowVanityPathConsumer); configureVanityPathPrefixes(config.resource_resolver_vanitypath_blacklist(), config.resource_resolver_vanitypath_denylist(), "resource_resolver_vanitypath_blacklist", "resource_resolver_vanitypath_denylist", denyVanityPathConsumer); }

String pathPrefixesPropertyName, String pathPrefixesFallbackPropertyName,
Consumer<String[]> filteredPathPrefixesConsumer) {
if (pathPrefixes != null) {
configureVanityPathPrefixes(pathPrefixes, filteredPathPrefixesConsumer);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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().

} else {
logger.debug("The " + pathPrefixesPropertyName + " was null. Using the " +
pathPrefixesFallbackPropertyName + " instead if defined.");
if (pathPrefixesFallback != null) {
configureVanityPathPrefixes(pathPrefixesFallback, filteredPathPrefixesConsumer);
}
}
}

private static void configureVanityPathPrefixes(String[] pathPrefixes, Consumer<String[]> pathPrefixesConsumer) {
final List<String> filterVanityPaths = filterVanityPathPrefixes(pathPrefixes);
if (filterVanityPaths.size() > 0) {
pathPrefixesConsumer.accept(filterVanityPaths.toArray(new String[filterVanityPaths.size()]));
}
}

@NotNull
private static List<String> filterVanityPathPrefixes(String[] vanityPathPrefixes) {
final List<String> prefixList = new ArrayList<>();
for (final String value : vanityPathPrefixes) {
if (value.trim().length() > 0) {
if (value.trim().endsWith("/")) {
prefixList.add(value.trim());
} else {
prefixList.add(value.trim() + "/");
}
}
}
return prefixList;
}
}

/** Logger. */
private final Logger logger = LoggerFactory.getLogger(this.getClass());
Expand Down Expand Up @@ -145,6 +186,8 @@ private static final class FactoryRegistration {
/** Factory registration. */
private volatile FactoryRegistration factoryRegistration;

private final VanityPathConfigurer vanityPathConfigurer = new VanityPathConfigurer();

/**
* Get the resource decorator tracker.
*/
Expand Down Expand Up @@ -341,40 +384,18 @@ protected void activate(final BundleContext bundleContext, final ResourceResolve

// vanity path white list
this.vanityPathWhiteList = null;
String[] vanityPathPrefixes = config.resource_resolver_vanitypath_whitelist();
if ( vanityPathPrefixes != null ) {
final List<String> prefixList = new ArrayList<>();
for(final String value : vanityPathPrefixes) {
if ( value.trim().length() > 0 ) {
if ( value.trim().endsWith("/") ) {
prefixList.add(value.trim());
} else {
prefixList.add(value.trim() + "/");
}
}
}
if ( prefixList.size() > 0 ) {
this.vanityPathWhiteList = prefixList.toArray(new String[prefixList.size()]);
}
}
vanityPathConfigurer.configureVanityPathPrefixes(config.resource_resolver_vanitypath_whitelist(),
config.resource_resolver_vanitypath_allowedlist(),
"resource_resolver_vanitypath_whitelist",
"resource_resolver_vanitypath_allowedlist",
filteredPrefixes -> this.vanityPathWhiteList = filteredPrefixes);
// vanity path black list
this.vanityPathBlackList = null;
vanityPathPrefixes = config.resource_resolver_vanitypath_blacklist();
if ( vanityPathPrefixes != null ) {
final List<String> prefixList = new ArrayList<>();
for(final String value : vanityPathPrefixes) {
if ( value.trim().length() > 0 ) {
if ( value.trim().endsWith("/") ) {
prefixList.add(value.trim());
} else {
prefixList.add(value.trim() + "/");
}
}
}
if ( prefixList.size() > 0 ) {
this.vanityPathBlackList = prefixList.toArray(new String[prefixList.size()]);
}
}
vanityPathConfigurer.configureVanityPathPrefixes(config.resource_resolver_vanitypath_blacklist(),
config.resource_resolver_vanitypath_deniedlist(),
"resource_resolver_vanitypath_blacklist",
"resource_resolver_vanitypath_deniedlist",
filteredPrefixes -> this.vanityPathBlackList = filteredPrefixes);

// check for required property
Set<String> requiredResourceProvidersLegacy = getStringSet(config.resource_resolver_required_providers());
Expand Down
16 changes: 14 additions & 2 deletions src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,15 +163,27 @@
@AttributeDefinition(name = "Allowed Vanity Path Location",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we should remove the metatype info for this property

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use 'allowlist' instead of 'allowedlist' . See also https://cwiki.apache.org/confluence/display/SLING/Removal+of+problematic+language


@AttributeDefinition(name = "Denied Vanity Path Location",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should remove the metatype information for this property

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();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use 'denylist' instead of 'deniedlist'


@AttributeDefinition(name = "Vanity Path Precedence",
description ="This flag controls whether vanity paths" +
" will have precedence over existing /etc/map mapping when resolving paths to resources")
Expand Down
10 changes: 10 additions & 0 deletions src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,11 @@ public String[] resource_resolver_virtual() {

@Override
public String[] resource_resolver_vanitypath_whitelist() {
return resource_resolver_vanitypath_allowedlist();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can get confusing, please return just null.

}

@Override
public String[] resource_resolver_vanitypath_allowedlist() {
return null;
}

Expand All @@ -215,6 +220,11 @@ public int resource_resolver_vanitypath_bloomfilter_maxBytes() {

@Override
public String[] resource_resolver_vanitypath_blacklist() {
return resource_resolver_vanitypath_deniedlist();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This can get confusing, please return just null.

}

@Override
public String[] resource_resolver_vanitypath_deniedlist() {
return null;
}

Expand Down