Use ApplicationTransactions/CustomField to power Differential global search

Summary:
Ref T2222. Ref T3886. Ref T418. A few changes:

  - CustomField can now index into global search.
  - Use CustomField fields instead of older custom fields for Differential global search. (This slightly breaks any custom fields which exist, but they are presumably very rare, and probably do not exist; this break is also very mild.)
  - Automatically perform CustomField and Subscribable indexing on applicable object types.

Test Plan: Used `bin/search index` to reindex a bunch of stuff, then searched for it. Debug-dumped abstract documents to inspect them.

Reviewers: btrahan

Reviewed By: btrahan

CC: aran

Maniphest Tasks: T418, T3886, T2222

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

src/applications/differential/customfield/DifferentialSummaryField.php

@@ -78,4 +78,15 @@ final class DifferentialSummaryField
       $xaction->getNewValue());
   }
 
+  public function shouldAppearInGlobalSearch() {
+    return true;
+  }
+
+  public function updateAbstractDocument(
+    PhabricatorSearchAbstractDocument $document) {
+    if (strlen($this->getValue())) {
+      $document->addField('body', $this->getValue());
+    }
+  }
+
 }

src/applications/differential/customfield/DifferentialTestPlanField.php

@@ -92,4 +92,16 @@ final class DifferentialTestPlanField
       $xaction->getNewValue());
   }
 
+
+  public function shouldAppearInGlobalSearch() {
+    return true;
+  }
+
+  public function updateAbstractDocument(
+    PhabricatorSearchAbstractDocument $document) {
+    if (strlen($this->getValue())) {
+      $document->addField('plan', $this->getValue());
+    }
+  }
+
 }

src/applications/differential/search/DifferentialSearchIndexer.php

@@ -1,8 +1,5 @@
 <?php
 
-/**
- * @group differential
- */
 final class DifferentialSearchIndexer
   extends PhabricatorSearchDocumentIndexer {
 
@@ -10,6 +7,18 @@ final class DifferentialSearchIndexer
     return new DifferentialRevision();
   }
 
+  protected function loadDocumentByPHID($phid) {
+    $object = id(new DifferentialRevisionQuery())
+      ->setViewer($this->getViewer())
+      ->withPHIDs(array($phid))
+      ->needReviewerStatus(true)
+      ->executeOne();
+    if (!$object) {
+      throw new Exception("Unable to load object by phid '{$phid}'!");
+    }
+    return $object;
+  }
+
   protected function buildAbstractDocumentByPHID($phid) {
     $rev = $this->loadDocumentByPHID($phid);
 
@@ -20,24 +29,6 @@ final class DifferentialSearchIndexer
     $doc->setDocumentCreated($rev->getDateCreated());
     $doc->setDocumentModified($rev->getDateModified());
 
-    $aux_fields = DifferentialFieldSelector::newSelector()
-      ->getFieldSpecifications();
-    foreach ($aux_fields as $key => $aux_field) {
-      $aux_field->setUser(PhabricatorUser::getOmnipotentUser());
-      if (!$aux_field->shouldAddToSearchIndex()) {
-        unset($aux_fields[$key]);
-      }
-    }
-
-    $aux_fields = DifferentialAuxiliaryField::loadFromStorage(
-      $rev,
-      $aux_fields);
-    foreach ($aux_fields as $aux_field) {
-      $doc->addField(
-        $aux_field->getKeyForSearchIndex(),
-        $aux_field->getValueForSearchIndex());
-    }
-
     $doc->addRelationship(
       PhabricatorSearchRelationship::RELATIONSHIP_AUTHOR,
       $rev->getAuthorPHID(),
@@ -52,29 +43,16 @@ final class DifferentialSearchIndexer
       DifferentialPHIDTypeRevision::TYPECONST,
       time());
 
-    $comments = id(new DifferentialCommentQuery())
-      ->withRevisionPHIDs(array($rev->getPHID()))
-      ->execute();
-
-    $inlines = id(new DifferentialInlineCommentQuery())
-      ->withRevisionIDs(array($rev->getID()))
-      ->withNotDraft(true)
-      ->execute();
-
-    foreach (array_merge($comments, $inlines) as $comment) {
-      if (strlen($comment->getContent())) {
-        $doc->addField(
-          PhabricatorSearchField::FIELD_COMMENT,
-          $comment->getContent());
-      }
-    }
-
-    $rev->loadRelationships();
+    $this->indexTransactions(
+      $doc,
+      new DifferentialTransactionQuery(),
+      array($rev->getPHID()));
 
     // If a revision needs review, the owners are the reviewers. Otherwise, the
     // owner is the author (e.g., accepted, rejected, closed).
     if ($rev->getStatus() == ArcanistDifferentialRevisionStatus::NEEDS_REVIEW) {
-      $reviewers = $rev->getReviewers();
+      $reviewers = $rev->getReviewerStatus();
+      $reviewers = mpull($reviewers, 'getReviewerPHID', 'getReviewerPHID');
       if ($reviewers) {
         foreach ($reviewers as $phid) {
           $doc->addRelationship(
@@ -98,20 +76,6 @@ final class DifferentialSearchIndexer
         $rev->getDateCreated());
     }
 
-    $ccphids = $rev->getCCPHIDs();
-    $handles = id(new PhabricatorHandleQuery())
-      ->setViewer(PhabricatorUser::getOmnipotentUser())
-      ->withPHIDs($ccphids)
-      ->execute();
-
-    foreach ($handles as $phid => $handle) {
-      $doc->addRelationship(
-        PhabricatorSearchRelationship::RELATIONSHIP_SUBSCRIBER,
-        $phid,
-        $handle->getType(),
-        $rev->getDateModified()); // Bogus timestamp.
-    }
-
     return $doc;
   }
 }

src/applications/maniphest/search/ManiphestSearchIndexer.php

@@ -81,8 +81,6 @@ final class ManiphestSearchIndexer
         time());
     }
 
