Move Task <=> Revision storage to Edges

Summary:
  - Add edges for this relationship.
  - Use edges to store this data.
  - Migrate old data.
  - Fix some warnings with generating feed stories about Aux and Edge transactions.
  - Fix a task-task edge issue with "Create Subtask".

Test Plan:
  - Migrated data, verified reivsions showed up.
  - Attached and detached tasks to revisions and vice versa.
  - Created a new revision with attached tasks.
  - Created a subtask.

Reviewers: btrahan, vrana

Reviewed By: btrahan

CC: aran

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

resources/sql/patches/migrate-maniphest-revisions.php

@@ -0,0 +1,42 @@
+<?php
+
+/*
+ * Copyright 2012 Facebook, Inc.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *   http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+echo "Migrating task revisions to edges...\n";
+foreach (new LiskMigrationIterator(new ManiphestTask()) as $task) {
+  $id = $task->getID();
+  echo "Task {$id}: ";
+
+  $revs = $task->getAttachedPHIDs(PhabricatorPHIDConstants::PHID_TYPE_DREV);
+  if (!$revs) {
+    echo "-\n";
+    continue;
+  }
+
+  $editor = new PhabricatorEdgeEditor();
+  $editor->setSuppressEvents(true);
+  foreach ($revs as $rev) {
+    $editor->addEdge(
+      $task->getPHID(),
+      PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV,
+      $rev);
+  }
+  $editor->save();
+  echo "OKAY\n";
+}
+
+echo "Done.\n";

src/__phutil_library_map__.php

@@ -814,7 +814,6 @@ phutil_register_library_map(array(
     'PhabricatorOAuthServerTestController' => 'applications/oauthserver/controller/PhabricatorOAuthServerTestController.php',
     'PhabricatorOAuthServerTokenController' => 'applications/oauthserver/controller/PhabricatorOAuthServerTokenController.php',
     'PhabricatorOAuthUnlinkController' => 'applications/auth/controller/PhabricatorOAuthUnlinkController.php',
-    'PhabricatorObjectAttachmentEditor' => 'applications/search/editor/PhabricatorObjectAttachmentEditor.php',
     'PhabricatorObjectHandle' => 'applications/phid/PhabricatorObjectHandle.php',
     'PhabricatorObjectHandleConstants' => 'applications/phid/handle/const/PhabricatorObjectHandleConstants.php',
     'PhabricatorObjectHandleData' => 'applications/phid/handle/PhabricatorObjectHandleData.php',

src/applications/differential/field/specification/DifferentialManiphestTasksFieldSpecification.php

@@ -49,8 +49,12 @@ final class DifferentialManiphestTasksFieldSpecification
 
   private function getManiphestTaskPHIDs() {
     $revision = $this->getRevision();
-    return $revision->getAttachedPHIDs(
+    if (!$revision->getPHID()) {
-      PhabricatorPHIDConstants::PHID_TYPE_TASK);
+      return array();
+    }
+    return PhabricatorEdgeQuery::loadDestinationPHIDs(
+      $revision->getPHID(),
+      PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK);
   }
 
   /**
@@ -59,14 +63,28 @@ final class DifferentialManiphestTasksFieldSpecification
    * @return void
    */
   public function didWriteRevision(DifferentialRevisionEditor $editor) {
-    $aeditor = new PhabricatorObjectAttachmentEditor(
+    $revision = $editor->getRevision();
-      PhabricatorPHIDConstants::PHID_TYPE_DREV,
+    $revision_phid = $revision->getPHID();
-      $editor->getRevision());
+    $edge_type = PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK;
-    $aeditor->setUser($this->getUser());
+
-    $aeditor->attachObjects(
+    $old_phids = PhabricatorEdgeQuery::loadDestinationPHIDs(
-      PhabricatorPHIDConstants::PHID_TYPE_TASK,
+      $revision_phid,
-      $this->maniphestTasks,
+      $edge_type);
-      $two_way = true);
+    $add_phids = $this->maniphestTasks;
+    $rem_phids = array_diff($old_phids, $add_phids);
+
+    $edge_editor = id(new PhabricatorEdgeEditor())
+      ->setUser($this->getUser());
+
+    foreach ($add_phids as $phid) {
+      $edge_editor->addEdge($revision_phid, $edge_type, $phid);
+    }
+
+    foreach ($rem_phids as $phid) {
+      $edge_editor->removeEdge($revision_phid, $edge_type, $phid);
+    }
+
+    $edge_editor->save();
   }
 
   protected function didSetRevision() {

src/applications/maniphest/constants/ManiphestAction.php

@@ -38,6 +38,8 @@ final class ManiphestAction extends ManiphestConstants {
   const ACTION_DESCRIPTION = ManiphestTransactionType::TYPE_DESCRIPTION;
   const ACTION_REASSIGN    = ManiphestTransactionType::TYPE_OWNER;
   const ACTION_ATTACH      = ManiphestTransactionType::TYPE_ATTACH;
+  const ACTION_EDGE        = ManiphestTransactionType::TYPE_EDGE;
+  const ACTION_AUXILIARY   = ManiphestTransactionType::TYPE_AUXILIARY;
 
   public static function getActionPastTenseVerb($action) {
     static $map = array(
@@ -53,7 +55,9 @@ final class ManiphestAction extends ManiphestConstants {
       self::ACTION_TITLE       => 'updated title of',
       self::ACTION_DESCRIPTION => 'updated description of',
       self::ACTION_ATTACH      => 'attached something to',
+      self::ACTION_EDGE        => 'changed related objects of',
       self::ACTION_REOPEN      => 'reopened',
+      self::ACTION_AUXILIARY   => 'updated an auxiliary field of',
     );
 
     return idx($map, $action, "brazenly {$action}'d");
@@ -67,12 +71,14 @@ final class ManiphestAction extends ManiphestConstants {
    */
   public static function selectStrongestAction(array $actions) {
     static $strengths = array(
+      self::ACTION_AUXILIARY   => -1,
       self::ACTION_UPDATE      => 0,
       self::ACTION_CC          => 1,
       self::ACTION_PROJECT     => 2,
       self::ACTION_DESCRIPTION => 3,
       self::ACTION_TITLE       => 4,
       self::ACTION_ATTACH      => 5,
+      self::ACTION_EDGE        => 5,
       self::ACTION_COMMENT     => 6,
       self::ACTION_PRIORITY    => 7,
       self::ACTION_REASSIGN    => 8,

src/applications/maniphest/controller/ManiphestTaskDetailController.php

@@ -54,6 +54,7 @@ final class ManiphestTaskDetailController extends ManiphestController {
     $e_commit = PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT;
     $e_dep_on = PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK;
     $e_dep_by = PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK;
+    $e_rev    = PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV;
 
     $phid = $task->getPHID();
 
@@ -64,12 +65,14 @@ final class ManiphestTaskDetailController extends ManiphestController {
           $e_commit,
           $e_dep_on,
           $e_dep_by,
+          $e_rev,
         ));
     $edges = $query->execute();
 
     $commit_phids = array_keys($edges[$phid][$e_commit]);
     $dep_on_tasks = array_keys($edges[$phid][$e_dep_on]);
     $dep_by_tasks = array_keys($edges[$phid][$e_dep_by]);
+    $revs         = array_keys($edges[$phid][$e_rev]);
 
     $phids = array_fill_keys($query->getDestinationPHIDs(), true);
 
@@ -177,10 +180,9 @@ final class ManiphestTaskDetailController extends ManiphestController {
         array_select_keys($handles, $dep_on_tasks));
     }
 
-    $revs = idx($attached, PhabricatorPHIDConstants::PHID_TYPE_DREV);
     if ($revs) {
       $dict['Revisions'] = $this->renderHandleList(
-        array_select_keys($handles, array_keys($revs)));
+        array_select_keys($handles, $revs));
     }
 
     if ($commit_phids) {

src/applications/maniphest/controller/ManiphestTaskEditController.php

@@ -248,22 +248,13 @@ final class ManiphestTaskEditController extends ManiphestController {
 
 
         if ($parent_task) {
-          $type_task = PhabricatorPHIDConstants::PHID_TYPE_TASK;
+          id(new PhabricatorEdgeEditor())
-
+            ->setUser($user)
-          // NOTE: It's safe to simply apply this transaction without doing
+            ->addEdge(
-          // cycle detection because we know the new task has no children.
+              $parent_task->getPHID(),
-          $new_value = $parent_task->getAttached();
+              PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK,
-          $new_value[$type_task][$task->getPHID()] = array();
+              $task->getPHID())
-
+            ->save();
-          $parent_xaction = clone $template;
-          $attach_type = ManiphestTransactionType::TYPE_ATTACH;
-          $parent_xaction->setTransactionType($attach_type);
-          $parent_xaction->setNewValue($new_value);
-
-          $editor = new ManiphestTransactionEditor();
-          $editor->setAuxiliaryFields($aux_fields);
-          $editor->applyTransactions($parent_task, array($parent_xaction));
-
           $workflow = $parent_task->getID();
         }
 

src/applications/maniphest/view/ManiphestTransactionDetailView.php

@@ -623,7 +623,7 @@ final class ManiphestTransactionDetailView extends ManiphestView {
     $links = array();
     foreach ($phids as $phid) {
       if ($this->forEmail) {
-        $links[] = $this->handles[$phid]->getName();
+        $links[] = $this->handles[$phid]->getFullName();
       } else {
         $links[] = $this->handles[$phid]->renderLink();
       }
@@ -664,6 +664,8 @@ final class ManiphestTransactionDetailView extends ManiphestView {
   private function getEdgeEmailTitle($type, array $list) {
     $count = count($list);
     switch ($type) {
+      case PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV:
+        return pht('DIFFERENTIAL %d REVISION(S)', $count);
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
         return pht('DEPENDS ON %d TASK(S)', $count);
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
@@ -681,6 +683,8 @@ final class ManiphestTransactionDetailView extends ManiphestView {
    */
   private function getEdgeAddVerb($type) {
     switch ($type) {
+      case PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV:
+        return pht('Added Revision');
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
         return pht('Added Dependency');
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
@@ -698,6 +702,8 @@ final class ManiphestTransactionDetailView extends ManiphestView {
    */
   private function getEdgeRemVerb($type) {
     switch ($type) {
+      case PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV:
+        return pht('Removed Revision');
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
         return pht('Removed Dependency');
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
@@ -715,6 +721,8 @@ final class ManiphestTransactionDetailView extends ManiphestView {
    */
   private function getEdgeEditVerb($type) {
     switch ($type) {
+      case PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV:
+        return pht('Changed Revisions');
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
         return pht('Changed Dependencies');
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
@@ -735,6 +743,8 @@ final class ManiphestTransactionDetailView extends ManiphestView {
     $count = count($add);
 
     switch ($type) {
+      case PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV:
+        return pht('added %d revision(s): %s', $count, $list);
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
         return pht('added %d dependencie(s): %s', $count, $list);
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
@@ -755,6 +765,8 @@ final class ManiphestTransactionDetailView extends ManiphestView {
     $count = count($rem);
 
     switch ($type) {
+      case PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV:
+        return pht('removed %d revision(s): %s', $count, $list);
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
         return pht('removed %d dependencie(s): %s', $count, $list);
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDED_ON_BY_TASK:
@@ -777,6 +789,14 @@ final class ManiphestTransactionDetailView extends ManiphestView {
     $rem_count = count($rem_list);
 
     switch ($type) {
+      case PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV:
+        return pht(
+          'changed %d revision(s), added %d: %s; removed %d: %s',
+          $add_count + $rem_count,
+          $add_count,
+          $add_list,
+          $rem_count,
+          $rem_list);
       case PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK:
         return pht(
           'changed %d dependencie(s), added %d: %s; removed %d: %s',

src/applications/search/controller/PhabricatorSearchAttachController.php

@@ -60,6 +60,7 @@ final class PhabricatorSearchAttachController
     switch ($this->action) {
       case self::ACTION_EDGE:
       case self::ACTION_DEPENDENCIES:
+      case self::ACTION_ATTACH:
         $edge_type = $this->getEdgeType($object_type, $attach_type);
         break;
     }
@@ -93,23 +94,7 @@ final class PhabricatorSearchAttachController
 
         return id(new AphrontReloadResponse())->setURI($handle->getURI());
       } else {
-        switch ($this->action) {
-          case self::ACTION_MERGE:
         return $this->performMerge($object, $handle, $phids);
-          case self::ACTION_ATTACH:
-            $editor = new PhabricatorObjectAttachmentEditor(
-              $object_type,
-              $object);
-            $editor->setUser($this->getRequest()->getUser());
-            $editor->attachObjects(
-              $attach_type,
-              $phids,
-              $two_way = true);
-
-            return id(new AphrontReloadResponse())->setURI($handle->getURI());
-          default:
-            throw new Exception("Unsupported attach action.");
-        }
       }
     } else {
       if ($edge_type) {
@@ -117,14 +102,8 @@ final class PhabricatorSearchAttachController
           $this->phid,
           $edge_type);
       } else {
-        switch ($this->action) {
+        // This is a merge.
-          case self::ACTION_ATTACH:
-            $phids = $object->getAttachedPHIDs($attach_type);
-            break;
-          default:
         $phids = array();
-            break;
-        }
       }
     }
 
@@ -282,9 +261,11 @@ final class PhabricatorSearchAttachController
       $t_task => array(
         $t_cmit => PhabricatorEdgeConfig::TYPE_TASK_HAS_COMMIT,
         $t_task => PhabricatorEdgeConfig::TYPE_TASK_DEPENDS_ON_TASK,
+        $t_drev => PhabricatorEdgeConfig::TYPE_TASK_HAS_RELATED_DREV,
       ),
       $t_drev => array(
         $t_drev => PhabricatorEdgeConfig::TYPE_DREV_DEPENDS_ON_DREV,
+        $t_task => PhabricatorEdgeConfig::TYPE_DREV_HAS_RELATED_TASK,
       ),
     );
 

src/applications/search/editor/PhabricatorObjectAttachmentEditor.php

@@ -1,139 +0,0 @@
-<?php
-
-/*
- * Copyright 2011 Facebook, Inc.
- *
- * Licensed under the Apache License, Version 2.0 (the "License");
- * you may not use this file except in compliance with the License.
- * You may obtain a copy of the License at
- *
- *   http://www.apache.org/licenses/LICENSE-2.0
- *
- * Unless required by applicable law or agreed to in writing, software
- * distributed under the License is distributed on an "AS IS" BASIS,
- * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
- * See the License for the specific language governing permissions and
- * limitations under the License.
- */
-
-/**
- * Manage attaching, detaching and updating edges between objects (for instance,
- * relationships between Tasks and Revisions).
- */
-final class PhabricatorObjectAttachmentEditor {
-
-  private $objectType;
-  private $object;
-  private $user;
-
-  public function __construct($object_type, $object) {
-    $this->objectType = $object_type;
-    $this->object = $object;
-  }
-
-  public function setUser(PhabricatorUser $user) {
-    $this->user = $user;
-    return $this;
-  }
-
-  public function attachObjects($attach_type, array $phids, $two_way) {
-    $object_type = $this->objectType;
-    $object = $this->object;
-    $object_phid = $object->getPHID();
-
-    // sort() so that removing [X, Y] and then adding [Y, X] is correctly
-    // detected as a no-op.
-    sort($phids);
-    $old_phids = $object->getAttachedPHIDs($attach_type);
-    sort($old_phids);
-    $phids = array_values($phids);
-    $old_phids = array_values($old_phids);
-
-    if ($phids === $old_phids) {
-      return;
-    }
-
-    $all_phids = array_merge($phids, $old_phids);
-    $attach_objs = id(new PhabricatorObjectHandleData($all_phids))
-      ->loadObjects();
-
-    // Remove PHIDs which don't actually exist, to prevent silliness.
-    $phids = array_keys(array_select_keys($attach_objs, $phids));
-    if ($phids) {
-      $phids = array_combine($phids, $phids);
-    }
-
-    // Update the primary object.
-    $this->writeOutboundEdges($object_type, $object, $attach_type, $phids);
-
-    if (!$two_way) {
-      return;
-    }
-
-    // Loop through all of the attached/detached objects and update them.
-    foreach ($attach_objs as $phid => $attach_obj) {
-      $attached_phids = $attach_obj->getAttachedPHIDs($object_type);
-      // Figure out if we're attaching or detaching this object.
-      if (isset($phids[$phid])) {
-        if (in_array($object_phid, $attached_phids)) {
-          // Already attached.
-          continue;
-        }
-        $attached_phids[] = $object_phid;
-      } else {
-        $attached_phids = array_fill_keys($attached_phids, true);
-        unset($attached_phids[$object_phid]);
-        $attached_phids = array_keys($attached_phids);
-      }
-      $this->writeOutboundEdges(
-        $attach_type,
-        $attach_obj,
-        $object_type,
-        $attached_phids);
-    }
-  }
-
-  private function writeOutboundEdges(
-    $object_type,
-    $object,
-    $attach_type,
-    array $attach_phids) {
-    switch ($object_type) {
-      case PhabricatorPHIDConstants::PHID_TYPE_DREV:
-        $object->setAttachedPHIDs($attach_type, $attach_phids);
-        $object->save();
-        break;
-      case PhabricatorPHIDConstants::PHID_TYPE_TASK:
-        $this->applyTaskTransaction(
-          $object,
-          $attach_type,
-          $attach_phids);
-        break;
-    }
-  }
-
-  private function applyTaskTransaction(
-    ManiphestTask $task,
-    $attach_type,
-    array $new_phids) {
-
-    if (!$this->user) {
-      throw new Exception("Call setUser() before editing attachments!");
-    }
-    $user = $this->user;
-
-    $editor = new ManiphestTransactionEditor();
-    $type = ManiphestTransactionType::TYPE_ATTACH;
-
-    $transaction = new ManiphestTransaction();
-    $transaction->setAuthorPHID($user->getPHID());
-    $transaction->setTransactionType($type);
-
-    $new = $task->getAttached();
-    $new[$attach_type] = array_fill_keys($new_phids, array());
-
-    $transaction->setNewValue($new);
-    $editor->applyTransactions($task, array($transaction));
-  }
-
-}

src/infrastructure/edges/constants/PhabricatorEdgeConfig.php

@@ -35,6 +35,9 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants {
   const TYPE_BLOG_HAS_BLOGGER           = 9;
   const TYPE_BLOGGER_HAS_BLOG           = 10;
 
+  const TYPE_TASK_HAS_RELATED_DREV      = 11;
+  const TYPE_DREV_HAS_RELATED_TASK      = 12;
+
   const TYPE_TEST_NO_CYCLE              = 9000;
 
   public static function getInverse($edge_type) {
@@ -52,6 +55,9 @@ final class PhabricatorEdgeConfig extends PhabricatorEdgeConstants {
       self::TYPE_POST_HAS_BLOG    => self::TYPE_BLOG_HAS_POST,
       self::TYPE_BLOG_HAS_BLOGGER => self::TYPE_BLOGGER_HAS_BLOG,
       self::TYPE_BLOGGER_HAS_BLOG => self::TYPE_BLOG_HAS_BLOGGER,
+
+      self::TYPE_TASK_HAS_RELATED_DREV => self::TYPE_DREV_HAS_RELATED_TASK,
+      self::TYPE_DREV_HAS_RELATED_TASK => self::TYPE_TASK_HAS_RELATED_DREV,
     );
 
     return idx($map, $edge_type);

src/infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php

@@ -103,6 +103,24 @@ abstract class PhabricatorBaseEnglishTranslation
         'DEPENDS ON TASKS',
       ),
 
+      'DIFFERENTIAL %d REVISION(S)' => array(
+        'DIFFERENTIAL REVISION',
+        'DIFFERENTIAL REVISIONS',
+      ),
+
+      'added %d revision(s): %s' => array(
+        'added revision: %2$s',
+        'added revisions: %2$s',
+      ),
+
+      'removed %d revision(s): %s' => array(
+        'removed revision: %2$s',
+        'removed revisions: %2$s',
+      ),
+
+      'changed %d revision(s), added %d: %s; removed %d: %s' =>
+        'changed revisions, added %3$s; removed %5$s',
+
     );
   }
 

src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php

@@ -928,6 +928,10 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList {
         'type'    => 'sql',
         'name'    => $this->getPatchPath('phameblog.sql'),
       ),
+      'migrate-maniphest-revisions.php' => array(
+        'type'    => 'php',
+        'name'    => $this->getPatchPath('migrate-maniphest-revisions.php'),
+      ),
     );
   }