Make the output of PropertiesFileTransformer reproducible (#1861)

* `PropertiesFileTransformer` - make merged properties reproducible

`PropertiesFileTransformer` leverages `java.util.Properties`, which relies on `java.util.Hashtable`. The serialized properties are not guaranteed to be reproducible.

This change changes the transformer to use a sorted map to generate reproducible output. The existing class `CleanProperties` is replaced with `ReproducibleProperties`.
Functionality around charset handling is retained, and extended with a functionality to generate unicode escapes (ASCII output).

* Cleanups

---------

Co-authored-by: Goooler <wangzongler@gmail.com>

Author: snazy

docs/changes/README.md

@@ -20,6 +20,7 @@
 - Update ASM and jdependency to support Java 26. ([#1799](https://github.com/GradleUp/shadow/pull/1799))
 - Bump min Gradle requirement to 9.0.0. ([#1801](https://github.com/GradleUp/shadow/pull/1801))
 - Deprecate `PreserveFirstFoundResourceTransformer.resources`. ([#1855](https://github.com/GradleUp/shadow/pull/1855))
+- Make the output of `PropertiesFileTransformer` reproducible. ([#1861](https://github.com/GradleUp/shadow/pull/1861))
 
 ### Fixed
 

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

@@ -5,7 +5,7 @@ import assertk.assertThat
 import assertk.assertions.contains
 import assertk.assertions.doesNotContain
 import com.github.jengelman.gradle.plugins.shadow.tasks.FindResourceInClasspath
-import com.github.jengelman.gradle.plugins.shadow.util.variantSeparatorsPathString
+import com.github.jengelman.gradle.plugins.shadow.testkit.variantSeparatorsPathString
 import kotlin.io.path.appendText
 import org.junit.jupiter.api.Test
 

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

@@ -4,9 +4,9 @@ import assertk.assertThat
 import assertk.assertions.contains
 import assertk.assertions.isEqualTo
 import com.github.jengelman.gradle.plugins.shadow.testkit.getContent
+import com.github.jengelman.gradle.plugins.shadow.testkit.invariantEolString
 import com.github.jengelman.gradle.plugins.shadow.transformers.PropertiesFileTransformer.MergeStrategy
 import com.github.jengelman.gradle.plugins.shadow.util.Issue
-import com.github.jengelman.gradle.plugins.shadow.util.invariantEolString
 import kotlin.io.path.appendText
 import org.gradle.testkit.runner.TaskOutcome.FAILED
 import org.junit.jupiter.api.Assertions.fail
@@ -51,13 +51,31 @@ class PropertiesFileTransformerTest : BaseTransformerTest() {
       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")
+        MergeStrategy.First ->
+          """
+          |key1=one
+          |key2=one
+          |key3=two
+          |
+          """.trimMargin()
+        MergeStrategy.Latest ->
+          """
+          |key1=one
+          |key2=two
+          |key3=two
+          |
+          """.trimMargin()
+        MergeStrategy.Append ->
+          """
+          |key1=one
+          |key2=one;two
+          |key3=two
+          |
+          """.trimMargin()
         else -> fail("Unexpected strategy: $strategy")
       }
       val content = outputShadowedJar.use { it.getContent("META-INF/test.properties") }
-      assertThat(content).contains(*expected)
+      assertThat(content.invariantEolString).isEqualTo(expected)
     }
   }
 
@@ -168,8 +186,6 @@ class PropertiesFileTransformerTest : BaseTransformerTest() {
     val content = outputShadowedJar.use { it.getContent("META-INF/test.properties") }
     assertThat(content.trimIndent()).isEqualTo(
       """
-        #
-
         foo=one,two
       """.trimIndent(),
     )

src/functionalTest/kotlin/com/github/jengelman/gradle/plugins/shadow/util/Paths.kt

@@ -1,14 +1,7 @@
 package com.github.jengelman.gradle.plugins.shadow.util
 
-import java.nio.file.FileSystems
 import java.nio.file.Path
 import kotlin.io.path.readText
 import kotlin.io.path.writeText
 
 fun Path.prependText(text: String) = writeText(text + readText())
-
-val String.invariantEolString: String get() = replace(System.lineSeparator(), "\n")
-
-val String.variantSeparatorsPathString: String get() = replace("/", fileSystem.separator)
-
-private val fileSystem = FileSystems.getDefault()

src/main/kotlin/com/github/jengelman/gradle/plugins/shadow/internal/CleanProperties.kt

@@ -1,31 +1,40 @@
 package com.github.jengelman.gradle.plugins.shadow.internal
 
-import java.io.BufferedWriter
-import java.io.IOException
+import java.io.OutputStream
+import java.io.StringWriter
 import java.io.Writer
-import java.util.Date
+import java.nio.charset.Charset
 import java.util.Properties
 
 /**
- * Introduced in order to remove prepended timestamp when creating output stream.
+ * Provides functionality for reproducible serialization.
  */
 internal class CleanProperties : Properties() {
-  @Throws(IOException::class)
   override fun store(writer: Writer, comments: String) {
-    super.store(StripCommentsWithTimestampBufferedWriter(writer), comments)
+    throw UnsupportedOperationException("use writeWithoutComments()")
   }
 
-  private class StripCommentsWithTimestampBufferedWriter(out: Writer) : BufferedWriter(out) {
-    private val lengthOfExpectedTimestamp = ("#" + Date().toString()).length
+  override fun store(out: OutputStream, comments: String?) {
+    throw UnsupportedOperationException("use writeWithoutComments()")
+  }
 
-    @Throws(IOException::class)
-    override fun write(str: String) {
-      if (str.couldBeCommentWithTimestamp) return
-      super.write(str)
-    }
+  fun writeWithoutComments(charset: Charset, os: OutputStream) {
+    val bufferedReader = StringWriter().apply {
+      super.store(this, null)
+    }.toString().reader().buffered()
 
-    private val String?.couldBeCommentWithTimestamp: Boolean get() {
-      return this != null && startsWith("#") && length == lengthOfExpectedTimestamp
-    }
+    os.bufferedWriter(charset).apply {
+      var line: String? = null
+      while (bufferedReader.readLine().also { line = it } != null && line != null) {
+        if (!line.startsWith("#")) {
+          write(line)
+          newLine()
+        }
+      }
+    }.flush()
   }
+
+  override val entries: MutableSet<MutableMap.MutableEntry<Any, Any>>
+    // Yields the entries for Properties.store0() in sorted order.
+    get() = super.entries.toSortedSet(compareBy { it.key.toString() })
 }

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

@@ -1,7 +1,6 @@
 package com.github.jengelman.gradle.plugins.shadow.transformers
 
 import com.github.jengelman.gradle.plugins.shadow.internal.CleanProperties
-import com.github.jengelman.gradle.plugins.shadow.internal.inputStream
 import com.github.jengelman.gradle.plugins.shadow.internal.mapProperty
 import com.github.jengelman.gradle.plugins.shadow.internal.property
 import com.github.jengelman.gradle.plugins.shadow.internal.setProperty
@@ -123,6 +122,10 @@ public open class PropertiesFileTransformer @Inject constructor(
   @get:Input
   public open val mergeSeparator: Property<String> = objectFactory.property(",")
 
+  /**
+   * The character set to use when reading and writing property files.
+   * Defaults to `ISO-8859-1`.
+   */
   @get:Input
   public open val charsetName: Property<String> = objectFactory.property(Charsets.ISO_8859_1.name())
 
@@ -146,49 +149,31 @@ public open class PropertiesFileTransformer @Inject constructor(
   }
 
   override fun transform(context: TransformerContext) {
-    val props = propertiesEntries[context.path]
-    val incoming = loadAndTransformKeys(context.inputStream)
-    if (props == null) {
-      propertiesEntries[context.path] = incoming
-    } else {
-      for ((key, value) in incoming) {
-        if (props.containsKey(key)) {
-          when (MergeStrategy.from(mergeStrategyFor(context.path))) {
-            MergeStrategy.Latest -> {
-              props[key] = value
-            }
-            MergeStrategy.Append -> {
-              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 }
-            }
+    val props = propertiesEntries.computeIfAbsent(context.path) { CleanProperties() }
+    loadAndTransformKeys(context.inputStream) { key, value ->
+      if (props.containsKey(key)) {
+        when (MergeStrategy.from(mergeStrategyFor(context.path))) {
+          MergeStrategy.Latest -> {
+            props[key] = value
+          }
+          MergeStrategy.Append -> {
+            props[key] = props[key] as String + mergeSeparatorFor(context.path) + value
+          }
+          MergeStrategy.First -> Unit
+          MergeStrategy.Fail -> {
+            val conflictsForPath = conflicts.computeIfAbsent(context.path) { mutableMapOf() }
+            conflictsForPath.compute(key) { _, count -> (count ?: 1) + 1 }
           }
-        } else {
-          props[key] = value
         }
+      } else {
+        props[key] = value
       }
     }
   }
 
-  private fun loadAndTransformKeys(inputStream: InputStream): CleanProperties {
-    val props = CleanProperties()
-    // InputStream closed by caller, so we don't do it here.
-    props.load(inputStream.bufferedReader(charset))
-    return transformKeys(props)
-  }
-
-  private fun transformKeys(properties: Properties): CleanProperties {
-    if (keyTransformer == IDENTITY) {
-      return properties as CleanProperties
-    }
-    val result = CleanProperties()
-    properties.forEach { (key, value) ->
-      result[keyTransformer(key as String)] = value
-    }
-    return result
+  private fun loadAndTransformKeys(inputStream: InputStream, action: (key: String, value: String) -> Unit) {
+    val props = Properties().apply { load(inputStream.bufferedReader(charset)) }
+    props.forEach { action(keyTransformer(it.key as String), it.value as String) }
   }
 
   private fun mergeStrategyFor(path: String): String {
@@ -233,14 +218,9 @@ public open class PropertiesFileTransformer @Inject constructor(
       error(message)
     }
 
-    // Cannot close the writer as the OutputStream needs to remain open.
-    val zipWriter = os.writer(charset)
     propertiesEntries.forEach { (path, props) ->
       os.putNextEntry(zipEntry(path, preserveFileTimestamps))
-      props.inputStream(charset).bufferedReader(charset).use {
-        it.copyTo(zipWriter)
-      }
-      zipWriter.flush()
+      props.writeWithoutComments(charset, os)
       os.closeEntry()
     }
   }

src/test/kotlin/com/github/jengelman/gradle/plugins/shadow/internal/CleanPropertiesTest.kt

@@ -0,0 +1,89 @@
+package com.github.jengelman.gradle.plugins.shadow.internal
+
+import assertk.assertThat
+import assertk.assertions.isEqualTo
+import com.github.jengelman.gradle.plugins.shadow.testkit.invariantEolString
+import java.io.ByteArrayOutputStream
+import java.nio.charset.Charset
+import java.nio.charset.StandardCharsets
+import org.junit.jupiter.params.ParameterizedTest
+import org.junit.jupiter.params.provider.MethodSource
+
+class CleanPropertiesTest {
+  @ParameterizedTest
+  @MethodSource("generalCharsetsProvider")
+  fun emptyProperties(charset: Charset) {
+    val output = CleanProperties().writeToString(charset)
+
+    assertThat(output).isEqualTo("")
+  }
+
+  @ParameterizedTest
+  @MethodSource("generalCharsetsProvider")
+  fun asciiProps(charset: Charset) {
+    val output = CleanProperties().also { props ->
+      props["key"] = "value"
+      props["key2"] = "value2"
+      props["a"] = "b"
+      props["d"] = "e"
+      props["0"] = "1"
+      props["b"] = "c"
+      props["c"] = "d"
+      props["e"] = "f"
+    }.writeToString(charset)
+
+    assertThat(output).isEqualTo(
+      """
+      |0=1
+      |a=b
+      |b=c
+      |c=d
+      |d=e
+      |e=f
+      |key=value
+      |key2=value2
+      |
+      """.trimMargin(),
+    )
+  }
+
+  @ParameterizedTest
+  @MethodSource("utfCharsetsProvider")
+  fun utfProps(charset: Charset) {
+    val output = CleanProperties().also { props ->
+      props["äöüß"] = "aouss"
+      props["áèô"] = "aeo"
+      props["€²³"] = "x"
+      props["传傳磨宿说説"] = "b"
+    }.writeToString(charset)
+
+    assertThat(output).isEqualTo(
+      """
+      |áèô=aeo
+      |äöüß=aouss
+      |€²³=x
+      |传傳磨宿说説=b
+      |
+      """.trimMargin(),
+    )
+  }
+
+  private companion object Companion {
+    @JvmStatic
+    fun generalCharsetsProvider() = listOf(
+      StandardCharsets.ISO_8859_1,
+      StandardCharsets.US_ASCII,
+    ) + utfCharsetsProvider()
+
+    @JvmStatic
+    fun utfCharsetsProvider() = listOf(
+      StandardCharsets.UTF_8,
+      StandardCharsets.UTF_16,
+    )
+
+    fun CleanProperties.writeToString(charset: Charset): String {
+      return ByteArrayOutputStream().also { writeWithoutComments(charset, it) }
+        .toString(charset.name()).invariantEolString
+    }
+  }
+}

src/testKit/kotlin/com/github/jengelman/gradle/plugins/shadow/testkit/Strings.kt

@@ -0,0 +1,9 @@
+package com.github.jengelman.gradle.plugins.shadow.testkit
+
+import java.nio.file.FileSystems
+
+val String.invariantEolString: String get() = replace(System.lineSeparator(), "\n")
+
+val String.variantSeparatorsPathString: String get() = replace("/", fileSystem.separator)
+
+private val fileSystem = FileSystems.getDefault()