[303466] CDO not robust when using dynamic packages

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

6 files changed, 288 insertions, 4 deletions

diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/common/model/CDOModelUtil.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/common/model/CDOModelUtil.java
index 6b5f696326..24ee4ba0fc 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/common/model/CDOModelUtil.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/common/model/CDOModelUtil.java
@@ -22,14 +22,18 @@ import org.eclipse.net4j.util.io.ExtendedDataOutput;
 import org.eclipse.emf.common.notify.Adapter;
 import org.eclipse.emf.common.util.EList;
 import org.eclipse.emf.common.util.Enumerator;
+import org.eclipse.emf.common.util.TreeIterator;
 import org.eclipse.emf.ecore.EClass;
 import org.eclipse.emf.ecore.EClassifier;
 import org.eclipse.emf.ecore.EDataType;
 import org.eclipse.emf.ecore.EEnum;
 import org.eclipse.emf.ecore.EEnumLiteral;
+import org.eclipse.emf.ecore.EGenericType;
+import org.eclipse.emf.ecore.EObject;
 import org.eclipse.emf.ecore.EPackage;
 import org.eclipse.emf.ecore.EStructuralFeature;
 import org.eclipse.emf.ecore.EcorePackage;
+import org.eclipse.emf.ecore.InternalEObject;
 import org.eclipse.emf.ecore.resource.ResourceSet;
 import org.eclipse.emf.ecore.util.EcoreUtil;
 import org.eclipse.emf.ecore.util.FeatureMapUtil;
