[328681] LockObjectsRequest can cause corruption of client-side graph

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

2 files changed, 40 insertions, 30 deletions

diff --git a/plugins/org.eclipse.emf.cdo.server.net4j/src/org/eclipse/emf/cdo/server/internal/net4j/protocol/LockObjectsIndication.java b/plugins/org.eclipse.emf.cdo.server.net4j/src/org/eclipse/emf/cdo/server/internal/net4j/protocol/LockObjectsIndication.java
index 04bd19cbd0..8466f254c8 100644
--- a/plugins/org.eclipse.emf.cdo.server.net4j/src/org/eclipse/emf/cdo/server/internal/net4j/protocol/LockObjectsIndication.java
+++ b/plugins/org.eclipse.emf.cdo.server.net4j/src/org/eclipse/emf/cdo/server/internal/net4j/protocol/LockObjectsIndication.java
@@ -8,16 +8,20 @@
  * Contributors:
  *    Simon McDuff - initial API and implementation
  *    Eike Stepper - maintenance
+ *    Caspar De Groot - maintenance
  */
 package org.eclipse.emf.cdo.server.internal.net4j.protocol;
 
 import org.eclipse.emf.cdo.common.branch.CDOBranch;
+import org.eclipse.emf.cdo.common.id.CDOID;
 import org.eclipse.emf.cdo.common.id.CDOIDUtil;
 import org.eclipse.emf.cdo.common.protocol.CDODataInput;
 import org.eclipse.emf.cdo.common.protocol.CDODataOutput;
 import org.eclipse.emf.cdo.common.protocol.CDOProtocolConstants;
+import org.eclipse.emf.cdo.common.revision.CDORevision;
 import org.eclipse.emf.cdo.common.revision.CDORevisionKey;
 import org.eclipse.emf.cdo.server.IView;
+import org.eclipse.emf.cdo.spi.common.revision.InternalCDORevision;
 import org.eclipse.emf.cdo.spi.server.InternalLockManager;
 
 import org.eclipse.net4j.util.WrappedException;
@@ -30,14 +34,10 @@ import java.util.List;
 /**
  * @author Simon McDuff
  */
-public class LockObjectsIndication extends RefreshSessionIndication
+public class LockObjectsIndication extends CDOReadIndication
 {
   private List<Object> objectsToBeLocked = new ArrayList<Object>();
 
-  private IView view;
-
-  private LockType lockType;
-
   public LockObjectsIndication(CDOServerProtocol protocol)
   {
     super(protocol, CDOProtocolConstants.SIGNAL_LOCK_OBJECTS);
@@ -46,14 +46,21 @@ public class LockObjectsIndication extends RefreshSessionIndication
   @Override
   protected void indicating(CDODataInput in) throws IOException
   {
-    super.indicating(in);
     int viewID = in.readInt();
-    lockType = in.readCDOLockType();
+    LockType lockType = in.readCDOLockType();
     long timeout = in.readLong();
+    CDOBranch viewedBranch = in.readCDOBranch();
 
-    view = getSession().getView(viewID);
-    InternalLockManager lockManager = getRepository().getLockManager();
+    int nRevisions = in.readInt();
+    CDORevisionKey[] revKeys = new CDORevisionKey[nRevisions];
+    for (int i = 0; i < nRevisions; i++)
+    {
+      revKeys[i] = in.readCDORevisionKey();
+      handleViewedRevision(viewedBranch, revKeys[i]);
+    }
 
+    IView view = getSession().getView(viewID);
+    InternalLockManager lockManager = getRepository().getLockManager();
     try
     {
       lockManager.lock(lockType, view, objectsToBeLocked, timeout);
@@ -64,31 +71,40 @@ public class LockObjectsIndication extends RefreshSessionIndication
     }
   }
 
-  @Override
-  protected CDORevisionKey handleViewedRevision(CDOBranch branch, CDORevisionKey revision)
+  private void handleViewedRevision(CDOBranch viewedBranch, CDORevisionKey revKey)
   {
+    // TODO (CD) I'm using IllegalArgExceptions here because that's how it worked before. But
+    // personally I don't think it makes a lot of sense to throw exceptions from #indicating or
+    // #responding when *expected* problems are detected, especially because they trigger
+    // a separate signal. Why not just report problems through the response stream?
+
+    CDOID id = revKey.getID();
+    InternalCDORevision rev = getRepository().getRevisionManager().getRevision(id, viewedBranch.getHead(),
+        CDORevision.UNCHUNKED, CDORevision.DEPTH_NONE, true);
+    if (rev == null)
+    {
+      throw new IllegalArgumentException(String.format("Object %s not found in branch %s (possibly detached)", id,
+          viewedBranch));
+    }
+    if (!revKey.equals(rev))
+    {
+      throw new IllegalArgumentException(String.format(
+          "Client's revision of object %s is not the latest version in branch %s", id, viewedBranch));
+    }
+
     if (getRepository().isSupportingBranches())
     {
-      objectsToBeLocked.add(CDOIDUtil.createIDAndBranch(revision.getID(), branch));
+      objectsToBeLocked.add(CDOIDUtil.createIDAndBranch(id, viewedBranch));
     }
     else
     {
-      objectsToBeLocked.add(revision.getID());
+      objectsToBeLocked.add(id);
     }
-
-    return revision;
-  }
-
-  @Override
-  protected void writeDetachedObject(CDODataOutput out, CDORevisionKey key) throws IOException
-  {
-    getRepository().getLockManager().unlock(lockType, view, objectsToBeLocked);
-    throw new IllegalArgumentException("Object has been detached: " + key); //$NON-NLS-1$
   }
 
   @Override
-  protected void respondingDone()
+  protected void responding(CDODataOutput out) throws IOException
   {
-    // Do nothing
+    out.writeBoolean(true);
   }
 }
diff --git a/plugins/org.eclipse.emf.cdo.server.net4j/src/org/eclipse/emf/cdo/server/internal/net4j/protocol/RefreshSessionIndication.java b/plugins/org.eclipse.emf.cdo.server.net4j/src/org/eclipse/emf/cdo/server/internal/net4j/protocol/RefreshSessionIndication.java
index 7273095d6d..d71fa4c9c1 100644
--- a/plugins/org.eclipse.emf.cdo.server.net4j/src/org/eclipse/emf/cdo/server/internal/net4j/protocol/RefreshSessionIndication.java
+++ b/plugins/org.eclipse.emf.cdo.server.net4j/src/org/eclipse/emf/cdo/server/internal/net4j/protocol/RefreshSessionIndication.java
@@ -80,17 +80,11 @@ public class RefreshSessionIndication extends CDOReadIndication
       for (int j = 0; j < size; j++)
       {
         CDORevisionKey revision = in.readCDORevisionKey();
-        revision = handleViewedRevision(branch, revision);
         revisions.add(revision);
       }
     }
   }
 
-  protected CDORevisionKey handleViewedRevision(CDOBranch branch, CDORevisionKey revision)
-  {
-    return revision;
-  }
-
   @Override
   protected void responding(CDODataOutput out) throws IOException
   {