Bug 488570 - Need to be more robust in the container persistence whenI20160301-1230I20160301-0800

faced with invalid attribute types

Change-Id: Ia8f3b7d6139d04c13e11917525517d55ffe4f0bb
Signed-off-by: Thomas Watson <tjwatson@us.ibm.com>

3 files changed, 114 insertions, 21 deletions

diff --git a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java
index 7426a9335..33b33ddd9 100644
--- a/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java
+++ b/bundles/org.eclipse.osgi.tests/src/org/eclipse/osgi/tests/container/TestModuleContainer.java
@@ -14,6 +14,7 @@ import static java.util.jar.Attributes.Name.MANIFEST_VERSION;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNotNull;
 import static org.junit.Assert.assertNull;
+import static org.junit.Assert.assertTrue;
 import static org.junit.Assert.fail;
 
 import java.io.*;
@@ -2408,7 +2409,94 @@ public class TestModuleContainer extends AbstractTest {
 		assertNotNull("No requirer found.", requirerModule);
 		requirerAttrs = requirerModule.getCurrentRevision().getRequirements("optional").get(0).getAttributes();
 		assertEquals("Wrong requirer attrs", attrs, requirerAttrs);
+	}
+
+	@Test
+	public void testInvalidAttributes() throws IOException, BundleException {
+		DummyContainerAdaptor adaptor = createDummyAdaptor();
+		ModuleContainer container = adaptor.getContainer();
+
+		// install the system.bundle
+		installDummyModule("system.bundle.MF", Constants.SYSTEM_BUNDLE_LOCATION, Constants.SYSTEM_BUNDLE_SYMBOLICNAME, null, null, container);
+
+		// provider with all supported types
+		Map<String, String> invalidAttrManifest = new HashMap<String, String>();
+		invalidAttrManifest.put(Constants.BUNDLE_MANIFESTVERSION, "2");
+		invalidAttrManifest.put(Constants.BUNDLE_SYMBOLICNAME, "invalid");
+
+		invalidAttrManifest.put(Constants.PROVIDE_CAPABILITY, "provider.cap; invalid:Boolean=true");
+		checkInvalidManifest(invalidAttrManifest, container);
+
+		invalidAttrManifest.put(Constants.PROVIDE_CAPABILITY, "provider.cap; invalid:Integer=1");
+		checkInvalidManifest(invalidAttrManifest, container);
+
+		invalidAttrManifest.put(Constants.PROVIDE_CAPABILITY, "provider.cap; invalid:List<Boolean>=true");
+		checkInvalidManifest(invalidAttrManifest, container);
+
+		invalidAttrManifest.put(Constants.PROVIDE_CAPABILITY, "provider.cap; invalid:List<Integer>=1");
+		checkInvalidManifest(invalidAttrManifest, container);
+	}
+
+	private void checkInvalidManifest(Map<String, String> invalidAttrManifest, ModuleContainer container) {
+		try {
+			installDummyModule(invalidAttrManifest, "invalid", container);
+			fail("Expected to get a BundleException with MANIFEST_ERROR");
+		} catch (BundleException e) {
+			// find expected type
+			assertEquals("Wrong type.", BundleException.MANIFEST_ERROR, e.getType());
+		}
+	}
+
+	@Test
+	public void testStoreInvalidAttributes() throws BundleException, IOException {
+		DummyContainerAdaptor adaptor = createDummyAdaptor();
+		ModuleContainer container = adaptor.getContainer();
+
+		// install the system.bundle
+		installDummyModule("system.bundle.MF", Constants.SYSTEM_BUNDLE_LOCATION, Constants.SYSTEM_BUNDLE_SYMBOLICNAME, null, null, container);
+
+		Integer testInt = Integer.valueOf(1);
+		List<Integer> testIntList = Collections.singletonList(testInt);
+		ModuleRevisionBuilder builder = new ModuleRevisionBuilder();
+		builder.setSymbolicName("invalid.attr");
+		builder.setVersion(Version.valueOf("1.0.0"));
+		builder.addCapability("test", Collections.<String, String> emptyMap(), Collections.singletonMap("test", (Object) testInt));
+		builder.addCapability("test.list", Collections.<String, String> emptyMap(), Collections.singletonMap("test.list", (Object) testIntList));
+		Module invalid = container.install(null, builder.getSymbolicName(), builder, null);
+
+		Object testAttr = invalid.getCurrentRevision().getCapabilities("test").get(0).getAttributes().get("test");
+		assertEquals("Wrong test attr", testInt, testAttr);
+
+		Object testAttrList = invalid.getCurrentRevision().getCapabilities("test.list").get(0).getAttributes().get("test.list");
+		assertEquals("Wrong test list attr", testIntList, testAttrList);
+
+		ByteArrayOutputStream bytes = new ByteArrayOutputStream();
+		DataOutputStream data = new DataOutputStream(bytes);
+		adaptor.getDatabase().store(data, true);
+
+		List<DummyContainerEvent> events = adaptor.getDatabase().getContainerEvents();
+		// make sure we see the errors
+		assertEquals("Wrong number of events.", 2, events.size());
+		for (DummyContainerEvent event : events) {
+			assertEquals("Wrong type of event.", ContainerEvent.ERROR, event.type);
+			assertTrue("Wrong type of exception.", event.error instanceof BundleException);
+		}
+
+		// reload into a new container
+		adaptor = createDummyAdaptor();
+		container = adaptor.getContainer();
+		adaptor.getDatabase().load(new DataInputStream(new ByteArrayInputStream(bytes.toByteArray())));
+
+		invalid = container.getModule("invalid.attr");
+		assertNotNull("Could not find module.", invalid);
+
+		String testIntString = String.valueOf(testInt);
+		List<String> testIntStringList = Collections.singletonList(testIntString);
+		testAttr = invalid.getCurrentRevision().getCapabilities("test").get(0).getAttributes().get("test");
+		assertEquals("Wrong test attr", testIntString, testAttr);
 
+		testAttrList = invalid.getCurrentRevision().getCapabilities("test.list").get(0).getAttributes().get("test.list");
+		assertEquals("Wrong test list attr", testIntStringList, testAttrList);
 	}
 
 	@Test
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleDatabase.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleDatabase.java
index d0f66bc22..72e4a94cb 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleDatabase.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/ModuleDatabase.java
@@ -18,14 +18,14 @@ import java.util.concurrent.locks.ReentrantReadWriteLock.ReadLock;
 import java.util.concurrent.locks.ReentrantReadWriteLock.WriteLock;
 import org.eclipse.osgi.container.Module.Settings;
 import org.eclipse.osgi.container.Module.State;