@@ -377,6 +381,8 @@ public final class CDOModelUtil
   public static void writePackage(ExtendedDataOutput out, EPackage ePackage, boolean zipped,
       EPackage.Registry packageRegistry) throws IOException
   {
+    checkCrossResourceURIs(ePackage);
+
     byte[] bytes = EMFUtil.getEPackageBytes(ePackage, zipped, packageRegistry);
     out.writeString(ePackage.getNsURI());
     out.writeBoolean(zipped);
@@ -386,6 +392,70 @@ public final class CDOModelUtil
   /**
    * @since 3.0
    */
+  public static void checkCrossResourceURIs(EPackage ePackage)
+  {
+    TreeIterator<EObject> it = ePackage.eAllContents();
+    while (it.hasNext())
+    {
+      EObject e = it.next();
+      for (EObject r : e.eCrossReferences())
+      {
+        if (r.eIsProxy())
+        {
+          String msg = "Package '%s' contains unresolved proxy '%s'";
+          msg = String.format(msg, ePackage.getNsURI(), ((InternalEObject)r).eProxyURI());
+          throw new IllegalStateException(msg);
+        }
+
+        if (r.eResource() != null && r.eResource() != e.eResource())
+        {
+          // It's a ref into another resource
+          EPackage pkg = null;
+          if (r instanceof EClassifier)
+          {
+            pkg = ((EClassifier)r).getEPackage();
+          }
+          else if (r instanceof EStructuralFeature)
+          {
+            EStructuralFeature feature = (EStructuralFeature)r;
+            EClass ownerClass = (EClass)feature.eContainer();
+            pkg = ownerClass.getEPackage();
+          }
+          else if (r instanceof EGenericType)
+          {
+            EGenericType genType = (EGenericType)r;
+            EClassifier c = genType.getEClassifier();
+            if (c != null)
+            {
+              pkg = c.getEPackage();
+            }
+          }
+
+          if (pkg == null)
+          {
+            continue;
+          }
+
+          while (pkg.getESuperPackage() != null)
+          {
+            pkg = pkg.getESuperPackage();
+          }
+
+          String resourceURI = r.eResource().getURI().toString();
+          if (!resourceURI.toString().equals(pkg.getNsURI()))
+          {
+            String msg = "URI of the resource (%s) does not match the nsURI (%s) of the top-level package";
+            msg = String.format(msg, resourceURI, pkg.getNsURI());
+            throw new IllegalStateException(msg);
+          }
+        }
+      }
+    }
+  }
+
+  /**
+   * @since 3.0
+   */
   public static EPackage readPackage(ExtendedDataInput in, ResourceSet resourceSet, boolean lookForResource)
       throws IOException
   {
diff --git a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/common/model/EMFUtil.java b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/common/model/EMFUtil.java
index 23b606304d..e455d03218 100644
--- a/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/common/model/EMFUtil.java
+++ b/plugins/org.eclipse.emf.cdo.common/src/org/eclipse/emf/cdo/common/model/EMFUtil.java
@@ -17,6 +17,7 @@ import org.eclipse.net4j.util.WrappedException;
 import org.eclipse.emf.common.notify.Adapter;
 import org.eclipse.emf.common.notify.Notifier;
 import org.eclipse.emf.common.util.EList;
+import org.eclipse.emf.common.util.TreeIterator;
 import org.eclipse.emf.common.util.URI;
 import org.eclipse.emf.ecore.EAttribute;
 import org.eclipse.emf.ecore.EClass;
@@ -27,6 +28,7 @@ import org.eclipse.emf.ecore.EReference;
 import org.eclipse.emf.ecore.EStructuralFeature;
 import org.eclipse.emf.ecore.EcoreFactory;
 import org.eclipse.emf.ecore.EcorePackage;
+import org.eclipse.emf.ecore.InternalEObject;
 import org.eclipse.emf.ecore.impl.EPackageImpl;
 import org.eclipse.emf.ecore.resource.Resource;
 import org.eclipse.emf.ecore.resource.ResourceSet;
@@ -40,6 +42,7 @@ import java.util.ArrayList;
 import java.util.Arrays;
 import java.util.Comparator;
 import java.util.HashMap;
+import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Map.Entry;
@@ -377,4 +380,44 @@ public final class EMFUtil
   {
     return newEcoreResourceSet(EPackage.Registry.INSTANCE);
   }
+
+  /**
+   * @since 3.0
+   */
+  public static EObject safeResolve(EObject proxy, ResourceSet resourceSet)
+  {
+    if (!proxy.eIsProxy())
+    {
+      return proxy;
+    }
+
+    EObject resolved = EcoreUtil.resolve(proxy, resourceSet);
+    if (resolved == proxy)
+    {
+      throw new IllegalStateException("Unresolvable proxy: " + ((InternalEObject)proxy).eProxyURI());
+    }
+
+    return resolved;
+  }
+
+  /**
+   * @since 3.0
+   */
+  public static void safeResolveAll(ResourceSet resourceSet)
+  {
+    TreeIterator<Notifier> it = resourceSet.getAllContents();
+    while (it.hasNext())
+    {
+      Notifier notifier = it.next();
+      if (notifier instanceof EObject)
+      {
+        EMFUtil.safeResolve((EObject)notifier, resourceSet);
+        Iterator<EObject> it2 = ((EObject)notifier).eCrossReferences().iterator();
+        while (it2.hasNext())
+        {
+          EMFUtil.safeResolve(it2.next(), resourceSet);
+        }
+      }
+    }
+  }
 }
diff --git a/plugins/org.eclipse.emf.cdo.server.net4j/src/org/eclipse/emf/cdo/server/internal/net4j/protocol/CommitTransactionIndication.java b/plugins/org.eclipse.emf.cdo.server.net4j/src/org/eclipse/emf/cdo/server/internal/net4j/protocol/CommitTransactionIndication.java
index 8ddcbfd0cf..34c475bec3 100644
--- a/plugins/org.eclipse.emf.cdo.server.net4j/src/org/eclipse/emf/cdo/server/internal/net4j/protocol/CommitTransactionIndication.java
+++ b/plugins/org.eclipse.emf.cdo.server.net4j/src/org/eclipse/emf/cdo/server/internal/net4j/protocol/CommitTransactionIndication.java
@@ -50,8 +50,10 @@ import org.eclipse.net4j.util.om.monitor.OMMonitor;
 import org.eclipse.net4j.util.om.monitor.ProgressDistributor;
 import org.eclipse.net4j.util.om.trace.ContextTracer;
 
+import org.eclipse.emf.ecore.resource.Resource;
 import org.eclipse.emf.ecore.resource.ResourceSet;
-import org.eclipse.emf.ecore.util.EcoreUtil;
+import org.eclipse.emf.ecore.resource.impl.ResourceSetImpl;
+import org.eclipse.emf.ecore.xmi.impl.EcoreResourceFactoryImpl;
 
 import java.io.IOException;
 import java.util.List;
@@ -201,7 +203,7 @@ public class CommitTransactionIndication extends IndicationWithMonitoring
       }
 
       InternalCDOPackageRegistry packageRegistry = commitContext.getPackageRegistry();
