Fix endless loop with floating relocation pattern

Fixes #4. Thank you @jkebinger for reporting!
basepom · Sep 14, 2024 · 5636851 · 5636851
1 parent cc58bca
commit 5636851
Show file tree

Hide file tree

Showing 3 changed files with 129 additions and 39 deletions.
diff --git a/pom.xml b/pom.xml
@@ -12,7 +12,8 @@
 ~   See the License for the specific language governing permissions and
 ~   limitations under the License.
 -->
-<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
+<project xmlns="http://maven.apache.org/POM/4.0.0" xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
+    xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
     <modelVersion>4.0.0</modelVersion>
 
     <parent>
@@ -61,8 +62,22 @@
         <project.build.targetJdk>11</project.build.targetJdk>
         <project.moduleName>org.basepom.policy</project.moduleName>
         <basepom.javadoc.legacy-mode>true</basepom.javadoc.legacy-mode>
+
+        <dep.junit5.version>5.11.0</dep.junit5.version>
     </properties>
 
+    <dependencyManagement>
+        <dependencies>
+            <dependency>
+                <groupId>org.junit</groupId>
+                <artifactId>junit-bom</artifactId>
+                <version>${dep.junit5.version}</version>
+                <type>pom</type>
+                <scope>import</scope>
+            </dependency>
+        </dependencies>
+    </dependencyManagement>
+
     <dependencies>
         <dependency>
             <groupId>org.apache.maven.plugins</groupId>
@@ -83,5 +98,17 @@
             <version>${dep.spotbugs.version}</version>
             <optional>true</optional>
         </dependency>
+
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-api</artifactId>
+            <scope>test</scope>
+        </dependency>
+
+        <dependency>
+            <groupId>org.junit.jupiter</groupId>
+            <artifactId>junit-jupiter-engine</artifactId>
+            <scope>test</scope>
+        </dependency>
     </dependencies>
 </project>
diff --git a/src/main/java/org/basepom/maven/shade/CollectingManifestResourceTransformer.java b/src/main/java/org/basepom/maven/shade/CollectingManifestResourceTransformer.java
@@ -11,8 +11,11 @@
  * See the License for the specific language governing permissions and
  * limitations under the License.
  */
+
 package org.basepom.maven.shade;
 
+import static java.lang.String.format;
+
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.ArrayList;
@@ -26,17 +29,17 @@
 import java.util.jar.JarFile;
 import java.util.jar.JarOutputStream;
 import java.util.jar.Manifest;
+
 import org.apache.maven.plugins.shade.relocation.Relocator;
 import org.apache.maven.plugins.shade.resource.ManifestResourceTransformer;
 
 /**
- * Extends {@link ManifestResourceTransformer} to collect the additional sections
- * in the jar manifests. This keeps the build information from the internal jars in the
- * shaded jars. Submitted back to Maven as <a href="https://issues.apache.org/jira/browse/MSHADE-165">MSHADE-165</a>.
+ * Extends {@link ManifestResourceTransformer} to collect the additional sections in the jar manifests. This keeps the build information from the internal jars
+ * in the shaded jars. Submitted back to Maven as <a href="https://issues.apache.org/jira/browse/MSHADE-165">MSHADE-165</a>.
  */
 public final class CollectingManifestResourceTransformer
