Hi again,
Just after having sent the previous mail I found Manifest::clone
explicitly says it creates a shallow copy which is true only to a
certain extent. It deeply copies main attributes as well as individual
sections map already now and only shallowly copies individual sections
attributes maps.
I don't know about the background of it or why clone should only do a
shallow copy but if clone should really create a shallow copy, it
should probably create an even more shallow copy. In any case I figure
some potential for clarification at least.
Probably mostly because I already began a patch in the previous
message, I continued and attached another patch for a deep copy
approach. There might still be some reason not to deeply copy manifests
which I don't intend to forestall.
Philipp
On Sat, 2019-01-19 at 19:32 +0100, Philipp Kunz wrote:
> Hi,
>
> Creating a new manifest as a copy from an existing one using the copy
> constructor assigns the new manifest individual sections entries map
> the same values which are references to attributes as the original
> rather than copying them as well deeply resulting in two manifests
> pointing to the same attributes instances modifications to which
> always affect both manifests. See test and proposed fix in the
> attached patch.
>
> Regards,
> Philipp
diff -r 3f224e4a891e src/java.base/share/classes/java/util/jar/Manifest.java
--- a/src/java.base/share/classes/java/util/jar/Manifest.java Fri Jan 18 17:34:43 2019 -0800
+++ b/src/java.base/share/classes/java/util/jar/Manifest.java Sat Jan 19 20:31:40 2019 +0100
@@ -109,7 +109,9 @@
*/
public Manifest(Manifest man) {
attr.putAll(man.getMainAttributes());
- entries.putAll(man.getEntries());
+ for (Map.Entry<String, Attributes> e : man.getEntries().entrySet()) {
+ entries.put(e.getKey(), new Attributes(e.getValue()));
+ }
jv = man.jv;
}
@@ -401,14 +403,12 @@
}
/**
- * Returns a shallow copy of this Manifest. The shallow copy is
- * implemented as follows:
- * <pre>
- * public Object clone() { return new Manifest(this); }
- * </pre>
- * @return a shallow copy of this Manifest
+ * Returns a deep copy of this Manifest.
+ *
+ * @return a deep copy of this Manifest
+ * @see #Manifest(Manifest)
*/
- public Object clone() {
+ public Manifest clone() {
return new Manifest(this);
}
diff -r 3f224e4a891e test/jdk/java/util/jar/Manifest/ManifestCopyConstructorAndClone.java
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/test/jdk/java/util/jar/Manifest/ManifestCopyConstructorAndClone.java Sat Jan 19 20:31:40 2019 +0100
@@ -0,0 +1,164 @@
+/*
+ * Copyright (c) 2019, Oracle and/or its affiliates. All rights reserved.
+ * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
+ *
+ * This code is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License version 2 only, as
+ * published by the Free Software Foundation.
+ *
+ * This code is distributed in the hope that it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License
+ * version 2 for more details (a copy is included in the LICENSE file that
+ * accompanied this code).
+ *
+ * You should have received a copy of the GNU General Public License version
+ * 2 along with this work; if not, write to the Free Software Foundation,
+ * Inc., 51 Franklin St, Fifth Floor, Boston, MA 02110-1301 USA.
+ *
+ * Please contact Oracle, 500 Oracle Parkway, Redwood Shores, CA 94065 USA
+ * or visit www.oracle.com if you need additional information or have any
+ * questions.
+ */
+
+import java.util.jar.Attributes;
+import java.util.jar.Manifest;
+
+import org.testng.annotations.Test;
+import static org.testng.Assert.*;
+
+/**
+ * @test
+ * @run testng ManifestCopyConstructorAndClone
+ * @summary Checks Manifest copy constructor and clone deeply copy main
+ * attributes, individual section entries, and individual section attributes.
+ */
+public class ManifestCopyConstructorAndClone {
+
+ @Test
+ public void testDeepCopyMainAttributes() throws Exception {
+ Manifest m1 = new Manifest();
+ Manifest m2 = new Manifest(m1);
+
+ // modify main attributes of manifest m1 after m2 was copied from m1
+ m1.getMainAttributes().putValue("x", "x");
+ assertEquals(m1.getMainAttributes().getValue("x"), "x");
+
+ // expect earlier copy unaffected
+ assertNull(m2.getMainAttributes().getValue("x"));
+
+ // modify attributes of copied manifest m2
+ m2.getMainAttributes().putValue("y", "y");
+ assertEquals(m2.getMainAttributes().getValue("y"), "y");
+
+ // expect original manifest m1 unaffected
+ assertNull(m1.getMainAttributes().getValue("y"));
+ }
+
+ @Test
+ public void testDeepCopyEntries() throws Exception {
+ Manifest m1 = new Manifest();
+ Attributes a1 = new Attributes();
+ m1.getEntries().put("x", a1);
+ Manifest m2 = new Manifest(m1);
+
+ // modify entries of manifest m1 after m2 was copied from m1
+ assertNotNull(m1.getEntries().remove("x"));
+
+ // expect earlier copy unaffected
+ assertNotNull(m2.getAttributes("x"));
+
+ // modify attributes of copied manifest m2
+ m2.getEntries().put("y", new Attributes());
+ assertNotNull(m2.getAttributes("y"));
+
+ // expect original manifest m1 unaffected
+ assertNull(m1.getAttributes("y"));
+ }
+
+ @Test
+ public void testDeepCopyIndividualSectionAttributes() throws Exception {
+ Manifest m1 = new Manifest();
+ Attributes a1 = new Attributes();
+ m1.getEntries().put("x", a1);
+ Manifest m2 = new Manifest(m1);
+
+ // modify attributes a1 of manifest m1 after m2 was copied from m1
+ a1.putValue("x", "x");
+ assertEquals(m1.getAttributes("x").getValue("x"), "x");
+
+ // expect earlier copy unaffected
+ assertNull(m2.getAttributes("x").getValue("x"));
+
+ // modify attributes of copied manifest m2
+ m2.getAttributes("x").putValue("y", "y");
+ assertEquals(m2.getAttributes("x").getValue("y"), "y");
+
+ // expect original manifest m1 unaffected
+ assertNull(m1.getAttributes("x").getValue("y"));
+ }
+
+ @Test
+ public void testDeepCloneMainAttributes() throws Exception {
+ Manifest m1 = new Manifest();
+ Manifest m2 = m1.clone();
+
+ // modify main attributes of manifest m1 after m2 was copied from m1
+ m1.getMainAttributes().putValue("x", "x");
+ assertEquals(m1.getMainAttributes().getValue("x"), "x");
+
+ // expect earlier copy unaffected
+ assertNull(m2.getMainAttributes().getValue("x"));
+
+ // modify attributes of copied manifest m2
+ m2.getMainAttributes().putValue("y", "y");
+ assertEquals(m2.getMainAttributes().getValue("y"), "y");
+
+ // expect original manifest m1 unaffected
+ assertNull(m1.getMainAttributes().getValue("y"));
+ }
+
+ @Test
+ public void testDeepCloneEntries() throws Exception {
+ Manifest m1 = new Manifest();
+ Attributes a1 = new Attributes();
+ m1.getEntries().put("x", a1);
+ Manifest m2 = m1.clone();
+
+ // modify entries of manifest m1 after m2 was copied from m1
+ assertNotNull(m1.getEntries().remove("x"));
+
+ // expect earlier copy unaffected
+ assertNotNull(m2.getAttributes("x"));
+
+ // modify attributes of copied manifest m2
+ m2.getEntries().put("y", new Attributes());
+ assertNotNull(m2.getAttributes("y"));
+
+ // expect original manifest m1 unaffected
+ assertNull(m1.getAttributes("y"));
+ }
+
+ @Test
+ public void testDeepCloneIndividualSectionAttributes() throws Exception {
+ Manifest m1 = new Manifest();
+ Attributes a1 = new Attributes();
+ m1.getEntries().put("x", a1);
+ Manifest m2 = m1.clone();
+
+ // modify attributes a1 of manifest m1 after m2 was copied from m1
+ a1.putValue("x", "x");
+ assertEquals(m1.getAttributes("x").getValue("x"), "x");
+
+ // expect earlier copy unaffected
+ assertNull(m2.getAttributes("x").getValue("x"));
+
+ // modify attributes of copied manifest m2
+ m2.getAttributes("x").putValue("y", "y");
+ assertEquals(m2.getAttributes("x").getValue("y"), "y");
+
+ // expect original manifest m1 unaffected
+ assertNull(m1.getAttributes("x").getValue("y"));
+ }
+
+}
