Feature/sling 7768: String Interpolation for ETC Map #15

schaefa · 2019-07-30T16:28:30Z

Fix for ticket: https://issues.apache.org/jira/browse/SLING-7768 providing a way to make /etc/map configurable for different environments.

This provides the ability to define placeholders that then can be used inside /etc/map to define environment w/o having to create multiple /etc/map trees.

…ated a test suite to test the String Interpolation Provider with Map Entries

…me of the unit tests to consolidate them

…ed some common test code into MockTestUtil class

…urceResolverTest to make sure the Sling Interpolation works with Resource Resolving

# Conflicts: # pom.xml # src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java # src/test/java/org/apache/sling/resourceresolver/impl/EtcMappingResourceResolverTest.java # src/test/java/org/apache/sling/resourceresolver/impl/SimpleValueMapImpl.java # src/test/java/org/apache/sling/resourceresolver/impl/mapping/AbstractMappingMapEntriesTest.java # src/test/java/org/apache/sling/resourceresolver/impl/mapping/EtcMappingMapEntriesTest.java # src/test/java/org/apache/sling/resourceresolver/impl/mapping/MapEntriesTest.java # src/test/java/org/apache/sling/resourceresolver/util/MockTestUtil.java

…tests and fixed some merge problems

…code coverage

…uted the StringInterpolationProviderImpl to make it work

cziegeler · 2020-01-23T06:31:44Z

pom.xml

@@ -63,6 +63,32 @@
                    </execution>
                </executions>
            </plugin>
+<!--


Changes in the build should not be part of this PR - unless they are strictly required

cziegeler · 2020-01-23T06:32:32Z

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

@@ -107,6 +109,19 @@
    @Reference
    EventAdmin eventAdmin;

+    /** Event admin. */


Comment above seems to be wrong :) Please make the reference greedy and remove the commented out lines below

cziegeler · 2020-01-23T06:34:08Z

src/main/java/org/apache/sling/resourceresolver/impl/mapping/Interpolator.java

@@ -0,0 +1,154 @@
+/*


It's ok to have a copy here for getting the functionality in, but we should rather directly use the class from Apache Felix by embedding just that class (and using the maven shade plugin to move it to a different package). But we can do this in a follow up commit

The class is not exported from the Bundle (AFAI can tell) and so I could not use it.

I reverted everything back to the code from Felix with the exception of added logging statements and constants

We can embed it, exactly because it is not exported - but there is no strict need for it, other than to avoid code duplication - and as mentioned we can do this later on

cziegeler · 2020-01-23T06:34:37Z

src/main/java/org/apache/sling/resourceresolver/impl/mapping/MapEntries.java

@@ -140,8 +142,10 @@

    private boolean updateBloomFilterFile = false;

+    private StringInterpolationProvider stringInterpolationProvider;


This can be marked as final

cziegeler · 2020-01-23T06:35:50Z

...org/apache/sling/resourceresolver/impl/mapping/StringInterpolationProviderConfiguration.java

+            "The key is not permitted to contain a '=' sign and the first occurrence of '=' " +
+            "separates the key from the value. If no '=' is found the entire key / value pair " +
+            "is dropped.")
+    String[] placeHolderKeyValuePairs() default {"phv.default.host.name=localhost"};


Do we really need such a default value?

That was here because I wanted to illustrate it. I am going to remove it.

I still see it :)

cziegeler · 2020-01-23T06:36:15Z

...ain/java/org/apache/sling/resourceresolver/impl/mapping/StringInterpolationProviderImpl.java

+import java.util.Map;
+
+@Designate(ocd = StringInterpolationProviderConfiguration.class)
+@Component(name = "org.apache.sling.resourceresolver.impl.mapping.StringInterpolationProviderImpl")


name attribute is not needed

cziegeler · 2020-01-23T06:37:23Z

...ain/java/org/apache/sling/resourceresolver/impl/mapping/StringInterpolationProviderImpl.java

+                v = getVariableFromEnvironment(name);
+            } else if (TYPE_PROP.equals(type)) {
+                v = getVariableFromProperty(name);
+            } else {


we should define a type for this, like "config:" and not let all types fallback to this. The fallback prevents adding new types later on (if required)

I did not like the fallback either but I wanted to avoid forcing the user to write 'config:' for each value. That said etc-mapping is not easy by any chance and so I guess we add that.

Done. This required the tests to be adjusted as now $[config: is required.

cziegeler · 2020-01-23T06:39:04Z

...ain/java/org/apache/sling/resourceresolver/impl/mapping/StringInterpolationProviderImpl.java

+    /** Logger. */
+    private final Logger logger = LoggerFactory.getLogger(this.getClass());
+
+    private Map<String, String> placeholderEntries = new HashMap<>();


You either need to make this a concurrent map or change the logic in activate as modified() can change the map while it is used

Thanks for point this out

Opted to replace the Map in activate (modified)

cziegeler · 2020-01-23T06:39:24Z

