Bug 547461 - Make all ProvidedCapability's ctors use immutable mapsI20190807-1800

ProvidedCapability(String, Map<String, Object>):
- (good) stores properties in an unmodifiable map
- (bad) properties still could be changed via original map
- (bad) would not create an empty version if missing
- (bad) holds a reference to the original map

ProvidedCapability(String, String, Version):
- (bad) stores properties in a mutable map

Always store properties as unmodifiable map to prevent their mutation
via getProperties().put(). As query operations on that map are "read
through" to the original map, first make a copy of original
properties.

Change-Id: If7cf15d153d72e4bedc30e9ad52423d5121f3fa2
Signed-off-by: Mykola Nikishov <mn@mn.com.ua>

2 files changed, 71 insertions, 6 deletions

diff --git a/bundles/org.eclipse.equinox.p2.metadata/src/org/eclipse/equinox/internal/p2/metadata/ProvidedCapability.java b/bundles/org.eclipse.equinox.p2.metadata/src/org/eclipse/equinox/internal/p2/metadata/ProvidedCapability.java
index 5b22d2960..9c251928d 100644
--- a/bundles/org.eclipse.equinox.p2.metadata/src/org/eclipse/equinox/internal/p2/metadata/ProvidedCapability.java
+++ b/bundles/org.eclipse.equinox.p2.metadata/src/org/eclipse/equinox/internal/p2/metadata/ProvidedCapability.java
@@ -61,21 +61,23 @@ public class ProvidedCapability implements IProvidedCapability, IMemberProvider
 		// Verify the version
 		Object version = resolvedProps.get(PROPERTY_VERSION);
 		if (version != null) {
-			Assert.isTrue(props.get(PROPERTY_VERSION) instanceof Version);
+			Assert.isTrue(version instanceof Version);
 		} else {
 			resolvedProps.put(PROPERTY_VERSION, Version.emptyVersion);
 		}
 
-		this.properties = Collections.unmodifiableMap(props);
+		this.properties = Collections.unmodifiableMap(resolvedProps);
 	}
 
 	public ProvidedCapability(String namespace, String name, Version version) {
 		Assert.isNotNull(namespace, NLS.bind(Messages.provided_capability_namespace_not_defined, null));
 		Assert.isNotNull(name, NLS.bind(Messages.provided_capability_name_not_defined, namespace));
 		this.namespace = namespace;
-		this.properties = new HashMap<>();
-		properties.put(namespace, name);
-		properties.put(PROPERTY_VERSION, version == null ? Version.emptyVersion : version);
+
+		Map<String, Object> props = new HashMap<>();
+		props.put(namespace, name);
+		props.put(PROPERTY_VERSION, version == null ? Version.emptyVersion : version);
+		this.properties = Collections.unmodifiableMap(props);
 	}
 
 	@Override
diff --git a/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/metadata/ProvidedCapabilityTest.java b/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/metadata/ProvidedCapabilityTest.java
index d1478c731..44875a623 100644
--- a/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/metadata/ProvidedCapabilityTest.java
+++ b/bundles/org.eclipse.equinox.p2.tests/src/org/eclipse/equinox/p2/tests/metadata/ProvidedCapabilityTest.java
@@ -13,7 +13,11 @@
  *******************************************************************************/
 package org.eclipse.equinox.p2.tests.metadata;
 
-import org.eclipse.equinox.p2.metadata.*;
+import java.util.HashMap;
+import java.util.Map;
+import org.eclipse.equinox.p2.metadata.IProvidedCapability;
+import org.eclipse.equinox.p2.metadata.MetadataFactory;
+import org.eclipse.equinox.p2.metadata.Version;
 import org.eclipse.equinox.p2.tests.AbstractProvisioningTest;
 
 /**
@@ -28,4 +32,63 @@ public class ProvidedCapabilityTest extends AbstractProvisioningTest {
 		assertFalse("1.1", cap.equals(notEqual));
 		assertFalse("1.1", notEqual.equals(cap));
 	}
+
+	public void testProperties_Unmodifiable() {
+		String namespace = "aNamespace";
+		String name = "name";
+		Version version = Version.createOSGi(2, 0, 0);
+
+		Map properties = new HashMap<>();
+		properties.put(namespace, name);
+		properties.put(IProvidedCapability.PROPERTY_VERSION, version);
+
+		IProvidedCapability capability1 = MetadataFactory.createProvidedCapability(namespace, properties);
+		IProvidedCapability capability2 = MetadataFactory.createProvidedCapability(namespace, name, version);
+		assertEquals(capability1, capability2);
+
+		try {
+			capability1.getProperties().put("key", "value");
+			fail("properties must be unmodifiable");
+		} catch (UnsupportedOperationException e) {
+			// ok
+		}
+
+		try {
+			capability2.getProperties().put("key", "value");
+			fail("properties must be unmodifiable");
+		} catch (UnsupportedOperationException e) {
+			// ok
+		}
+	}
+
+	public void testProperties_Immutable() {
+		String namespace = "aNamespace";
+		String name = "name";
+		Version version = Version.createOSGi(2, 0, 0);
+
+		Map properties = new HashMap<>();
+		properties.put(namespace, name);
+		properties.put(IProvidedCapability.PROPERTY_VERSION, version);
+
+		IProvidedCapability capability1 = MetadataFactory.createProvidedCapability(namespace, properties);
+		IProvidedCapability capability2 = MetadataFactory.createProvidedCapability(namespace, name, version);
+
+		// mutate original value
+		properties.put(IProvidedCapability.PROPERTY_VERSION, Version.createOSGi(9, 9, 9));
+
+		assertEquals(capability1, capability2);
+	}
+
+	public void testProperties_NoVersion() {
+		String namespace = "aNamespace";
+		String name = "name";
+
+		Map properties = new HashMap<>();
+		properties.put(namespace, name);
+		// no version this time
+
+		IProvidedCapability capability1 = MetadataFactory.createProvidedCapability(namespace, properties);
+		IProvidedCapability capability2 = MetadataFactory.createProvidedCapability(namespace, name, null);
+		assertEquals(capability1, capability2);
+	}
 }