Fix issue with isolation of Properties objects

subprojects/model-core/src/main/java/org/gradle/internal/snapshot/impl/AbstractIsolatedMap.java

@@ -0,0 +1,55 @@
+/*
+ * Copyright 2019 the original author or authors.
+ *
+ * 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.gradle.internal.snapshot.impl;
+
+import com.google.common.collect.ImmutableList;
+import org.gradle.internal.Factory;
+import org.gradle.internal.isolation.Isolatable;
+import org.gradle.internal.snapshot.ValueSnapshot;
+
+import javax.annotation.Nullable;
+import java.util.Map;
+
+abstract public class AbstractIsolatedMap<T extends Map<Object, Object>> extends AbstractMapSnapshot<Isolatable<?>> implements Isolatable<T>, Factory<T> {
+    public AbstractIsolatedMap(ImmutableList<MapEntrySnapshot<Isolatable<?>>> entries) {
+        super(entries);
+    }
+
+    @Override
+    public ValueSnapshot asSnapshot() {
+        ImmutableList.Builder<MapEntrySnapshot<ValueSnapshot>> builder = ImmutableList.builderWithExpectedSize(entries.size());
+        for (MapEntrySnapshot<Isolatable<?>> entry : entries) {
+            builder.add(new MapEntrySnapshot<ValueSnapshot>(entry.getKey().asSnapshot(), entry.getValue().asSnapshot()));
+        }
+        return new MapValueSnapshot(builder.build());
+    }
+
+    @Override
+    public T isolate() {
+        T map = create();
+        for (MapEntrySnapshot<Isolatable<?>> entry : getEntries()) {
+            map.put(entry.getKey().isolate(), entry.getValue().isolate());
+        }
+        return map;
+    }
+
+    @Nullable
+    @Override
+    public <S> S coerce(Class<S> type) {
+        return null;
+    }
+}

subprojects/model-core/src/main/java/org/gradle/internal/snapshot/impl/DefaultValueSnapshotter.java

@@ -36,6 +36,7 @@
 import java.lang.reflect.Array;
 import java.util.List;
 import java.util.Map;
+import java.util.Properties;
 import java.util.Set;
 
 public class DefaultValueSnapshotter implements ValueSnapshotter, IsolatableFactory {
@@ -123,7 +124,11 @@ private <T> T processValue(@Nullable Object value, ValueVisitor<T> visitor) {
             for (Map.Entry<?, ?> entry : map.entrySet()) {
                 builder.add(new MapEntrySnapshot<T>(processValue(entry.getKey(), visitor), processValue(entry.getValue(), visitor)));
             }
-            return visitor.map(builder.build());
+            if (value instanceof Properties) {
+                return visitor.properties(builder.build());
+            } else {
+                return visitor.map(builder.build());
+            }
         }
         if (value.getClass().isArray()) {
             int length = Array.getLength(value);
@@ -211,6 +216,8 @@ private interface ValueVisitor<T> {
 
         T map(ImmutableList<MapEntrySnapshot<T>> elements);
 
+        T properties(ImmutableList<MapEntrySnapshot<T>> elements);
+
         T serialized(Object value, byte[] serializedValue);
     }
 
@@ -320,6 +327,11 @@ public ValueSnapshot set(ImmutableSet<ValueSnapshot> elements) {
         public ValueSnapshot map(ImmutableList<MapEntrySnapshot<ValueSnapshot>> elements) {
             return new MapValueSnapshot(elements);
         }
+
+        @Override
+        public ValueSnapshot properties(ImmutableList<MapEntrySnapshot<ValueSnapshot>> elements) {
+            return new MapValueSnapshot(elements);
+        }
     }
 
     private static class IsolatableVisitor implements ValueVisitor<Isolatable<?>> {
@@ -430,5 +442,10 @@ public Isolatable<?> set(ImmutableSet<Isolatable<?>> elements) {
         public Isolatable<?> map(ImmutableList<MapEntrySnapshot<Isolatable<?>>> elements) {
             return new IsolatedMap(elements);
         }
+
+        @Override
+        public Isolatable<?> properties(ImmutableList<MapEntrySnapshot<Isolatable<?>>> elements) {
+            return new IsolatedProperties(elements);
+        }
     }
 }

subprojects/model-core/src/main/java/org/gradle/internal/snapshot/impl/IsolatedMap.java

@@ -18,38 +18,19 @@
 
 import com.google.common.collect.ImmutableList;
 import org.gradle.internal.isolation.Isolatable;
-import org.gradle.internal.snapshot.ValueSnapshot;
 
 import javax.annotation.Nullable;
 import java.util.LinkedHashMap;
 import java.util.Map;
 
-public class IsolatedMap extends AbstractMapSnapshot<Isolatable<?>> implements Isolatable<Map<Object, Object>> {
+public class IsolatedMap extends AbstractIsolatedMap<Map<Object, Object>> {
     public IsolatedMap(ImmutableList<MapEntrySnapshot<Isolatable<?>>> entries) {
         super(entries);
     }
 
-    @Override
-    public ValueSnapshot asSnapshot() {
-        ImmutableList.Builder<MapEntrySnapshot<ValueSnapshot>> builder = ImmutableList.builderWithExpectedSize(entries.size());
-        for (MapEntrySnapshot<Isolatable<?>> entry : entries) {
-            builder.add(new MapEntrySnapshot<ValueSnapshot>(entry.getKey().asSnapshot(), entry.getValue().asSnapshot()));
-        }
-        return new MapValueSnapshot(builder.build());
-    }
-
-    @Override
-    public Map<Object, Object> isolate() {
-        Map<Object, Object> map = new LinkedHashMap<>(getEntries().size());
-        for (MapEntrySnapshot<Isolatable<?>> entry : getEntries()) {
-            map.put(entry.getKey().isolate(), entry.getValue().isolate());
-        }
-        return map;
-    }
-
     @Nullable
     @Override
-    public <S> S coerce(Class<S> type) {
-        return null;
+    public Map<Object, Object> create() {
+        return new LinkedHashMap<>(getEntries().size());
     }
 }

subprojects/model-core/src/main/java/org/gradle/internal/snapshot/impl/IsolatedProperties.java

@@ -0,0 +1,35 @@
+/*
+ * Copyright 2019 the original author or authors.
+ *
+ * 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.gradle.internal.snapshot.impl;
+
+import com.google.common.collect.ImmutableList;
+import org.gradle.internal.isolation.Isolatable;
+
+import javax.annotation.Nullable;
+import java.util.Properties;
+
+public class IsolatedProperties extends AbstractIsolatedMap<Properties> {
+    public IsolatedProperties(ImmutableList<MapEntrySnapshot<Isolatable<?>>> entries) {
+        super(entries);
+    }
+
+    @Nullable
+    @Override
+    public Properties create() {
+        return new Properties();
+    }
+}

subprojects/model-core/src/test/groovy/org/gradle/internal/snapshot/impl/DefaultValueSnapshotterTest.groovy

@@ -424,6 +424,56 @@ class DefaultValueSnapshotterTest extends Specification {
         snapshotter.snapshot([a: "1", b: "2"]) != snapshotter.snapshot(isolated2)
     }
 
+    Properties properties(Map<String, String> entries) {
+        def properties = new Properties()
+        entries.each { key, value -> properties.setProperty(key, value) }
+        return properties
+    }
+
+    def "creates snapshot for properties"() {
+        expect:
+        def snapshot1 = snapshotter.snapshot(properties([:]))
+        snapshot1 instanceof MapValueSnapshot
+        snapshot1 == snapshotter.snapshot(properties([:]))
+        snapshot1 != snapshotter.snapshot("abc")
+        snapshot1 != snapshotter.snapshot(properties(["a": "123"]))
+
+        def snapshot2 = snapshotter.snapshot(properties(["a": "123"]))
+        snapshot2 instanceof MapValueSnapshot
+        snapshot2 == snapshotter.snapshot(properties([a: "123"]))
+        snapshot2 != snapshotter.snapshot(["123"])
+        snapshot2 != snapshotter.snapshot(properties([:]))
+        snapshot2 != snapshotter.snapshot(properties([a: "123", b: "abc"]))
+        snapshot2 != snapshot1
+    }
+
+    def "creates isolated properties"() {
+        expect:
+        def original1 = properties([:])
+        def isolated1 = snapshotter.isolate(original1)
+        isolated1 instanceof IsolatedProperties
+        def copy1 = isolated1.isolate()
+        copy1 == properties([:])
+        !copy1.is(original1)
+
+        def original2 = properties([a: "123"])
+        def isolated2 = snapshotter.isolate(original2)
+        isolated2 instanceof IsolatedProperties
+        def copy2 = isolated2.isolate()
+        copy2 == properties([a: "123"])
+        !copy2.is(isolated2)
+    }
+
+    def "creates snapshot for isolated properties"() {
+        expect:
+        def isolated1 = snapshotter.isolate(properties([:]))
+        snapshotter.snapshot(properties([:])) == snapshotter.snapshot(isolated1)
+
+        def isolated2 = snapshotter.isolate(properties([a: "123"]))
+        snapshotter.snapshot(properties([a: "123"])) == snapshotter.snapshot(isolated2)
+        snapshotter.snapshot(properties([a: "1", b: "2"])) != snapshotter.snapshot(isolated2)
+    }
+
     enum Type2 {
         TWO, THREE
     }
@@ -1036,6 +1086,27 @@ class DefaultValueSnapshotterTest extends Specification {
         snapshotter.snapshot(map3, snapshot4) == snapshotter.snapshot(map3)
     }
 
+    def "creates snapshot for properties from candidate"() {
+        expect:
+        def snapshot1 = snapshotter.snapshot(properties([:]))
+        snapshotter.snapshot(properties([:]), snapshot1).is(snapshot1)
+
+        snapshotter.snapshot(properties(["12": "123"]), snapshot1) != snapshot1
+
+        snapshotter.snapshot("other", snapshot1) != snapshot1
+        snapshotter.snapshot("other", snapshot1) == snapshotter.snapshot("other")
+        snapshotter.snapshot(new Bean(), snapshot1) != snapshot1
+        snapshotter.snapshot(new Bean(), snapshot1) == snapshotter.snapshot(new Bean())
+
+        def snapshot2 = snapshotter.snapshot(properties(["12": "123"]))
+        snapshotter.snapshot(properties(["12": "123"]), snapshot2).is(snapshot2)
+
+        snapshotter.snapshot(properties(["12": "456"]), snapshot2) != snapshot2
+        snapshotter.snapshot(properties([:]), snapshot2) != snapshot2
+        snapshotter.snapshot(properties(["123": "123"]), snapshot2) != snapshot2
+        snapshotter.snapshot(properties(["12": "123", "10": "123"]), snapshot2) != snapshot2
+    }
+
     def "creates snapshot for provider type from candidate"() {
         def value = Providers.of("123")
         def value2 = Providers.of("123")

subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorLegacyApiIntegrationTest.groovy

@@ -21,6 +21,7 @@ import org.gradle.integtests.fixtures.BuildOperationsFixture
 import org.gradle.internal.jvm.Jvm
 import org.gradle.util.TestPrecondition
 import org.gradle.workers.fixtures.OptionsVerifier
+import spock.lang.Issue
 import spock.lang.Unroll
 
 import static org.gradle.api.internal.file.TestFiles.systemSpecificAbsolutePath
@@ -216,6 +217,82 @@ class WorkerExecutorLegacyApiIntegrationTest extends AbstractIntegrationSpec {
         isolationMode << ISOLATION_MODES
     }
 
+    @Issue("https://github.com/gradle/gradle/issues/10323")
+    def "can use a Properties object as a parameter"() {
+        buildFile << """
+            import org.gradle.api.DefaultTask
+            import org.gradle.api.tasks.TaskAction
+            import org.gradle.workers.IsolationMode
+            import org.gradle.workers.WorkerExecutor
+
+            import javax.inject.Inject
+
+            task myTask(type: MyTask) {
+                description 'My Task'
+                outputFile = file("\${buildDir}/workOutput")
+            }
+
+            class MyTask extends DefaultTask {
+                final WorkerExecutor workerExecutor
+
+                @OutputFile
+                File outputFile
+
+                @Inject
+                MyTask(WorkerExecutor workerExecutor) {
+                    this.workerExecutor = workerExecutor
+                }
+
+                @TaskAction
+                def run() {
+                    Properties myProps = new Properties()
+                    myProps.setProperty('key1', 'value1')
+                    myProps.setProperty('key2', 'value2')
+                    myProps.setProperty('key3', 'value3')
+
+                    workerExecutor.submit(MyRunner.class) { config ->
+                        config.isolationMode = IsolationMode.NONE
+
+                        config.params(myProps, outputFile)
+                    }
+
+                    workerExecutor.await()
+                }
+
+                private static class MyRunner implements Runnable {
+                    Properties myProps
+                    File outputFile
+
+                    @Inject
+                    MyRunner(Properties myProps, File outputFile) {
+                        this.myProps = myProps
+                        this.outputFile = outputFile
+                    }
+
+                    @Override
+                    void run() {
+                        Properties myProps = this.myProps;
+                        def writer = outputFile.newWriter()
+                        try {
+                            myProps.store(writer, null)
+                        } finally {
+                            writer.close()
+                        }
+                    }
+                }
+            }
+        """
+
+        expect:
+        succeeds(":myTask")
+
+        and:
+        file("build/workOutput").text.readLines().containsAll([
+                "key1=value1",
+                "key2=value2",
+                "key3=value3"
+        ])
+    }
 
     String getLegacyWorkerTypeAndTask() {
         return """

subprojects/workers/src/integTest/groovy/org/gradle/workers/internal/WorkerExecutorParametersIntegrationTest.groovy

@@ -242,6 +242,30 @@ class WorkerExecutorParametersIntegrationTest extends AbstractIntegrationSpec {
         isolationMode << ISOLATION_MODES
     }
 
+    def "can provide a Properties object with isolation mode #isolationMode"() {
+        buildFile << """
+            ${parameterWorkAction('Properties', 'println parameters.testParam.collect { it.key + ":" + it.value }.join(",")', true) }
+
+            task runWork(type: ParameterTask) {
+                isolationMode = ${isolationMode}
+                parameters {
+                    testParam = new Properties()
+                    testParam.setProperty("foo", "bar")
+                    testParam.setProperty("bar", "baz")
+                }
+            } 
+        """
+
+        when:
+        succeeds("runWork")
+
+        then:
+        outputContains("bar:baz,foo:bar")
+
+        where:
+        isolationMode << ISOLATION_MODES
+    }
+
     def "can provide file property parameters with isolation mode #isolationMode"() {
         buildFile << """
             ${parameterWorkAction('RegularFileProperty', 'println parameters.testParam.get().getAsFile().name')}