Apply inverse edge edits after committing primary object edits

Summary:
Fixes T13082. When you create a revision (say, `D111`) with `Ref T222` in the body, we write a `D111 -> T222` edge ("revision 111 references task 222") and an inverse `T222 -> D111` edge ("task 222 is referenced by revision 111").

We also apply a transaction to `D111` ("alice added a task: Txxx.") and an inverse transaction to `T222` ("alice added a revision: Dxxx").

Currently, it appears that the inverse transaction can sometimes generate mail faster than `D111` actually commits its (database) transactions, so the mail says "alice added a revision: Unknown Object (Differential Revision)". See T13082 for evidence that this is true, and a reproduction case.

To fix this, apply the inverse transaction (to `T222`) after we commit the main object (here, `D111`).

This is tricky because when we apply transactions, the transaction editor automatically "fixes" them to be consistent with the database state. For example, if a task already has title "XYZ" and you set the title to "XYZ" (same title), we just no-op the transaction.

It also fixes edge edits. The old sequence was:

  - Open (database) transaction.
  - Apply our transaction ("alice added a task").
  - Apply the inverse transaction ("alice added a revision").
  - Write the edges to the database.
  - Commit (database) transaction.

Under this sequence, the inverse transaction was "correct" and didn't need to be fixed, so the fixing step didn't touch it.

The new sequence is:

  - Open (database) transaction.
  - Apply our transaction ("alice added a task").
  - Write the edges.
  - Commit (database) transaction.
  - Apply the inverse transaction ("alice added a revision").

Since the inverse transaction now happens after the database edge write, the fixing step detects that it's a no-op and throws it away if we do this naively.

Instead, add some special cases around inverse edits to skip the correction/fixing logic, and just pass the "right" values in the first place.

Test Plan:
Added and removed related tasks from revisions, saw appropriate transactions render on both objects.

(It's hard to be certain this completely fixes the issue since it only happened occasionally in the first place, but we can see if it happens any more on `secure`.)

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13082, T222

Differential Revision: https://secure.phabricator.com/D19969

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -447,6 +447,12 @@ abstract class PhabricatorApplicationTransactionEditor
               'edge:type'));
         }
 
+        // See T13082. If this is an inverse edit, the parent editor has
+        // already populated the transaction values correctly.
+        if ($this->getIsInverseEdgeEditor()) {
+          return $xaction->getOldValue();
+        }
+
         $old_edges = array();
         if ($object->getPHID()) {
           $edge_src = $object->getPHID();
@@ -513,6 +519,12 @@ abstract class PhabricatorApplicationTransactionEditor
           return $space_phid;
         }
       case PhabricatorTransactions::TYPE_EDGE:
+        // See T13082. If this is an inverse edit, the parent editor has
+        // already populated appropriate transaction values.
+        if ($this->getIsInverseEdgeEditor()) {
+          return $xaction->getNewValue();
+        }
+
         $new_value = $this->getEdgeTransactionNewValue($xaction);
 
         $edge_type = $xaction->getMetadataValue('edge:type');
@@ -790,14 +802,6 @@ abstract class PhabricatorApplicationTransactionEditor
         $src = $object->getPHID();
         $const = $xaction->getMetadataValue('edge:type');
 
-        $type = PhabricatorEdgeType::getByConstant($const);
-        if ($type->shouldWriteInverseTransactions()) {
-          $this->applyInverseEdgeTransactions(
-            $object,
-            $xaction,
-            $type->getInverseEdgeConstant());
-        }
-
         foreach ($new as $dst_phid => $edge) {
           $new[$dst_phid]['src'] = $src;
         }
@@ -900,6 +904,30 @@ abstract class PhabricatorApplicationTransactionEditor
     foreach ($xactions as $xaction) {
       $type = $xaction->getTransactionType();
 
+      // See T13082. When we're writing edges that imply corresponding inverse
+      // transactions, apply those inverse transactions now. We have to wait
+      // until the object we're editing (with this editor) has committed its
+      // transactions to do this. If we don't, the inverse editor may race,
+      // build a mail before we actually commit this object, and render "alice
+      // added an edge: Unknown Object".
+
+      if ($type === PhabricatorTransactions::TYPE_EDGE) {
+        // Don't do anything if we're already an inverse edge editor.
+        if ($this->getIsInverseEdgeEditor()) {
+          continue;
+        }
+
+        $edge_const = $xaction->getMetadataValue('edge:type');
+        $edge_type = PhabricatorEdgeType::getByConstant($edge_const);
+        if ($edge_type->shouldWriteInverseTransactions()) {
+          $this->applyInverseEdgeTransactions(
+            $object,
+            $xaction,
+            $edge_type->getInverseEdgeConstant());
+        }
+        continue;
+      }
+
       $xtype = $this->getModularTransactionType($type);
       if (!$xtype) {
         continue;
@@ -1504,6 +1532,12 @@ abstract class PhabricatorApplicationTransactionEditor
       $expect_value = !$xaction->shouldGenerateOldValue();
       $has_value = $xaction->hasOldValue();
 
+      // See T13082. In the narrow case of applying inverse edge edits, we
+      // expect the old value to be populated.
+      if ($this->getIsInverseEdgeEditor()) {
+        $expect_value = true;
+      }
+
       if ($expect_value && !$has_value) {
         throw new PhabricatorApplicationTransactionStructureException(
           $xaction,
@@ -3853,6 +3887,8 @@ abstract class PhabricatorApplicationTransactionEditor
       ->withPHIDs($all)
       ->execute();
 
+    $object_phid = $object->getPHID();
+
     foreach ($nodes as $node) {
       if (!($node instanceof PhabricatorApplicationTransactionInterface)) {
         continue;
@@ -3865,22 +3901,38 @@ abstract class PhabricatorApplicationTransactionEditor
         continue;
       }
 
+      $node_phid = $node->getPHID();
       $editor = $node->getApplicationTransactionEditor();
       $template = $node->getApplicationTransactionTemplate();
 
-      if (isset($add[$node->getPHID()])) {
-        $edge_edit_type = '+';
+      // See T13082. We have to build these transactions with synthetic values
+      // because we've already applied the actual edit to the edge database
+      // table. If we try to apply this transaction naturally, it will no-op
+      // itself because it doesn't have any effect.
+
+      $edge_query = id(new PhabricatorEdgeQuery())
+        ->withSourcePHIDs(array($node_phid))
+        ->withEdgeTypes(array($inverse_type));
+
+      $edge_query->execute();
+
+      $edge_phids = $edge_query->getDestinationPHIDs();
+      $edge_phids = array_fuse($edge_phids);
+
+      $new_phids = $edge_phids;
+      $old_phids = $edge_phids;
+
+      if (isset($add[$node_phid])) {
+        unset($old_phids[$object_phid]);
       } else {
-        $edge_edit_type = '-';
+        $old_phids[$object_phid] = $object_phid;
       }
 
       $template
         ->setTransactionType($xaction->getTransactionType())
         ->setMetadataValue('edge:type', $inverse_type)
-        ->setNewValue(
-          array(
-            $edge_edit_type => array($object->getPHID() => $object->getPHID()),
-          ));
+        ->setOldValue($old_phids)
+        ->setNewValue($new_phids);
 
       $editor
         ->setContinueOnNoEffect(true)