...ain/java/org/apache/sling/resourceresolver/impl/mapping/StringInterpolationProviderImpl.java

+    /**
+     * Activates this component (called by SCR before)
+     */
+    @Activate


I suggest to change this to a constructor

You want to make the activate method part of the constructor? Can OCD being part of the constructor?

cziegeler · 2020-01-23T06:41:20Z

src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java

-        int n = fullpath.lastIndexOf("/");
-        return fullpath.substring(n+1);
-    }
+//    /**


Please remove the commented code

Not my change but will do

cziegeler · 2020-01-23T06:41:57Z

src/test/java/org/apache/sling/resourceresolver/impl/MockedResourceResolverImplTest.java

@@ -399,7 +400,7 @@ private Resource buildResource(String fullpath, Iterable<Resource> children) {
    @SuppressWarnings("unchecked")
    private Resource buildResource(String fullpath, Iterable<Resource> children, ResourceResolver resourceResolver, ResourceProvider<?> provider, String ... properties) {
        Resource resource = Mockito.mock(Resource.class);
-        Mockito.when(resource.getName()).thenReturn(getName(fullpath));
+        Mockito.when(resource.getName()).thenReturn(getResourceName(fullpath));


ResourceUtil.getName() ?

I merge in some changes from master to make it easier to merge and so this is not a change of mine.

Kept it as this was not my change

cziegeler · 2020-01-23T06:44:14Z

src/test/java/org/apache/sling/resourceresolver/impl/mapping/EtcMappingMapEntriesTest.java

@@ -16,12 +16,40 @@
 */
 package org.apache.sling.resourceresolver.impl.mapping;


There are a lot of changes in this test and I can't see whether these are just improvements or tests need to be adapted?

As far as I can tell I did not change anything here beside that I might have removed commented out code.

There are some additional tests cases to deal with types and defaults but the rest did change little.

…d by Betrand

cziegeler

Lgtm, thanks @schaefa

…s not my code

sonarcloud · 2020-01-28T22:38:26Z

SonarCloud Quality Gate failed.

0 Bugs
0 Vulnerabilities (and 1 Security Hotspot to review)
5 Code Smells

0.0% Coverage
0.0% Duplication

schaefa and others added 9 commits July 6, 2018 12:15

Initial development of the etc-map placeholder provider

028427b

This provides the ability to define placeholders that then can be used inside /etc/map to define environment w/o having to create multiple /etc/map trees.

Fix minor type

92bd020

Rename the PlaceHolderProvider to StringInterpolationProvider and cre…

511d33d

…ated a test suite to test the String Interpolation Provider with Map Entries

Adding Resource Resolver based path resolution tests and revamping so…

7e03737

…me of the unit tests to consolidate them

SLING-7768: Added /etc/map tests to the resource resolver and extract…

f3b9a00

…ed some common test code into MockTestUtil class

SLING-7768 Added the String Interpolation tests to the EtcMappingReso…

f4aae98

…urceResolverTest to make sure the Sling Interpolation works with Resource Resolving

Migrated the Sling Interplation to Lang3 StrSubtitutor, adjusted the …

bdc07c8

…tests and fixed some merge problems

SLING-7768 - Fixed all the issues reported by SonarCube and improved …

323b7d8

…code coverage

schaefa requested a review from cziegeler July 31, 2019 19:02

schaefa self-assigned this Jul 31, 2019

Andreas Schaefer added 3 commits July 31, 2019 12:14

SLING-7768 - Some more adjustments for SonarCube review

96ebac4

Merge branch 'master' into feature/SLING-7768

9b5bc80

Replaced the StrSubstitutor with the Interpolator from Felix and adjs…

980943d

…uted the StringInterpolationProviderImpl to make it work

cziegeler reviewed Jan 23, 2020

View reviewed changes

Andreas Schaefer added 2 commits January 23, 2020 11:56

Changes requested by Carsten’s Code Review or PR 15

4e7c2ef

Added Interpolator tests for additional jacoco test coverage requeste…

81e2bc9

…d by Betrand

cziegeler approved these changes Jan 24, 2020

View reviewed changes

Fixed two code smells. Ignored the ones of the Interpolator as this i…

3301e49

…s not my code

schaefa merged commit dc56b45 into master Jan 28, 2020

schaefa deleted the feature/SLING-7768 branch January 28, 2020 22:44

		@@ -140,8 +142,10 @@

		private boolean updateBloomFilterFile = false;

		private StringInterpolationProvider stringInterpolationProvider;

		@@ -16,12 +16,40 @@
		*/
		package org.apache.sling.resourceresolver.impl.mapping;

Feature/sling 7768: String Interpolation for ETC Map #15

Feature/sling 7768: String Interpolation for ETC Map #15

Conversation

schaefa commented Jul 30, 2019

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cziegeler Jan 23, 2020 • edited

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

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

cziegeler left a comment

Choose a reason for hiding this comment

sonarcloud bot commented Jan 28, 2020

cziegeler Jan 23, 2020 •

edited