Add new merge strategy Fail to PropertiesFileTransformer (#1856)

Co-authored-by: Zongle Wang <[email protected]>

Author: snazy

api/shadow.api

@@ -434,6 +434,7 @@ public class com/github/jengelman/gradle/plugins/shadow/transformers/PropertiesF
 public final class com/github/jengelman/gradle/plugins/shadow/transformers/PropertiesFileTransformer$MergeStrategy : java/lang/Enum {
 	public static final field Append Lcom/github/jengelman/gradle/plugins/shadow/transformers/PropertiesFileTransformer$MergeStrategy;
 	public static final field Companion Lcom/github/jengelman/gradle/plugins/shadow/transformers/PropertiesFileTransformer$MergeStrategy$Companion;
+	public static final field Fail Lcom/github/jengelman/gradle/plugins/shadow/transformers/PropertiesFileTransformer$MergeStrategy;
 	public static final field First Lcom/github/jengelman/gradle/plugins/shadow/transformers/PropertiesFileTransformer$MergeStrategy;
 	public static final field Latest Lcom/github/jengelman/gradle/plugins/shadow/transformers/PropertiesFileTransformer$MergeStrategy;
 	public static final fun from (Ljava/lang/String;)Lcom/github/jengelman/gradle/plugins/shadow/transformers/PropertiesFileTransformer$MergeStrategy;

docs/changes/README.md

@@ -11,6 +11,7 @@
 - Expose `patternSet` of `ApacheNoticeResourceTransformer` as `public`. ([#1850](https://github.com/GradleUp/shadow/pull/1850))
 - Expose `patternSet` of `PreserveFirstFoundResourceTransformer` as `public`. ([#1855](https://github.com/GradleUp/shadow/pull/1855))
 - Support overriding output path of `ApacheNoticeResourceTransformer`. ([#1851](https://github.com/GradleUp/shadow/pull/1851))
+- Add new merge strategy `Fail` to `PropertiesFileTransformer`. ([#1856](https://github.com/GradleUp/shadow/pull/1856))
 
 ### Changed
 

src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/transformers/PropertiesFileTransformerTest.kt

@@ -7,6 +7,8 @@ import com.github.jengelman.gradle.plugins.shadow.testkit.getContent
 import com.github.jengelman.gradle.plugins.shadow.transformers.PropertiesFileTransformer.MergeStrategy
 import com.github.jengelman.gradle.plugins.shadow.util.Issue
 import kotlin.io.path.appendText
+import org.gradle.testkit.runner.TaskOutcome.FAILED
+import org.junit.jupiter.api.Assertions.fail
 import org.junit.jupiter.api.Test
 import org.junit.jupiter.params.ParameterizedTest
 import org.junit.jupiter.params.provider.EnumSource
@@ -33,15 +35,29 @@ class PropertiesFileTransformerTest : BaseTransformerTest() {
       ),
     )
 
-    runWithSuccess(shadowJarPath)
+    if (strategy == MergeStrategy.Fail) {
+      val result = runWithFailure(shadowJarPath)
 
-    val expected = when (strategy) {
-      MergeStrategy.First -> arrayOf("key1=one", "key2=one", "key3=two")
-      MergeStrategy.Latest -> arrayOf("key1=one", "key2=two", "key3=two")
-      MergeStrategy.Append -> arrayOf("key1=one", "key2=one;two", "key3=two")
+      assertThat(result).taskOutcomeEquals(shadowJarPath, FAILED)
+      assertThat(result.output.replace(lineSeparator, "\n")).contains(
+        """
+          Caused by: java.lang.IllegalStateException: The following properties files have conflicting property values and cannot be merged:
+           * META-INF/test.properties
+             * Property key2 is duplicated 2 times with different values
+        """.trimIndent(),
+      )
+    } else {
+      runWithSuccess(shadowJarPath)
+
+      val expected = when (strategy) {
+        MergeStrategy.First -> arrayOf("key1=one", "key2=one", "key3=two")
+        MergeStrategy.Latest -> arrayOf("key1=one", "key2=two", "key3=two")
+        MergeStrategy.Append -> arrayOf("key1=one", "key2=one;two", "key3=two")
+        else -> fail("Unexpected strategy: $strategy")
+      }
+      val content = outputShadowedJar.use { it.getContent("META-INF/test.properties") }
+      assertThat(content).contains(*expected)
     }
-    val content = outputShadowedJar.use { it.getContent("META-INF/test.properties") }
-    assertThat(content).contains(*expected)
   }
 
   @Test

src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/transformers/PropertiesFileTransformer.kt

@@ -62,6 +62,8 @@ import org.gradle.api.tasks.Internal
  * - key2 = value2;balue2
  * - key3 = value3
  *
+ * With `mergeStrategy = MergeStrategy.Fail` the transformation will fail if there are conflicting values.
+ *
  * There are three additional properties that can be set: [paths], [mappings],
  * and [keyTransformer].
  * The first contains a list of strings or regexes that will be used to determine if
@@ -103,6 +105,9 @@ public open class PropertiesFileTransformer @Inject constructor(
 ) : ResourceTransformer {
   private inline val charset get() = Charset.forName(charsetName.get())
 
+  @get:Internal
+  internal val conflicts: MutableMap<String, MutableMap<String, Int>> = mutableMapOf()
+
   @get:Internal
   internal val propertiesEntries = mutableMapOf<String, CleanProperties>()
 
@@ -156,6 +161,10 @@ public open class PropertiesFileTransformer @Inject constructor(
               props[key] = props.getProperty(key as String) + mergeSeparatorFor(context.path) + value
             }
             MergeStrategy.First -> Unit
+            MergeStrategy.Fail -> {
+              val conflictsForPath = conflicts.computeIfAbsent(context.path) { mutableMapOf() }
+              conflictsForPath.compute(key as String) { _, count -> (count ?: 1) + 1 }
+            }
           }
         } else {
           props[key] = value
@@ -215,6 +224,15 @@ public open class PropertiesFileTransformer @Inject constructor(
   override fun hasTransformedResource(): Boolean = propertiesEntries.isNotEmpty()
 
   override fun modifyOutputStream(os: ZipOutputStream, preserveFileTimestamps: Boolean) {
+    if (conflicts.isNotEmpty()) {
+      val message = "The following properties files have conflicting property values and cannot be merged:" +
+        conflicts.map { (path, props) ->
+          path + props.map { "Property ${it.key} is duplicated ${it.value} times with different values" }
+            .joinToString(separator = "\n   * ", prefix = "\n   * ")
+        }.joinToString(separator = "\n * ", prefix = "\n * ")
+      error(message)
+    }
+
     // Cannot close the writer as the OutputStream needs to remain open.
     val zipWriter = os.writer(charset)
     propertiesEntries.forEach { (path, props) ->
@@ -231,6 +249,7 @@ public open class PropertiesFileTransformer @Inject constructor(
     First,
     Latest,
     Append,
+    Fail,
     ;
 
     public companion object {

src/test/kotlin/com/github/jengelman/gradle/plugins/shadow/transformers/PropertiesFileTransformerTest.kt

@@ -64,6 +64,7 @@ class PropertiesFileTransformerTest : BaseTransformerTest<PropertiesFileTransfor
     input1: Map<String, String>,
     input2: Map<String, String>,
     expectedOutput: Map<String, String>,
+    expectedConflicts: Map<String, Map<String, Int>>,
   ) {
     transformer.mergeStrategy.set(MergeStrategy.from(mergeStrategy))
     transformer.mergeSeparator.set(mergeSeparator)
@@ -74,6 +75,7 @@ class PropertiesFileTransformerTest : BaseTransformerTest<PropertiesFileTransfor
     }
 
     assertThat(transformer.propertiesEntries[path].orEmpty()).isEqualTo(expectedOutput)
+    assertThat(transformer.conflicts).isEqualTo(expectedConflicts)
   }
 
   @ParameterizedTest
@@ -263,6 +265,7 @@ class PropertiesFileTransformerTest : BaseTransformerTest<PropertiesFileTransfor
         mapOf("foo" to "foo"),
         mapOf("foo" to "bar"),
         mapOf("foo" to "foo"),
+        mapOf<String, Map<String, Int>>(),
       ),
       Arguments.of(
         "f.properties",
@@ -271,6 +274,7 @@ class PropertiesFileTransformerTest : BaseTransformerTest<PropertiesFileTransfor
         mapOf("foo" to "foo"),
         mapOf("foo" to "bar"),
         mapOf("foo" to "bar"),
+        mapOf<String, Map<String, Int>>(),
       ),
       Arguments.of(
         "f.properties",
@@ -279,6 +283,7 @@ class PropertiesFileTransformerTest : BaseTransformerTest<PropertiesFileTransfor
         mapOf("foo" to "foo"),
         mapOf("foo" to "bar"),
         mapOf("foo" to "foo,bar"),
+        mapOf<String, Map<String, Int>>(),
       ),
       Arguments.of(
         "f.properties",
@@ -287,6 +292,16 @@ class PropertiesFileTransformerTest : BaseTransformerTest<PropertiesFileTransfor
         mapOf("foo" to "foo"),
         mapOf("foo" to "bar"),
         mapOf("foo" to "foo;bar"),
+        mapOf<String, Map<String, Int>>(),
+      ),
+      Arguments.of(
+        "f.properties",
+        "fail",
+        ";",
+        mapOf("foo" to "foo"),
+        mapOf("foo" to "bar"),
+        mapOf("foo" to "foo"),
+        mapOf("f.properties" to mapOf("foo" to 2)),
       ),
     )