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

cristic83 · 2023-01-09T15:13:33Z

This PR introduces alternative terminology for terms that are considered un-appropriate for vanity paths, as described in [1]. The approach taken was to provide additional properties to the existing properties so that backwards compatibility is kept. For the sake of backwards compatibility, the existing vanity paths properties are checked first and if these are not defined, the newly introduced properties are checked. A log message is produced in this case.

[1] https://issues.apache.org/jira/browse/SLING-11742

rombert

Thanks for the PR @cristic83 ! I've addede a few comments.

rombert · 2023-01-11T11:47:50Z

src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java

+        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();


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

rombert · 2023-01-11T11:48:04Z

src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java

+        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();


Please use 'denylist' instead of 'deniedlist'

rombert · 2023-01-11T11:48:36Z

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


This can get confusing, please return just null.

rombert · 2023-01-11T11:48:41Z

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


This can get confusing, please return just null.

rombert · 2023-01-11T11:49:51Z

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 {


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

rombert · 2023-01-11T11:51:23Z

src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java

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


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

rombert · 2023-01-11T11:52:05Z

src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryActivator.java

+    private static final class VanityPathConfigurer {
+        private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+        void configureVanityPathPrefixes(String[] pathPrefixes, String[] pathPrefixesFallback,


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.

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); }

sonarcloud · 2023-01-20T14:34:55Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

88.9% Coverage
0.0% Duplication

rombert · 2023-03-03T16:36:10Z

I've assigned myself as a reviewer to make sure it's on my TODO list. Not sure when I will get around to it, if anyone beats me to it I won't mind :-)

cziegeler · 2023-03-13T09:05:23Z

@cristic83 I suggest we use an approach where we keep the existing code out of doing conversions from old to new. With a ConfigurationPlugin we can convert transparently the old properties to the new ones. The plugin can then also trigger to update the configuration in configuration admin (async). The components consuming the configurations should implement modified events and check for changes and only act on a real configuration change

sonarcloud · 2023-04-19T15:47:50Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

88.9% Coverage
0.0% Duplication

cziegeler · 2023-06-02T08:34:20Z

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.allowlist.")


I think the fallback should be in the opposite direction: if the new property is not used, the old one will be used

cziegeler · 2023-06-02T08:34:32Z

src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java

    @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.denylist.")


same as above

cziegeler · 2023-06-02T08:35:02Z

src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java

+            "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_allowlist();
+
    @AttributeDefinition(name = "Denied Vanity Path Location",


We should remove the metatype information for this property

cziegeler · 2023-06-02T08:35:35Z

src/main/java/org/apache/sling/resourceresolver/impl/ResourceResolverFactoryConfig.java

@@ -163,15 +163,27 @@
    @AttributeDefinition(name = "Allowed Vanity Path Location",


we should remove the metatype info for this property

cziegeler · 2023-06-02T08:38:42Z

src/main/java/org/apache/sling/resourceresolver/impl/VanityPathConfigurer.java

+class VanityPathConfigurer {
+    private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+    void configureVanityPathPrefixes(String[] pathPrefixes, String[] pathPrefixesFallback,


This could be a static method just taking the Config object as a parameter and returning a String[]

cziegeler · 2023-06-03T09:21:42Z

@cristic83 I think we were a little bit unfair here with all our comments and nit picking. I think you did very good work so far. In order to spare you the burden of applying all our nitpicking, I put your code changes into a new branch and applied our comments there.
Created a new PR based on that branch #97

sonarcloud · 2023-09-13T11:46:18Z

Kudos, SonarCloud Quality Gate passed!

0 Bugs
0 Vulnerabilities
0 Security Hotspots
7 Code Smells

88.9% Coverage
0.0% Duplication

cziegeler · 2024-01-10T07:01:02Z

This has been implemented a while ago by #97

[SLING-11742] Provide alternative equitable terminology for properties

208e9d4

rombert self-requested a review January 11, 2023 11:45

rombert requested changes Jan 11, 2023

View reviewed changes

cioriia added 2 commits January 11, 2023 14:53

Implement PR feedback.

f988af2

Implement PR feedback.

463613f

rombert self-requested a review March 3, 2023 16:35

cziegeler reviewed Jun 2, 2023

View reviewed changes

cziegeler mentioned this pull request Jun 3, 2023

Issues/sling 11742 #97

Merged

cziegeler closed this Jan 10, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

cristic83 commented Jan 9, 2023

rombert left a comment

rombert Jan 11, 2023

rombert Jan 11, 2023

rombert Jan 11, 2023

rombert Jan 11, 2023

rombert Jan 11, 2023

rombert Jan 11, 2023

rombert Jan 11, 2023

cristic83 Jan 11, 2023 •

edited

Loading

sonarcloud bot commented Jan 20, 2023

rombert commented Mar 3, 2023

cziegeler commented Mar 13, 2023 •

edited

Loading

sonarcloud bot commented Apr 19, 2023

cziegeler Jun 2, 2023

cziegeler Jun 2, 2023

cziegeler Jun 2, 2023

cziegeler Jun 2, 2023

cziegeler Jun 2, 2023

cziegeler commented Jun 3, 2023

sonarcloud bot commented Sep 13, 2023

cziegeler commented Jan 10, 2024

		@@ -163,15 +163,27 @@
		@AttributeDefinition(name = "Allowed Vanity Path Location",

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

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

Conversation

cristic83 commented Jan 9, 2023

rombert left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cristic83 Jan 11, 2023 • edited Loading

Choose a reason for hiding this comment

sonarcloud bot commented Jan 20, 2023

rombert commented Mar 3, 2023

cziegeler commented Mar 13, 2023 • edited Loading

sonarcloud bot commented Apr 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cziegeler commented Jun 3, 2023

sonarcloud bot commented Sep 13, 2023

cziegeler commented Jan 10, 2024

cristic83 Jan 11, 2023 •

edited

Loading

cziegeler commented Mar 13, 2023 •

edited

Loading