[sling-org-apache-sling-dynamic-include] branch master updated: SLING-7742 - Prevent selector duplication when using nested components handled with SDI

[ro...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

rombert pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/sling-org-apache-sling-dynamic-include.git


The following commit(s) were added to refs/heads/master by this push:
     new e55aab0  SLING-7742 - Prevent selector duplication when using nested components handled with SDI
e55aab0 is described below

commit e55aab0f996844d430fe1e12e16befdd84ce7d88
Author: Tomasz Niedzwiedz <to...@cognifide.com>
AuthorDate: Mon May 28 19:20:53 2018 +0200

    SLING-7742 - Prevent selector duplication when using nested components handled with SDI
    
      - Minor refactor to make the selector handling easier to unit test.
      - Unit tests for various combination of selector strings
      - Update the business logic to never add the same selector twice
      - Introduce an implementation package `org.apache.sling.dynamicinclude.impl` to encapsulate the implementation details and avoid changing the versions of exported packages
      - Update the pom version in preparation for a release.
    
    Minor update by rombert@apache.org - keep bundle version unchanged.
    
    Closes #5
---
 .../sling/dynamicinclude/IncludeTagFilter.java     |  21 +---
 .../sling/dynamicinclude/impl/UrlBuilder.java      |  54 ++++++++++
 .../sling/dynamicinclude/impl/UrlBuilderTest.java  | 114 +++++++++++++++++++++
 3 files changed, 171 insertions(+), 18 deletions(-)

diff --git a/src/main/java/org/apache/sling/dynamicinclude/IncludeTagFilter.java b/src/main/java/org/apache/sling/dynamicinclude/IncludeTagFilter.java
index c8e33ca..62b64c3 100644
--- a/src/main/java/org/apache/sling/dynamicinclude/IncludeTagFilter.java
+++ b/src/main/java/org/apache/sling/dynamicinclude/IncludeTagFilter.java
@@ -39,11 +39,11 @@ import org.apache.felix.scr.annotations.Reference;
 import org.apache.felix.scr.annotations.sling.SlingFilter;
 import org.apache.felix.scr.annotations.sling.SlingFilterScope;
 import org.apache.sling.api.SlingHttpServletRequest;
-import org.apache.sling.api.request.RequestPathInfo;
 import org.apache.sling.api.resource.Resource;
 import org.apache.sling.api.resource.ResourceUtil;
 import org.apache.sling.dynamicinclude.generator.IncludeGenerator;
 import org.apache.sling.dynamicinclude.generator.IncludeGeneratorWhiteboard;
+import org.apache.sling.dynamicinclude.impl.UrlBuilder;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -160,24 +160,9 @@ public class IncludeTagFilter implements Filter {
     }
 
     private String buildUrl(Configuration config, SlingHttpServletRequest request) {
-        final boolean synthetic = ResourceUtil.isSyntheticResource(request.getResource());
         final Resource resource = request.getResource();
-        final StringBuilder builder = new StringBuilder();
-        final RequestPathInfo pathInfo = request.getRequestPathInfo();
-
-        final String resourcePath = pathInfo.getResourcePath();
-        builder.append(resourcePath);
-        if (pathInfo.getSelectorString() != null) {
-            builder.append('.').append(sanitize(pathInfo.getSelectorString()));
-        }
-        builder.append('.').append(config.getIncludeSelector());
-        builder.append('.').append(pathInfo.getExtension());
-        if (synthetic) {
-            builder.append('/').append(resource.getResourceType());
-        } else {
-            builder.append(sanitize(pathInfo.getSuffix()));
-        }
-        return builder.toString();
+        final boolean synthetic = ResourceUtil.isSyntheticResource(request.getResource());
+        return UrlBuilder.buildUrl(config.getIncludeSelector(), resource.getResourceType(), synthetic, request.getRequestPathInfo());
     }
 
     private static String sanitize(String path) {
diff --git a/src/main/java/org/apache/sling/dynamicinclude/impl/UrlBuilder.java b/src/main/java/org/apache/sling/dynamicinclude/impl/UrlBuilder.java
new file mode 100644
index 0000000..79338b9
--- /dev/null
+++ b/src/main/java/org/apache/sling/dynamicinclude/impl/UrlBuilder.java
@@ -0,0 +1,54 @@
+/*-
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sling.dynamicinclude.impl;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.sling.api.request.RequestPathInfo;
+
+import java.util.Arrays;
+
+public final class UrlBuilder {
+
+
+    public static String buildUrl(String includeSelector, String resourceType, boolean synthetic, RequestPathInfo pathInfo) {
+        final StringBuilder builder = new StringBuilder();
+
+        final String resourcePath = pathInfo.getResourcePath();
+        builder.append(resourcePath);
+        String currentSelectorString = StringUtils.defaultString(pathInfo.getSelectorString());
+        if (pathInfo.getSelectorString() != null) {
+            builder.append('.').append(currentSelectorString);
+        }
+        if (includeSelectorNotAlreadyPresent(pathInfo.getSelectors(), includeSelector)) {
+            builder.append('.').append(includeSelector);
+        }
+        builder.append('.').append(pathInfo.getExtension());
+        if (synthetic) {
+            builder.append('/').append(resourceType);
+        } else {
+            builder.append(StringUtils.defaultString(pathInfo.getSuffix()));
+        }
+        return builder.toString();
+    }
+
+    private static boolean includeSelectorNotAlreadyPresent(String[] currentSelectors, String includeSelector) {
+        return !Arrays.asList(currentSelectors).contains(includeSelector);
+    }
+}
diff --git a/src/test/java/org/apache/sling/dynamicinclude/impl/UrlBuilderTest.java b/src/test/java/org/apache/sling/dynamicinclude/impl/UrlBuilderTest.java
new file mode 100644
index 0000000..3184bb7
--- /dev/null
+++ b/src/test/java/org/apache/sling/dynamicinclude/impl/UrlBuilderTest.java
@@ -0,0 +1,114 @@
+/*-
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements.  See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership.  The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License.  You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied.  See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+
+package org.apache.sling.dynamicinclude.impl;
+
+import org.apache.commons.lang.StringUtils;
+import org.apache.sling.api.request.RequestPathInfo;
+import org.junit.Test;
+import org.junit.runner.RunWith;
+import org.mockito.Mock;
+import org.mockito.junit.MockitoJUnitRunner;
+
+import static org.hamcrest.CoreMatchers.is;
+import static org.hamcrest.MatcherAssert.assertThat;
+import static org.mockito.Mockito.when;
+
+@RunWith(MockitoJUnitRunner.class)
+public class UrlBuilderTest {
+
+    @Mock
+    private RequestPathInfo requestPathInfo;
+
+    @Test
+    public void shouldAppendTheIncludeSelectorToUrlWithNoSelectors() {
+        givenAnHtmlRequestForResource("/resource/path");
+        withSelectorString(null);
+        boolean isSyntheticResource = false;
+
+        String actualResult = UrlBuilder.buildUrl("include", "apps/example/resource/type", isSyntheticResource, requestPathInfo);
+
+        assertThat(actualResult, is("/resource/path.include.html"));
+    }
+
+    @Test
+    public void shouldAppendTheIncludeSelectorToUrlThatAlreadyContainsOtherSelectors() {
+        givenAnHtmlRequestForResource("/resource/path");
+        withSelectorString("foo.bar.baz");
+        boolean isSyntheticResource = false;
+
+        String actualResult = UrlBuilder.buildUrl("include", "apps/example/resource/type", isSyntheticResource, requestPathInfo);
+
+        assertThat(actualResult, is("/resource/path.foo.bar.baz.include.html"));
+    }
+
+    @Test
+    public void shouldAppendTheIncludeSelectorToUrlContainingMixedAlphanumericSelectors() {
+        givenAnHtmlRequestForResource("/resource/path");
+        withSelectorString("foo.2.31");
+        boolean isSyntheticResource = false;
+
+        String actualResult = UrlBuilder.buildUrl("include", "apps/example/resource/type", isSyntheticResource, requestPathInfo);
+
+        assertThat(actualResult, is("/resource/path.foo.2.31.include.html"));
+    }
+
+    @Test
+    public void shouldNotDuplicateTheIncludeSelectorIfAlreadyPresent() {
+        givenAnHtmlRequestForResource("/resource/path");
+        withSelectorString("foo.include");
+        boolean isSyntheticResource = false;
+
+        String actualResult = UrlBuilder.buildUrl("include", "apps/example/resource/type", isSyntheticResource, requestPathInfo);
+
+        assertThat(actualResult, is("/resource/path.foo.include.html"));
+    }
+
+    @Test
+    public void shouldAppendTheIncludeSelectorWhenThereIsAnotherSelectorThatAccidentallyContainsTheIncludeOne() {
+        givenAnHtmlRequestForResource("/resource/path");
+        withSelectorString("longerSelectorThatHappensToContainTheIncludeSelector");
+        boolean isSyntheticResource = false;
+
+        String actualResult = UrlBuilder.buildUrl("IncludeSelector", "apps/example/resource/type", isSyntheticResource, requestPathInfo);
+
+        assertThat(actualResult, is("/resource/path.longerSelectorThatHappensToContainTheIncludeSelector.IncludeSelector.html"));
+    }
+
+    @Test
+    public void shouldAppendSuffixForSyntheticResources() {
+        givenAnHtmlRequestForResource("/resource/path");
+        withSelectorString("foo.include");
+        boolean isSyntheticResource = true;
+
+        String actualResult = UrlBuilder.buildUrl("include", "apps/example/resource/type", isSyntheticResource, requestPathInfo);
+
+        assertThat(actualResult, is("/resource/path.foo.include.html/apps/example/resource/type"));
+    }
+
+    private void givenAnHtmlRequestForResource(String resourcePath) {
+        when(requestPathInfo.getExtension()).thenReturn("html");
+        when(requestPathInfo.getResourcePath()).thenReturn(resourcePath);
+    }
+
+    private void withSelectorString(String selectorString) {
+        when(requestPathInfo.getSelectorString()).thenReturn(selectorString);
+        when(requestPathInfo.getSelectors()).thenReturn(StringUtils.defaultString(selectorString).split("\\."));
+    }
+}