-    $this->indexCustomFields($doc, $task);
-
     return $doc;
   }
 }

src/applications/people/search/PhabricatorUserSearchIndexer.php

@@ -25,8 +25,6 @@ final class PhabricatorUserSearchIndexer
       PhabricatorPeoplePHIDTypeUser::TYPECONST,
       time());
 
-    $this->indexCustomFields($doc, $user);
-
     return $doc;
   }
 }

src/applications/ponder/search/PonderSearchIndexer.php

@@ -46,8 +46,6 @@ final class PonderSearchIndexer
       new PonderAnswerTransactionQuery(),
       mpull($answers, 'getPHID'));
 
-    $this->indexSubscribers($doc);
-
     return $doc;
   }
 }

src/applications/project/search/PhabricatorProjectSearchIndexer.php

@@ -17,9 +17,6 @@ final class PhabricatorProjectSearchIndexer
     $doc->setDocumentCreated($project->getDateCreated());
     $doc->setDocumentModified($project->getDateModified());
 
-    $this->indexSubscribers($doc);
-    $this->indexCustomFields($doc, $project);
-
     $doc->addRelationship(
       $project->isArchived()
         ? PhabricatorSearchRelationship::RELATIONSHIP_CLOSED

src/applications/search/index/PhabricatorSearchDocumentIndexer.php

@@ -37,6 +37,19 @@ abstract class PhabricatorSearchDocumentIndexer {
     try {
       $document = $this->buildAbstractDocumentByPHID($phid);
 
+      $object = $this->loadDocumentByPHID($phid);
+
+      // Automatically rebuild CustomField indexes if the object uses custom
+      // fields.
+      if ($object instanceof PhabricatorCustomFieldInterface) {
+        $this->indexCustomFields($document, $object);
+      }
+
+      // Automatically rebuild subscriber indexes if the object is subscribable.
+      if ($object instanceof PhabricatorSubscribableInterface) {
+        $this->indexSubscribers($document);
+      }
+
       $engine = PhabricatorSearchEngineSelector::newSelector()->newEngine();
       try {
         $engine->reindexAbstractDocument($document);
@@ -107,7 +120,7 @@ abstract class PhabricatorSearchDocumentIndexer {
   }
 
   protected function indexCustomFields(
-    PhabricatorSearchAbstractDocument $doc,
+    PhabricatorSearchAbstractDocument $document,
     PhabricatorCustomFieldInterface $object) {
 
     // Rebuild the ApplicationSearch indexes. These are internal and not part of
@@ -116,17 +129,16 @@ abstract class PhabricatorSearchDocumentIndexer {
 
     $field_list = PhabricatorCustomField::getObjectFields(
       $object,
-      PhabricatorCustomField::ROLE_APPLICATIONSEARCH);
+      PhabricatorCustomField::ROLE_DEFAULT);
 
     $field_list->setViewer($this->getViewer());
     $field_list->readFieldsFromStorage($object);
+
+    // Rebuild ApplicationSearch indexes.
     $field_list->rebuildIndexes($object);
 
-    // We could also allow fields to provide fulltext content, and index it
-    // here on the document. No one has asked for this yet, though, and the
-    // existing "search" key isn't a good fit to interpret to mean we should
-    // index stuff here, since it can be set on a lot of fields which don't
-    // contain anything resembling fulltext.
+    // Rebuild global search indexes.
+    $field_list->updateAbstractDocument($document);
   }
 
   private function dispatchDidUpdateIndexEvent(

src/infrastructure/customfield/field/PhabricatorCustomField.php

@@ -1,16 +1,17 @@
 <?php
 
 /**
- * @task apps       Building Applications with Custom Fields
- * @task core       Core Properties and Field Identity
- * @task proxy      Field Proxies
- * @task context    Contextual Data
- * @task storage    Field Storage
- * @task appsearch  Integration with ApplicationSearch
- * @task appxaction Integration with ApplicationTransactions
- * @task edit       Integration with edit views
- * @task view       Integration with property views
- * @task list       Integration with list views
+ * @task apps         Building Applications with Custom Fields
+ * @task core         Core Properties and Field Identity
+ * @task proxy        Field Proxies
+ * @task context      Contextual Data
+ * @task storage      Field Storage
+ * @task edit         Integration with Edit Views
+ * @task view         Integration with Property Views
+ * @task list         Integration with List views
+ * @task appsearch    Integration with ApplicationSearch
+ * @task appxaction   Integration with ApplicationTransactions
+ * @task globalsearch Integration with Global Search
  */
 abstract class PhabricatorCustomField {
 
@@ -25,6 +26,7 @@ abstract class PhabricatorCustomField {
   const ROLE_EDIT                     = 'edit';
   const ROLE_VIEW                     = 'view';
   const ROLE_LIST                     = 'list';
+  const ROLE_GLOBALSEARCH             = 'GlobalSearch';
 
 
 /* -(  Building Applications with Custom Fields  )--------------------------- */
@@ -253,6 +255,8 @@ abstract class PhabricatorCustomField {
         return $this->shouldAppearInPropertyView();
       case self::ROLE_LIST:
         return $this->shouldAppearInListView();
+      case self::ROLE_GLOBALSEARCH:
+        return $this->shouldAppearInGlobalSearch();
       case self::ROLE_DEFAULT:
         return true;
       default:
@@ -1091,4 +1095,30 @@ abstract class PhabricatorCustomField {
   }
 
 
+/* -(  Global Search  )------------------------------------------------------ */
+
+
+  /**
+   * @task globalsearch
+   */
+  public function shouldAppearInGlobalSearch() {
+    if ($this->proxy) {
+      return $this->proxy->shouldAppearInGlobalSearch();
+    }
+    return false;
+  }
+
+
+  /**
+   * @task globalsearch
+   */
+  public function updateAbstractDocument(
+    PhabricatorSearchAbstractDocument $document) {
+    if ($this->proxy) {
+      return $this->proxy->updateAbstractDocument($document);
+    }
+    return $document;
+  }
+
+
 }

src/infrastructure/customfield/field/PhabricatorCustomFieldList.php

@@ -52,7 +52,7 @@ final class PhabricatorCustomFieldList extends Phobject {
     }
 
     if (!$keys) {
-      return;
+      return $this;
     }
 
     // NOTE: We assume all fields share the same storage. This isn't guaranteed
@@ -79,6 +79,8 @@ final class PhabricatorCustomFieldList extends Phobject {
         $field->setValueFromStorage(null);
       }
     }
+
+    return $this;
   }
 
   public function appendFieldsToForm(AphrontFormView $form) {
@@ -304,4 +306,19 @@ final class PhabricatorCustomFieldList extends Phobject {
     $any_index->saveTransaction();
   }
 
+  public function updateAbstractDocument(
+    PhabricatorSearchAbstractDocument $document) {
+
+    $role = PhabricatorCustomField::ROLE_GLOBALSEARCH;
+    foreach ($this->getFields() as $field) {
+      if (!$field->shouldEnableForRole($role)) {
+        continue;
+      }
+      $field->updateAbstractDocument($document);
+    }
+
+    return $document;
+  }
+
+
 }