Bug 463631: Regression: Referenced models are not read-only

https://bugs.eclipse.org/bugs/show_bug.cgi?id=463631

Fixed a bug in the DecoratorModelReadOnlyHandler that caused it to override
lower priority read-only policies with a 'writable' result on resources
that are not decorators of the model being edited.  And a similar treatment
for the user model resources that are decorated by a decorator model opened
as the 'main' model of an editor.

There was also a problem introduced recently in the ReadOnlyCache, which
resulted in the cache only being cleared once.  Subsequently, computed
read-only states of resources would be cached permanently (or, at least,
for the duration of the model set).

Fixing the DecoratorModelReadOnlyHandler bug exposed a latent bug in the
read-only policy for controlled resources.  The ControlledResourceTracker
did not properly handle the topology of a fragmented model in which a
sub-unit has multiple parent units (it stores multiple cross-resource-
contained root elements).  This was detected by JUnit tests for control
mode hanging on a make-writable dialog.

Finally, the ProfileMigrationTest in the decorator-models suite relied on
the broken behaviour of the read-only policy to prevent hanging on the
make-writable dialog when modifying a profile (that should have been
read-only under the standard policy).  This is fixed in the test by
disabling read-only checks in the transaction that modifies the profile.

3 files changed, 92 insertions, 32 deletions

diff --git a/tests/junit/plugins/infra/emf/org.eclipse.papyrus.infra.emf.readonly.tests/src/org/eclipse/papyrus/infra/emf/readonly/ReferencedModelReadOnlyHandlerTest.java b/tests/junit/plugins/infra/emf/org.eclipse.papyrus.infra.emf.readonly.tests/src/org/eclipse/papyrus/infra/emf/readonly/ReferencedModelReadOnlyHandlerTest.java
index cc89c3edb7a..f7c6f520814 100644
--- a/tests/junit/plugins/infra/emf/org.eclipse.papyrus.infra.emf.readonly.tests/src/org/eclipse/papyrus/infra/emf/readonly/ReferencedModelReadOnlyHandlerTest.java
+++ b/tests/junit/plugins/infra/emf/org.eclipse.papyrus.infra.emf.readonly.tests/src/org/eclipse/papyrus/infra/emf/readonly/ReferencedModelReadOnlyHandlerTest.java
@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2014 CEA and others.
+ * Copyright (c) 2014, 2015 CEA, Christian W. Damus, and others.
  *
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
@@ -8,6 +8,7 @@
  *
  * Contributors:
  *   Christian W. Damus (CEA) - Initial API and implementation
+ *   Christian W. Damus - bug 463631
  *
  */
 package org.eclipse.papyrus.infra.emf.readonly;
@@ -20,6 +21,7 @@ import static org.junit.Assert.fail;
 import static org.junit.Assume.assumeThat;
 
 import java.util.Collections;
+import java.util.Set;
 
 import org.eclipse.core.commands.operations.IOperationHistory;
 import org.eclipse.core.resources.IFile;
@@ -62,6 +64,7 @@ import org.junit.rules.TestName;
 
 import com.google.common.base.Optional;
 import com.google.common.collect.ImmutableList;