-      ResourceSet resourceSet = EMFUtil.newEcoreResourceSet(packageRegistry);
+      ResourceSet resourceSet = createResourceSet(packageRegistry);
       for (int i = 0; i < newPackageUnits.length; i++)
       {
         newPackageUnits[i] = (InternalCDOPackageUnit)in.readCDOPackageUnit(resourceSet);
@@ -210,7 +212,8 @@ public class CommitTransactionIndication extends IndicationWithMonitoring
       }
 
       // When all packages are deserialized and registered, resolve them
-      EcoreUtil.resolveAll(resourceSet);
+      // Note: EcoreUtil.resolveAll(resourceSet) does *not* do the trick
+      EMFUtil.safeResolveAll(resourceSet);
 
       // New objects
       if (TRACER.isEnabled())
@@ -254,6 +257,23 @@ public class CommitTransactionIndication extends IndicationWithMonitoring
     }
   }
 
+  private ResourceSet createResourceSet(InternalCDOPackageRegistry packageRegistry)
+  {
+    ResourceSet resourceSet = new ResourceSetImpl()
+    {
+      @Override
+      protected void demandLoad(Resource resource) throws IOException
+      {
+        // Do nothing: we don't want this ResourceSet to attempt demandloads.
+      }
+    };
+
+    Resource.Factory resourceFactory = new EcoreResourceFactoryImpl();
+    resourceSet.getResourceFactoryRegistry().getExtensionToFactoryMap().put("*", resourceFactory); //$NON-NLS-1$
+    resourceSet.setPackageRegistry(packageRegistry);
+    return resourceSet;
+  }
+
   protected void initializeCommitContext(CDODataInput in) throws Exception
   {
     int viewID = in.readInt();
diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllTestsAllConfigs.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllTestsAllConfigs.java
index e95c6232f0..85ddd68607 100644
--- a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllTestsAllConfigs.java
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/AllTestsAllConfigs.java
@@ -66,6 +66,7 @@ import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_294859_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_298561_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_299190_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_302233_Test;
+import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_303466_Test;
 import org.eclipse.emf.cdo.tests.bugzilla.Bugzilla_303807_Test;
 import org.eclipse.emf.cdo.tests.config.impl.ConfigTest;
 import org.eclipse.emf.cdo.tests.config.impl.ConfigTestSuite;
@@ -193,6 +194,7 @@ public abstract class AllTestsAllConfigs extends ConfigTestSuite
     testClasses.add(Bugzilla_298561_Test.class);
     testClasses.add(Bugzilla_299190_Test.class);
     testClasses.add(Bugzilla_302233_Test.class);
+    testClasses.add(Bugzilla_303466_Test.class);
     testClasses.add(Bugzilla_303807_Test.class);
 
     // TODO testClasses.add(NonCDOResourceTest.class);
diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/PackageRegistryTest.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/PackageRegistryTest.java
index f9cc7aac37..3cab2e5f83 100644
--- a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/PackageRegistryTest.java
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/PackageRegistryTest.java
@@ -678,7 +678,7 @@ public class PackageRegistryTest extends AbstractCDOTest
     assertEquals("Eike", company.getName());
   }
 