-        extends ManifestResourceTransformer
-{
+        extends ManifestResourceTransformer {
+
     private final List<String> defaultAttributes = Arrays.asList("Export-Package",
             "Import-Package",
             "Provide-Capability",
@@ -56,22 +59,19 @@ public final class CollectingManifestResourceTransformer
     private long time = Long.MIN_VALUE;
 
     /**
-     * If set, the transformer will collect all sections from jar manifests
-     * and adds them to the main manifest.
+     * If set, the transformer will collect all sections from jar manifests and adds them to the main manifest.
      */
     public void setCollectSections(boolean collectSections) {
         this.collectSections = collectSections;
     }
 
     @Override
-    public void setMainClass( String mainClass )
-    {
+    public void setMainClass(String mainClass) {
         this.mainClass = mainClass;
     }
 
     @Override
-    public void setManifestEntries(Map<String, Object> manifestEntries)
-    {
+    public void setManifestEntries(Map<String, Object> manifestEntries) {
         if (manifestEntries != null) {
             this.manifestEntries = new HashMap<>(manifestEntries);
         } else {
@@ -80,8 +80,7 @@ public void setManifestEntries(Map<String, Object> manifestEntries)
     }
 
     @Override
-    public void setAdditionalAttributes(List<String> additionalAttributes)
-    {
+    public void setAdditionalAttributes(List<String> additionalAttributes) {
         if (additionalAttributes != null) {
             this.additionalAttributes = new ArrayList<>(additionalAttributes);
         } else {
@@ -90,15 +89,13 @@ public void setAdditionalAttributes(List<String> additionalAttributes)
     }
 
     @Override
-    public boolean canTransformResource(String resource)
-    {
+    public boolean canTransformResource(String resource) {
         return JarFile.MANIFEST_NAME.equalsIgnoreCase(resource);
     }
 
     @Override
     public void processResource(String resource, InputStream is, List<Relocator> relocators, long time)
-            throws IOException
-    {
+            throws IOException {
         Manifest loadedManifest = new Manifest(is);
 
         // Relocate the just loaded manifest
@@ -141,33 +138,25 @@ public void processResource(String resource, InputStream is, List<Relocator> rel
 
             // loop through the sections of the just loaded manifest
             loadedManifest.getEntries().forEach((key, value) -> {
-                // Find the section that matches this entry or create a new one.
-                Attributes existingRootSectionAttributes = existingRootSections.get(key);
-
                 // create a new section if it does not already exist
-                if (existingRootSectionAttributes == null) {
-                    existingRootSectionAttributes = new Attributes();
-                    existingRootSections.put(key, existingRootSectionAttributes);
-                }
+                Attributes existingRootSectionAttributes = existingRootSections.computeIfAbsent(key, k -> new Attributes());
 
                 // Add all attributes from that section to the manifest
                 if (value != null) {
-                    value.forEach(existingRootSectionAttributes::put);
+                    existingRootSectionAttributes.putAll(value);
                 }
             });
         }
     }
 
     @Override
-    public boolean hasTransformedResource()
-    {
+    public boolean hasTransformedResource() {
         return true;
     }
 
     @Override
     public void modifyOutputStream(JarOutputStream jos)
-            throws IOException
-    {
+            throws IOException {
         // If we didn't find a manifest, then let's create one.
         if (rootManifest == null) {
             rootManifest = new Manifest();
@@ -193,17 +182,29 @@ public void modifyOutputStream(JarOutputStream jos)
         rootManifest.write(jos);
     }
 
-    private String relocate(String originalValue, List<Relocator> relocators)
-    {
-        String newValue = originalValue;
+    static String relocate(String originalName, List<Relocator> relocators) {
+        String existingPrefix = null;
+        String relocatedName = null;
         for (Relocator relocator : relocators) {
-            String value;
-            do {
-                value = newValue;
-                newValue = relocator.relocateClass(value);
+            for (; ; ) {
+                relocatedName = relocator.relocateClass(originalName);
+                if (originalName.equals(relocatedName)) {
+                    break; // for(;;
+                }
+                // see if this is just a regular "add a prefix" relocation
+                if (relocatedName.endsWith(originalName)) {
+                    String prefix = relocatedName.substring(0, relocatedName.length() - originalName.length());
+                    if (prefix.equals(existingPrefix)) {
+                        String errorMessage = format("Detected loop when relocating %s to %s, prefix %s already applied!%nRelocation pattern (<pattern>) may be unanchored (not starting with '^')!", originalName,
+                                relocatedName, prefix);
+                        throw new IllegalStateException(errorMessage);
+                    }
+                    existingPrefix = prefix;
+                }
+
+                originalName = relocatedName;
             }
-            while (!value.equals(newValue));
         }
-        return newValue;
+        return relocatedName;
     }
 }
diff --git a/src/test/java/org/basepom/maven/shade/CollectingManifestResourceTransformerTest.java b/src/test/java/org/basepom/maven/shade/CollectingManifestResourceTransformerTest.java
@@ -0,0 +1,62 @@
+/*
+ * Licensed 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.basepom.maven.shade;
+
+import java.util.Collections;
+
+import org.apache.maven.plugins.shade.relocation.Relocator;
+import org.apache.maven.plugins.shade.relocation.SimpleRelocator;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.Test;
+
+class CollectingManifestResourceTransformerTest {
+
+    @Test
+    void testPatternRelocation() {
+        Relocator relocator = new SimpleRelocator("^com\\.google\\.common", "prefab.shaded.com.google.common", null, null);
+
+        Assertions.assertEquals("prefab.shaded.com.google.common.SomeClass",
+                CollectingManifestResourceTransformer.relocate("com.google.common.SomeClass", Collections.singletonList(relocator)));
+    }
+
+    @Test
+    void testAnchoredRelocation() {
+        Relocator relocator = new SimpleRelocator("^com.google.common", "prefab.shaded.com.google.common", null, null);
+
+        Assertions.assertEquals("prefab.shaded.com.google.common.SomeClass",
+                CollectingManifestResourceTransformer.relocate("com.google.common.SomeClass", Collections.singletonList(relocator)));
+    }
+
+    @Test
+    void testFloatingNonMatchingRelocation() {
+        Relocator relocator = new SimpleRelocator("com.google.common", "prefab.shaded.cgm", null, null);
+
+        Assertions.assertEquals("prefab.shaded.cgm.SomeClass",
+                CollectingManifestResourceTransformer.relocate("com.google.common.SomeClass", Collections.singletonList(relocator)));
+    }
+
+    @Test
+    void testFloatingMatchingRelocation() {
+        Relocator relocator = new SimpleRelocator("com.google.common", "prefab.shaded.com.google.common", null, null);
+
+        IllegalStateException exception = Assertions.assertThrows(IllegalStateException.class,
+                () -> CollectingManifestResourceTransformer.relocate("com.google.common.SomeClass", Collections.singletonList(relocator)));
+
+        Assertions.assertEquals(
+                "Detected loop when relocating prefab.shaded.com.google.common.SomeClass to prefab.shaded.prefab.shaded.com.google.common.SomeClass, "
+                        + "prefix prefab.shaded. already applied!\nRelocation pattern (<pattern>) may be unanchored (not starting with '^')!",
+                exception.getMessage());
+    }
+}