+import com.google.common.collect.Iterables;
 
 
 /**
@@ -175,8 +178,8 @@ public class ReferencedModelReadOnlyHandlerTest extends AbstractPapyrusTest {
 	public void testObjectURIsWithFragment() {
 		Property ssn = person.getAttribute("ssn", null);
 		URI uri = EcoreUtil.getURI(ssn);
-		assertThat(fixture.anyReadOnly(discretionAxes(), new URI[]{ uri }).or(false), is(false));
-		assertThat(fixture.canMakeWritable(discretionAxes(), new URI[]{ uri }).or(true), is(false));
+		assertThat(fixture.anyReadOnly(discretionAxes(), new URI[] { uri }).or(false), is(false));
+		assertThat(fixture.canMakeWritable(discretionAxes(), new URI[] { uri }).or(true), is(false));
 	}
 
 	//
@@ -195,17 +198,17 @@ public class ReferencedModelReadOnlyHandlerTest extends AbstractPapyrusTest {
 		Resource res = importModel();
 		saveModels();
 
-		model = (Model)res.getContents().get(0);
+		model = (Model) res.getContents().get(0);
 		core = model.getNestedPackage("core");
-		person = (Class)core.getOwnedType("Person");
+		person = (Class) core.getOwnedType("Person");
 	}
 
 	protected void createProject() throws Exception {
 		project = ResourcesPlugin.getWorkspace().getRoot().getProject(testName.getMethodName());
-		if(!project.exists()) {
+		if (!project.exists()) {
 			project.create(null);
 		}
-		if(!project.isOpen()) {
+		if (!project.isOpen()) {
 			project.open(null);
 		}
 	}
@@ -240,7 +243,7 @@ public class ReferencedModelReadOnlyHandlerTest extends AbstractPapyrusTest {
 		domain = null;
 		fixture = null;
 
-		for(Resource next : rset.getResources()) {
+		for (Resource next : rset.getResources()) {
 			next.unload();
 			next.eAdapters().clear();
 		}
@@ -269,7 +272,7 @@ public class ReferencedModelReadOnlyHandlerTest extends AbstractPapyrusTest {
 		URI source = URI.createPlatformPluginURI("org.eclipse.papyrus.infra.emf.readonly.tests/resources/" + name, true);
 		URI destination = URI.createPlatformResourceURI(project.getName(), true);
 
-		for(String component : ImmutableList.of("uml", "notation", "di")) {
+		for (String component : ImmutableList.of("uml", "notation", "di")) {
 			Resource res = rset.getResource(source.appendFileExtension(component), true);
 			EcoreUtil.resolveAll(res);
 			res.setURI(destination.appendSegment(res.getURI().lastSegment()));
@@ -277,8 +280,8 @@ public class ReferencedModelReadOnlyHandlerTest extends AbstractPapyrusTest {
 	}
 
 	void saveModels() throws Exception {
-		for(Resource next : services.getService(ModelSet.class).getResources()) {
-			if(next.getURI().isPlatformResource()) {
+		for (Resource next : services.getService(ModelSet.class).getResources()) {
+			if (next.getURI().isPlatformResource()) {
 				next.save(null);
 			}
 		}
@@ -288,7 +291,7 @@ public class ReferencedModelReadOnlyHandlerTest extends AbstractPapyrusTest {
 		URI unit = element.eResource().getURI().trimSegments(1).appendSegment("units").appendSegment(URI.encodeSegment(element.getName() + ".uml", true));
 		execute(ControlModeManager.getInstance().getControlCommand(new ControlModeRequest(domain, element, unit)));
 
-		assertThat("Controlling the unit failed", ((InternalEObject)element).eDirectResource(), notNullValue());
+		assertThat("Controlling the unit failed", ((InternalEObject) element).eDirectResource(), notNullValue());
 
 		try {
 			saveModels();
@@ -299,7 +302,7 @@ public class ReferencedModelReadOnlyHandlerTest extends AbstractPapyrusTest {
 	}
 
 	void execute(ICommand command) {
-		IOperationHistory history = ((IWorkspaceCommandStack)domain.getCommandStack()).getOperationHistory();
+		IOperationHistory history = ((IWorkspaceCommandStack) domain.getCommandStack()).getOperationHistory();
 
 		try {
 			history.execute(command, null, null);
@@ -327,11 +330,12 @@ public class ReferencedModelReadOnlyHandlerTest extends AbstractPapyrusTest {
 	void assertLocalViewsNotReadOnly(EObject object) {
 		boolean foundSomeLocalViews = false;
 
-		for(EStructuralFeature.Setting setting : CacheAdapter.getCacheAdapter(object).getNonNavigableInverseReferences(object)) {
-			if(setting.getEStructuralFeature() == NotationPackage.Literals.VIEW__ELEMENT) {
+		for (EStructuralFeature.Setting setting : CacheAdapter.getCacheAdapter(object).getNonNavigableInverseReferences(object)) {
+			if (setting.getEStructuralFeature() == NotationPackage.Literals.VIEW__ELEMENT) {
 				EObject view = setting.getEObject();
-				URI uri = ControlledResourceTracker.getInstance(domain).getRootResourceURI(view.eResource().getURI());
-				if(uri.trimFileExtension().lastSegment().equals("referencing")) {
+				Set<URI> uris = ControlledResourceTracker.getInstance(domain).getRootResourceURIs(view.eResource().getURI());
+				assertThat(uris.size(), is(1));
+				if (Iterables.getOnlyElement(uris).trimFileExtension().lastSegment().equals("referencing")) {
 					foundSomeLocalViews = true;
 					assertNotReadOnly(view);
 				}
diff --git a/tests/junit/plugins/uml/decoratormodel/org.eclipse.papyrus.uml.decoratormodel.tests/src/org/eclipse/papyrus/uml/decoratormodel/tests/AbstractProfileExternalizationTest.java b/tests/junit/plugins/uml/decoratormodel/org.eclipse.papyrus.uml.decoratormodel.tests/src/org/eclipse/papyrus/uml/decoratormodel/tests/AbstractProfileExternalizationTest.java
index 1750d9291a6..4c6d9e5dde2 100644
--- a/tests/junit/plugins/uml/decoratormodel/org.eclipse.papyrus.uml.decoratormodel.tests/src/org/eclipse/papyrus/uml/decoratormodel/tests/AbstractProfileExternalizationTest.java
+++ b/tests/junit/plugins/uml/decoratormodel/org.eclipse.papyrus.uml.decoratormodel.tests/src/org/eclipse/papyrus/uml/decoratormodel/tests/AbstractProfileExternalizationTest.java
@@ -1,5 +1,5 @@
 /*****************************************************************************
- * Copyright (c) 2014 Christian W. Damus and others.
+ * Copyright (c) 2014, 2015 Christian W. Damus and others.
  * 
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
@@ -19,9 +19,9 @@ import static org.hamcrest.CoreMatchers.nullValue;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.junit.Assert.fail;
 
+import java.util.Collection;
 import java.util.Collections;
 import java.util.Iterator;
-import java.util.List;
 import java.util.Map;
 import java.util.Set;
 import java.util.concurrent.Callable;
@@ -39,6 +39,8 @@ import org.eclipse.emf.ecore.xmi.XMLResource;
 import org.eclipse.emf.edit.command.ChangeCommand;
 import org.eclipse.emf.edit.domain.EditingDomain;
 import org.eclipse.emf.transaction.RecordingCommand;
+import org.eclipse.emf.transaction.RollbackException;
+import org.eclipse.emf.transaction.TransactionalCommandStack;
 import org.eclipse.emf.transaction.TransactionalEditingDomain;
 import org.eclipse.papyrus.infra.core.resource.ModelSet;
 import org.eclipse.papyrus.infra.emf.utils.EMFHelper;
@@ -68,6 +70,7 @@ import org.junit.Rule;
 import org.junit.rules.TestName;
 
 import com.google.common.collect.ImmutableSet;
+import com.google.common.collect.Iterables;
 import com.google.common.collect.Iterators;
 import com.google.common.collect.Lists;
 import com.google.common.collect.Maps;
@@ -180,10 +183,18 @@ public abstract class AbstractProfileExternalizationTest extends AbstractPapyrus
 	}
 
 	protected void execute(final Runnable write) {
-		Command command;
+		execute(command(write));
+	}
+
+	protected void execute(final Runnable write, Map<?, ?> options) {
+		execute(command(write), options);
+	}
+
+	private Command command(final Runnable write) {
+		Command result;
 
 		if (getTestContext() instanceof TransactionalEditingDomain) {
-			command = new RecordingCommand((TransactionalEditingDomain) getTestContext()) {
+			result = new RecordingCommand((TransactionalEditingDomain) getTestContext()) {
 
 				@Override
 				protected void doExecute() {
@@ -191,7 +202,7 @@ public abstract class AbstractProfileExternalizationTest extends AbstractPapyrus
 				}
 			};
 		} else {
-			command = new ChangeCommand(getTestContextResourceSet()) {
+			result = new ChangeCommand(getTestContextResourceSet()) {
 				@Override
 				protected void doExecute() {
 					write.run();
@@ -199,15 +210,33 @@ public abstract class AbstractProfileExternalizationTest extends AbstractPapyrus
 			};
 		}
 
-		execute(command);
+		return result;
 	}
 
 	protected <V> V execute(final Callable<V> write) {
-		final List<V> result = Lists.newArrayListWithCapacity(1);
-		Command command;
+		Command command = command(write);
+		execute(command);
+
+		@SuppressWarnings("unchecked")
+		V result = (V) Iterables.getFirst(command.getResult(), null);
+		return result;
+	}
+
+	protected <V> V execute(final Callable<V> write, Map<?, ?> options) {
+		Command command = command(write);
+		execute(command, options);
+
+		@SuppressWarnings("unchecked")
+		V result = (V) Iterables.getFirst(command.getResult(), null);
+		return result;
+	}
+
+	private Command command(final Callable<?> write) {
+		Command result;
 
 		if (getTestContext() instanceof TransactionalEditingDomain) {
-			command = new RecordingCommand((TransactionalEditingDomain) getTestContext()) {
+			result = new RecordingCommand((TransactionalEditingDomain) getTestContext()) {
+				private final Collection<Object> result = Lists.newArrayListWithCapacity(1);
 
 				@Override
 				protected void doExecute() {
@@ -218,9 +247,16 @@ public abstract class AbstractProfileExternalizationTest extends AbstractPapyrus
 						fail("Exception in write operation: " + e.getLocalizedMessage());
 					}
 				}
+
+				@Override
+				public Collection<?> getResult() {
+					return result;
+				}
 			};
 		} else {
-			command = new ChangeCommand(getTestContextResourceSet()) {
+			result = new ChangeCommand(getTestContextResourceSet()) {
+				private final Collection<Object> result = Lists.newArrayListWithCapacity(1);
+
 				@Override
 				protected void doExecute() {
 					try {
@@ -230,18 +266,32 @@ public abstract class AbstractProfileExternalizationTest extends AbstractPapyrus
 						fail("Exception in write operation: " + e.getLocalizedMessage());
 					}
 				}
+
+				@Override
+				public Collection<?> getResult() {
+					return result;
+				}
 			};
 		}
 
-		execute(command);
-
-		return result.get(0);
+		return result;
 	}
 
 	protected void execute(Command command) {
 		getTestContext().getCommandStack().execute(command);
 	}
 
+	protected void execute(Command command, Map<?, ?> options) {
+		try {
+			((TransactionalCommandStack) getTestContext().getCommandStack()).execute(command, options);
+		} catch (RollbackException e) {
+			e.printStackTrace();
+			fail("Command execution rolled back: " + e.getLocalizedMessage());
+		} catch (InterruptedException e) {
+			fail("Command execution interrupted");
+		}
+	}
+
 	protected void undo() {
 		getTestContext().getCommandStack().undo();
 	}
diff --git a/tests/junit/plugins/uml/decoratormodel/org.eclipse.papyrus.uml.decoratormodel.tests/src/org/eclipse/papyrus/uml/decoratormodel/tests/ProfileMigrationTest.java b/tests/junit/plugins/uml/decoratormodel/org.eclipse.papyrus.uml.decoratormodel.tests/src/org/eclipse/papyrus/uml/decoratormodel/tests/ProfileMigrationTest.java
index dea79f32d44..1180bbdd0e3 100644
--- a/tests/junit/plugins/uml/decoratormodel/org.eclipse.papyrus.uml.decoratormodel.tests/src/org/eclipse/papyrus/uml/decoratormodel/tests/ProfileMigrationTest.java
+++ b/tests/junit/plugins/uml/decoratormodel/org.eclipse.papyrus.uml.decoratormodel.tests/src/org/eclipse/papyrus/uml/decoratormodel/tests/ProfileMigrationTest.java
@@ -1,5 +1,5 @@
 /*****************************************************************************
- * Copyright (c) 2014 Christian W. Damus and others.
+ * Copyright (c) 2014, 2015 Christian W. Damus and others.
  * 
  * All rights reserved. This program and the accompanying materials
  * are made available under the terms of the Eclipse Public License v1.0
@@ -16,6 +16,10 @@ package org.eclipse.papyrus.uml.decoratormodel.tests;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
 
+import java.util.EnumSet;
+
+import org.eclipse.papyrus.infra.core.resource.ReadOnlyAxis;
+import org.eclipse.papyrus.infra.core.utils.TransactionHelper;
 import org.eclipse.papyrus.junit.utils.rules.PluginResource;
 import org.eclipse.papyrus.uml.tools.helper.IProfileApplicationDelegate;
 import org.eclipse.uml2.uml.LiteralBoolean;
@@ -62,6 +66,8 @@ public class ProfileMigrationTest extends AbstractProfileExternalizationTest {
 
 	@Before
 	public void defineNewProfileVersion() {
+		// Referenced profiles are read-only in the context of the model being edited, so
+		// disable read-only controls
 		execute(new Runnable() {
 
 			@Override
@@ -72,7 +78,7 @@ public class ProfileMigrationTest extends AbstractProfileExternalizationTest {
 				bool.setValue(true);
 				profile.define();
 			}
-		});
+		}, TransactionHelper.readOnlyAxisOption(EnumSet.noneOf(ReadOnlyAxis.class)));
 	}
 
 	void migrate(final Package package_, final Profile profile) {