-  private static EPackage loadModel(String fileName) throws IOException
+  public static EPackage loadModel(String fileName) throws IOException
   {
     URI uri = URI.createURI("file://" + fileName);
     XMIResource resource = new XMIResourceImpl(uri);
diff --git a/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_303466_Test.java b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_303466_Test.java
new file mode 100644
index 0000000000..98fe5c230f
--- /dev/null
+++ b/plugins/org.eclipse.emf.cdo.tests/src/org/eclipse/emf/cdo/tests/bugzilla/Bugzilla_303466_Test.java
@@ -0,0 +1,149 @@
+/**
+ * Copyright (c) 2004 - 2010 Eike Stepper (Berlin, Germany) and others.
+ * All rights reserved. This program and the accompanying materials
+ * are made available under the terms of the Eclipse Public License v1.0
+ * which accompanies this distribution, and is available at
+ * http://www.eclipse.org/legal/epl-v10.html
+ * 
+ * Contributors:
+ *    Caspar De Groot
+ */
+package org.eclipse.emf.cdo.tests.bugzilla;
+
+import org.eclipse.emf.cdo.common.model.EMFUtil;
+import org.eclipse.emf.cdo.eresource.CDOResource;
+import org.eclipse.emf.cdo.session.CDOSession;
+import org.eclipse.emf.cdo.tests.AbstractCDOTest;
+import org.eclipse.emf.cdo.tests.PackageRegistryTest;
+import org.eclipse.emf.cdo.transaction.CDOTransaction;
+
+import org.eclipse.net4j.signal.RemoteException;
+import org.eclipse.net4j.util.transaction.TransactionException;
+
+import org.eclipse.emf.common.util.URI;
+import org.eclipse.emf.ecore.EClass;
+import org.eclipse.emf.ecore.EObject;
+import org.eclipse.emf.ecore.EPackage;
+import org.eclipse.emf.ecore.resource.Resource;
+import org.eclipse.emf.ecore.resource.ResourceSet;
+
+import java.io.IOException;
+import java.util.Map;
+
+/**
+ * CDO not robust when using dynamic packages
+ * 
+ * @author Caspar De Groot
+ */
+public class Bugzilla_303466_Test extends AbstractCDOTest
+{
+  public void test_missingDependency() throws IOException
+  {
+    CDOSession session = openSession();
+
+    ResourceSet resourceSet = EMFUtil.newEcoreResourceSet();
+    EPackage derivedPkg = PackageRegistryTest.loadModel("model/derived.ecore");
+    Resource resource = derivedPkg.eResource();
+    resourceSet.getResources().add(resource);
+    resource.load(null);
+
+    session.getPackageRegistry().putEPackage(derivedPkg);
+    assertEquals(3, session.getPackageRegistry().size());
+
+    EClass derivedClass = (EClass)derivedPkg.getEClassifier("DerivedClass");
+    EObject derived = derivedPkg.getEFactoryInstance().create(derivedClass);
+
+    // Verify that the feature inherited from the base class is missing
+    assertNull(derivedClass.getEStructuralFeature("couter"));
+
+    CDOTransaction tx = session.openTransaction();
+    CDOResource resource2 = tx.createResource("/resource1");
+    resource2.getContents().add(derived);
+
+    try
+    {
+      tx.commit();
+      fail("Should have thrown an exception because of the missing base package");
+    }
+    catch (TransactionException e)
+    {
+      if (e.getCause() instanceof IllegalStateException)
+      {
+        // Good
+      }
+      else if (e.getCause() instanceof RemoteException)
+      {
+        fail("Problem should have been detected on the client side");
+      }
+      else
+      {
+        throw e;
+      }
+    }
+    finally
+    {
+      tx.close();
+      session.close();
+    }
+  }
+
+  public void test_badUris() throws IOException
+  {
+    CDOSession session = openSession();
+
+    ResourceSet resourceSet = EMFUtil.newEcoreResourceSet();
+
+    EPackage basePkg = PackageRegistryTest.loadModel("model/base.ecore");
+    Resource resource1 = basePkg.eResource();
+    resourceSet.getResources().add(resource1);
+    resourceSet.getPackageRegistry().put(basePkg.getNsURI(), basePkg);
+
+    EPackage derivedPkg = PackageRegistryTest.loadModel("model/derived.ecore");
+    Resource resource2 = derivedPkg.eResource();
+    resourceSet.getResources().add(resource2);
+    resourceSet.getPackageRegistry().put(basePkg.getNsURI(), derivedPkg);
+
+    Map<URI, URI> uriMap = resourceSet.getURIConverter().getURIMap();
+    URI modelUri3 = URI.createFileURI("base.ecore");
+    uriMap.put(modelUri3, resource1.getURI());
+
+    EMFUtil.safeResolveAll(resourceSet);
+
+    session.getPackageRegistry().putEPackage(basePkg);
+    session.getPackageRegistry().putEPackage(derivedPkg);
+
+    EClass derivedClass = (EClass)derivedPkg.getEClassifier("DerivedClass");
+    EObject derived = derivedPkg.getEFactoryInstance().create(derivedClass);
+
+    // Verify that the feature inherited from the base class is NOT missing on the client side
+    assertNotNull(derivedClass.getEStructuralFeature("couter"));
+
+    CDOTransaction tx = session.openTransaction();
+    CDOResource resource3 = tx.createResource("/resource1");
+    resource3.getContents().add(derived);
+
+    try
+    {
+      tx.commit();
+      fail("Should have thrown an exception because of the file URIs in the dependent package");
+    }
+    catch (TransactionException e)
+    {
+      if (e.getCause() instanceof IllegalStateException)
+      {
+        // Good
+      }
+      else if (e.getCause() instanceof RemoteException)
+      {
+        fail("Problem should have been detected on the client side");
+      }
+      else
+      {
+        fail("Should have thrown an " + IllegalStateException.class.getName());
+      }
+    }
+
+    tx.close();
+    session.close();
+  }
+}