+import org.eclipse.osgi.container.ModuleContainerAdaptor.ContainerEvent;
 import org.eclipse.osgi.container.ModuleRevisionBuilder.GenericInfo;
 import org.eclipse.osgi.container.namespaces.EquinoxModuleDataNamespace;
 import org.eclipse.osgi.framework.util.ObjectPool;
 import org.eclipse.osgi.internal.container.Capabilities;
 import org.eclipse.osgi.internal.container.ComputeNodeOrder;
 import org.eclipse.osgi.internal.framework.EquinoxConfiguration;
-import org.osgi.framework.Constants;
-import org.osgi.framework.Version;
+import org.osgi.framework.*;
 import org.osgi.framework.namespace.PackageNamespace;
 import org.osgi.framework.wiring.BundleRevision;
 import org.osgi.resource.*;
@@ -999,7 +999,7 @@ public class ModuleDatabase {
 			// Followed by maps which may reference the strings and versions
 			out.writeInt(allMaps.size());
 			for (Map<String, ?> map : allMaps) {
-				writeMap(map, out, objectTable);
+				writeMap(map, out, objectTable, moduleDatabase);
 				out.writeInt(addToWriteTable(map, objectTable));
 			}
 
@@ -1404,7 +1404,7 @@ public class ModuleDatabase {
 
 		}
 
-		private static void writeMap(Map<String, ?> source, DataOutputStream out, Map<Object, Integer> objectTable) throws IOException {
+		private static void writeMap(Map<String, ?> source, DataOutputStream out, Map<Object, Integer> objectTable, ModuleDatabase moduleDatabase) throws IOException {
 			if (source == null) {
 				out.writeInt(0);
 			} else {
@@ -1428,12 +1428,13 @@ public class ModuleDatabase {
 						writeVersion((Version) value, out, objectTable);
 					} else if (value instanceof List) {
 						out.writeByte(VALUE_LIST);
-						writeList(out, (List<?>) value, objectTable);
+						writeList(out, key, (List<?>) value, objectTable, moduleDatabase);
 					} else {
-						// better do our best and write a string
-						// probably should warn here
+						// do our best and write a string; post an error.
+						// This will be difficult to debug because we don't know which module it is coming from, but it is better than being silent
+						moduleDatabase.adaptor.publishContainerEvent(ContainerEvent.ERROR, moduleDatabase.getModule(0), new BundleException("Invalid map value: " + key + " = " + value.getClass().getName() + '[' + value + ']')); //$NON-NLS-1$ //$NON-NLS-2$
 						out.writeByte(VALUE_STRING);
-						writeString((String) value, out, objectTable);
+						writeString(String.valueOf(value), out, objectTable);
 					}
 				}
 			}
@@ -1484,18 +1485,18 @@ public class ModuleDatabase {
 			}
 		}
 
-		private static void writeList(DataOutputStream out, List<?> list, Map<Object, Integer> objectTable) throws IOException {
+		private static void writeList(DataOutputStream out, String key, List<?> list, Map<Object, Integer> objectTable, ModuleDatabase moduleDatabase) throws IOException {
 			if (list.isEmpty()) {
 				out.writeInt(0);
 				return;
 			}
 			byte type = getListType(list);
-			if (type < 0) {
+			if (type == -1) {
 				out.writeInt(0);
 				return; // don't understand the list type
 			}
 			out.writeInt(list.size());
-			out.writeByte(type);
+			out.writeByte(type == -2 ? VALUE_STRING : type);
 			for (Object value : list) {
 				switch (type) {
 					case VALUE_STRING :
@@ -1511,6 +1512,10 @@ public class ModuleDatabase {
 						writeVersion((Version) value, out, objectTable);
 						break;
 					default :
+						// do our best and write a string; post an error.
+						// This will be difficult to debug because we don't know which module it is coming from, but it is better than being silent
+						moduleDatabase.adaptor.publishContainerEvent(ContainerEvent.ERROR, moduleDatabase.getModule(0), new BundleException("Invalid list element in map: " + key + " = " + value.getClass().getName() + '[' + value + ']')); //$NON-NLS-1$ //$NON-NLS-2$
+						writeString(String.valueOf(value), out, objectTable);
 						break;
 				}
 			}
diff --git a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/builders/OSGiManifestBuilderFactory.java b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/builders/OSGiManifestBuilderFactory.java
index b3b568aca..8a08c2dd9 100644
--- a/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/builders/OSGiManifestBuilderFactory.java
+++ b/bundles/org.eclipse.osgi/container/src/org/eclipse/osgi/container/builders/OSGiManifestBuilderFactory.java
@@ -306,7 +306,7 @@ public final class OSGiManifestBuilderFactory {
 		return symbolicName == null ? symbolicNameAlias : symbolicName;
 	}
 
-	private static void getPackageExports(ModuleRevisionBuilder builder, ManifestElement[] exportElements, Object symbolicName, Collection<Map<String, Object>> exportedPackages) {
+	private static void getPackageExports(ModuleRevisionBuilder builder, ManifestElement[] exportElements, Object symbolicName, Collection<Map<String, Object>> exportedPackages) throws BundleException {
 		if (exportElements == null)
 			return;
 		for (ManifestElement exportElement : exportElements) {
@@ -342,7 +342,7 @@ public final class OSGiManifestBuilderFactory {
 			addImplicitImports(builder, exportedPackages, importPackageNames);
 	}
 
-	private static void addPackageImports(ModuleRevisionBuilder builder, ManifestElement[] importElements, Collection<String> importPackageNames, boolean dynamic) {
+	private static void addPackageImports(ModuleRevisionBuilder builder, ManifestElement[] importElements, Collection<String> importPackageNames, boolean dynamic) throws BundleException {
 		if (importElements == null)
 			return;
 		for (ManifestElement importElement : importElements) {
@@ -424,7 +424,7 @@ public final class OSGiManifestBuilderFactory {
 		return directives;
 	}
 
-	private static void getRequireBundle(ModuleRevisionBuilder builder, ManifestElement[] requireBundles) {
+	private static void getRequireBundle(ModuleRevisionBuilder builder, ManifestElement[] requireBundles) throws BundleException {
 		if (requireBundles == null)
 			return;
 		for (ManifestElement requireElement : requireBundles) {
@@ -469,7 +469,7 @@ public final class OSGiManifestBuilderFactory {
 		}
 	}
 
-	private static void getFragmentHost(ModuleRevisionBuilder builder, ManifestElement[] fragmentHosts) {
+	private static void getFragmentHost(ModuleRevisionBuilder builder, ManifestElement[] fragmentHosts) throws BundleException {
 		if (fragmentHosts == null || fragmentHosts.length == 0)
 			return;
 
@@ -518,7 +518,7 @@ public final class OSGiManifestBuilderFactory {
 		}
 	}
 
-	private static void getRequireCapabilities(ModuleRevisionBuilder builder, ManifestElement[] requireElements) {
+	private static void getRequireCapabilities(ModuleRevisionBuilder builder, ManifestElement[] requireElements) throws BundleException {
 		if (requireElements == null)
 			return;
 		for (ManifestElement requireElement : requireElements) {
@@ -641,7 +641,7 @@ public final class OSGiManifestBuilderFactory {
 		}
 	}
 
-	private static Map<String, Object> getAttributes(ManifestElement element) {
+	private static Map<String, Object> getAttributes(ManifestElement element) throws BundleException {
 		Enumeration<String> keys = element.getKeys();
 		Map<String, Object> attributes = new HashMap<String, Object>();
 		if (keys == null)
@@ -660,12 +660,12 @@ public final class OSGiManifestBuilderFactory {
 		return attributes;
 	}
 
-	private static Object convertValueWithNoWhitespace(String type, String value) {
+	private static Object convertValueWithNoWhitespace(String type, String value) throws BundleException {
 		value = value.replaceAll("\\s", ""); //$NON-NLS-1$//$NON-NLS-2$
 		return convertValue(type, value);
 	}
 
-	private static Object convertValue(String type, String value) {
+	private static Object convertValue(String type, String value) throws BundleException {
 		if (ATTR_TYPE_STRING.equalsIgnoreCase(type)) {
 			return value;
 		}
@@ -688,13 +688,13 @@ public final class OSGiManifestBuilderFactory {
 		Tokenizer listTokenizer = new Tokenizer(type);
 		String listType = listTokenizer.getToken("<"); //$NON-NLS-1$
 		if (!ATTR_TYPE_LIST.equalsIgnoreCase(listType))
-			throw new RuntimeException("Unsupported type: " + type); //$NON-NLS-1$
+			throw new BundleException("Unsupported type: " + type, BundleException.MANIFEST_ERROR); //$NON-NLS-1$
 		char c = listTokenizer.getChar();
 		String componentType = ATTR_TYPE_STRING;
 		if (c == '<') {
 			componentType = listTokenizer.getToken(">"); //$NON-NLS-1$
 			if (listTokenizer.getChar() != '>')
-				throw new RuntimeException("Invalid type, missing ending '>' : " + type); //$NON-NLS-1$
+				throw new BundleException("Invalid type, missing ending '>' : " + type, BundleException.MANIFEST_ERROR); //$NON-NLS-1$
 		}
 		List<String> tokens = new Tokenizer(value).getEscapedTokens(","); //$NON-NLS-1$
 		List<Object> components = new ArrayList<Object>();