Lift Conpherence indexing up out of the Fulltext index

Summary:
Ref T9979. There are currently some hacks around Conpherence indexing: it does not really use the fulltext index, but its own specialized index. However, it's kind of hacked up so it can get reindexed by the normal indexing pipeline.

Lift it up into IndexEngine, instead of FulltextEngine. Specifically, the new stuff is going to look like this:

  - IndexEngine: Rebuild all indexes.
    - ConpherenceIndexExtension: Rebuild thread indexes.
    - ProjectMemberIndexExtension: Rebuild project membership views.
    - NgramIndexExtension: Rebuild ngram indexes.
    - FulltextIndexExtension / FulltextEngine: Rebuild fulltext indexes, a special type of index.
      - FulltextCommentExtension: Rebuild comment fulltext indexes.
      - FulltextProjectExtension: Rebuild project fulltext indexes.
      - etc.

Most of this is at least sort-of-in-place as of this diff, although some of the part in the middle is still pretty rough.

Test Plan:
  - Made a unique comment in a Conpherence thread.
  - Used `bin/search index --force` to rebuild the index.
  - Searched for the comment.
  - Found the thread.

Reviewers: chad

Reviewed By: chad

Maniphest Tasks: T9979

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

src/__phutil_library_map__.php

@@ -285,7 +285,7 @@ phutil_register_library_map(array(
     'ConpherenceSettings' => 'applications/conpherence/constants/ConpherenceSettings.php',
     'ConpherenceTestCase' => 'applications/conpherence/__tests__/ConpherenceTestCase.php',
     'ConpherenceThread' => 'applications/conpherence/storage/ConpherenceThread.php',
-    'ConpherenceThreadIndexer' => 'applications/conpherence/search/ConpherenceThreadIndexer.php',
+    'ConpherenceThreadIndexEngineExtension' => 'applications/conpherence/engineextension/ConpherenceThreadIndexEngineExtension.php',
     'ConpherenceThreadListView' => 'applications/conpherence/view/ConpherenceThreadListView.php',
     'ConpherenceThreadMailReceiver' => 'applications/conpherence/mail/ConpherenceThreadMailReceiver.php',
     'ConpherenceThreadMembersPolicyRule' => 'applications/conpherence/policyrule/ConpherenceThreadMembersPolicyRule.php',
@@ -2378,6 +2378,8 @@ phutil_register_library_map(array(
     'PhabricatorImageTransformer' => 'applications/files/PhabricatorImageTransformer.php',
     'PhabricatorImagemagickSetupCheck' => 'applications/config/check/PhabricatorImagemagickSetupCheck.php',
     'PhabricatorIndexEngine' => 'applications/search/index/PhabricatorIndexEngine.php',
+    'PhabricatorIndexEngineExtension' => 'applications/search/index/PhabricatorIndexEngineExtension.php',
+    'PhabricatorIndexEngineExtensionModule' => 'applications/search/index/PhabricatorIndexEngineExtensionModule.php',
     'PhabricatorInfrastructureTestCase' => '__tests__/PhabricatorInfrastructureTestCase.php',
     'PhabricatorInlineCommentController' => 'infrastructure/diff/PhabricatorInlineCommentController.php',
     'PhabricatorInlineCommentInterface' => 'infrastructure/diff/interface/PhabricatorInlineCommentInterface.php',
@@ -4181,7 +4183,7 @@ phutil_register_library_map(array(
       'PhabricatorMentionableInterface',
       'PhabricatorDestructibleInterface',
     ),
-    'ConpherenceThreadIndexer' => 'PhabricatorSearchDocumentIndexer',
+    'ConpherenceThreadIndexEngineExtension' => 'PhabricatorIndexEngineExtension',
     'ConpherenceThreadListView' => 'AphrontView',
     'ConpherenceThreadMailReceiver' => 'PhabricatorObjectMailReceiver',
     'ConpherenceThreadMembersPolicyRule' => 'PhabricatorPolicyRule',
@@ -6606,6 +6608,8 @@ phutil_register_library_map(array(
     'PhabricatorImageTransformer' => 'Phobject',
     'PhabricatorImagemagickSetupCheck' => 'PhabricatorSetupCheck',
     'PhabricatorIndexEngine' => 'Phobject',
+    'PhabricatorIndexEngineExtension' => 'Phobject',
+    'PhabricatorIndexEngineExtensionModule' => 'PhabricatorConfigModule',
     'PhabricatorInfrastructureTestCase' => 'PhabricatorTestCase',
     'PhabricatorInlineCommentController' => 'PhabricatorController',
     'PhabricatorInlineCommentInterface' => 'PhabricatorMarkupInterface',

src/applications/conpherence/editor/ConpherenceEditor.php

@@ -601,22 +601,6 @@ final class ConpherenceEditor extends PhabricatorApplicationTransactionEditor {
     return true;
   }
 
-  protected function getSearchContextParameter(
-    PhabricatorLiskDAO $object,
-    array $xactions) {
-
-    $comment_phids = array();
-    foreach ($xactions as $xaction) {
-      if ($xaction->hasComment()) {
-        $comment_phids[] = $xaction->getPHID();
-      }
-    }
-
-    return array(
-      'commentPHIDs' => $comment_phids,
-    );
-  }
-
   protected function extractFilePHIDsFromCustomTransaction(
     PhabricatorLiskDAO $object,
     PhabricatorApplicationTransaction $xaction) {

src/applications/conpherence/engineextension/ConpherenceThreadIndexEngineExtension.php

@@ -1,58 +1,50 @@
 <?php
 
-final class ConpherenceThreadIndexer
+final class ConpherenceThreadIndexEngineExtension
-  extends PhabricatorSearchDocumentIndexer {
+  extends PhabricatorIndexEngineExtension {
 
-  public function getIndexableObject() {
+  const EXTENSIONKEY = 'conpherence.thread';
-    return new ConpherenceThread();
+
+  public function getExtensionName() {
+    return pht('Conpherence Threads');
   }
 
-  protected function loadDocumentByPHID($phid) {
+  public function shouldIndexObject($object) {
-    $object = id(new ConpherenceThreadQuery())
+    return ($object instanceof ConpherenceThread);
-      ->setViewer($this->getViewer())
-      ->withPHIDs(array($phid))
-      ->executeOne();
-
-    if (!$object) {
-      throw new Exception(pht('No thread "%s" exists!', $phid));
-    }
-
-    return $object;
   }
 
-  protected function buildAbstractDocumentByPHID($phid) {
+  public function indexObject(
-    $thread = $this->loadDocumentByPHID($phid);
+    PhabricatorIndexEngine $engine,
+    $object) {
 
-    // NOTE: We're explicitly not building a document here, only rebuilding
+    $force = $this->shouldForceFullReindex();
-    // the Conpherence search index.
 
-    $context = nonempty($this->getContext(), array());
+    if (!$force) {
-    $comment_phids = idx($context, 'commentPHIDs');
+      $xaction_phids = $this->getParameter('transactionPHIDs');
-
+      if (!$xaction_phids) {
-    if (is_array($comment_phids) && !$comment_phids) {
+        return;
-      // If this property is set, but empty, the transaction did not
+      }
-      // include any chat text. For example, a user might have left the
-      // conversation.
-      return null;
     }
 
     $query = id(new ConpherenceTransactionQuery())
       ->setViewer($this->getViewer())
-      ->withObjectPHIDs(array($thread->getPHID()))
+      ->withObjectPHIDs(array($object->getPHID()))
       ->withTransactionTypes(array(PhabricatorTransactions::TYPE_COMMENT))
       ->needComments(true);
 
-    if ($comment_phids !== null) {
+    if (!$force) {
-      $query->withPHIDs($comment_phids);
+      $query->withPHIDs($xaction_phids);
     }
 
     $xactions = $query->execute();
 
-    foreach ($xactions as $xaction) {
+    if (!$xactions) {
-      $this->indexComment($thread, $xaction);
+      return;
     }
 
-    return null;
+    foreach ($xactions as $xaction) {
+      $this->indexComment($object, $xaction);
+    }
   }
 
   private function indexComment(

src/applications/search/index/PhabricatorIndexEngine.php

@@ -2,14 +2,108 @@
 
 final class PhabricatorIndexEngine extends Phobject {
 
-  public function indexDocumentByPHID($phid, $context) {
+  private $object;
+  private $extensions;
+  private $versions;
+  private $parameters;
+
+  public function setParameters(array $parameters) {
+    $this->parameters = $parameters;
+    return $this;
+  }
+
+  public function getParameters() {
+    return $this->parameters;
+  }
+
+  public function setObject($object) {
+    $this->object = $object;
+    return $this;
+  }
+
+  public function getObject() {
+    return $this->object;
+  }
+
+  public function shouldIndexObject() {
+    $extensions = $this->newExtensions();
+
+    $parameters = $this->getParameters();
+    foreach ($extensions as $extension) {
+      $extension->setParameters($parameters);
+    }
+
+    $object = $this->getObject();
+    $versions = array();
+    foreach ($extensions as $key => $extension) {
+      $version = $extension->getIndexVersion($object);
+      if ($version !== null) {
+        $versions[$key] = (string)$version;
+      }
+    }
+
+    if (idx($parameters, 'force')) {
+      $current_versions = array();
+    } else {
+      // TODO: Load current indexed versions.
+      $current_versions = array();
+    }
+
+    foreach ($versions as $key => $version) {
+      $current_version = idx($current_versions, $key);
+
+      if ($current_version === null) {
+        continue;
+      }
+
+      // If nothing has changed since we built the current index, we do not
+      // need to rebuild the index.
+      if ($current_version === $version) {
+        unset($extensions[$key]);
+      }
+    }
+
+    $this->extensions = $extensions;
+    $this->versions = $versions;
+
+    // We should index the object only if there is any work to be done.
+    return (bool)$this->extensions;
+  }
+
+  public function indexObject() {
+    $extensions = $this->extensions;
+    $object = $this->getObject();
+
+    foreach ($extensions as $key => $extension) {
+      $extension->indexObject($this, $object);
+    }
+
+    // TODO: Save new index versions.
+
+    return $this;
+  }
+
+  private function newExtensions() {
+    $object = $this->getObject();
+
+    $extensions = PhabricatorIndexEngineExtension::getAllExtensions();
+    foreach ($extensions as $key => $extension) {
+      if (!$extension->shouldIndexObject($object)) {
+        unset($extensions[$key]);
+      }
+    }
+
+    return $extensions;
+  }
+
+  public function indexDocumentByPHID($phid) {
     $indexers = id(new PhutilClassMapQuery())
       ->setAncestorClass('PhabricatorSearchDocumentIndexer')
       ->execute();
 
     foreach ($indexers as $indexer) {
       if ($indexer->shouldIndexDocumentByPHID($phid)) {
-        $indexer->indexDocumentByPHID($phid, $context);
+        $indexer->indexDocumentByPHID($phid);
         break;
       }
     }

src/applications/search/index/PhabricatorIndexEngineExtension.php

@@ -0,0 +1,48 @@
+<?php
+
+abstract class PhabricatorIndexEngineExtension extends Phobject {
+
+  private $parameters;
+  private $forceFullReindex;
+
+  public function setParameters(array $parameters) {
+    $this->parameters = $parameters;
+    return $this;
+  }
+
+  public function getParameter($key, $default = null) {
+    return idx($this->parameters, $key, $default);
+  }
+
+  final public function getExtensionKey() {
+    return $this->getPhobjectClassConstant('EXTENSIONKEY');
+  }
+
+  final protected function getViewer() {
+    return PhabricatorUser::getOmnipotentUser();
+  }
+
+  abstract public function getExtensionName();
+
+  abstract public function shouldIndexObject($object);
+
+  abstract public function indexObject(
+    PhabricatorIndexEngine $engine,
+    $object);
+
+  public function getIndexVersion($object) {
+    return null;
+  }
+
+  final public static function getAllExtensions() {
+    return id(new PhutilClassMapQuery())
+      ->setAncestorClass(__CLASS__)
+      ->setUniqueMethod('getExtensionKey')
+      ->execute();
+  }
+
+  final public function shouldForceFullReindex() {
+    return $this->getParameter('force');
+  }
+
+}

src/applications/search/index/PhabricatorIndexEngineExtensionModule.php

@@ -0,0 +1,44 @@
+<?php
+
+final class PhabricatorIndexEngineExtensionModule
+  extends PhabricatorConfigModule {
+
+  public function getModuleKey() {
+    return 'indexengine';
+  }
+
+  public function getModuleName() {
+    return pht('Engine: Index');
+  }
+
+  public function renderModuleStatus(AphrontRequest $request) {
+    $viewer = $request->getViewer();
+
+    $extensions = PhabricatorIndexEngineExtension::getAllExtensions();
+
+    $rows = array();
+    foreach ($extensions as $extension) {
+      $rows[] = array(
+        get_class($extension),
+        $extension->getExtensionName(),
+      );
+    }
+
+    $table = id(new AphrontTableView($rows))
+      ->setHeaders(
+        array(
+          pht('Class'),
+          pht('Name'),
+        ))
+      ->setColumnClasses(
+        array(
+          null,
+          'wide pri',
+        ));
+
+    return id(new PHUIObjectBoxView())
+      ->setHeaderText(pht('IndexEngine Extensions'))
+      ->setTable($table);
+  }
+
+}

src/applications/search/index/PhabricatorSearchDocumentIndexer.php

@@ -2,17 +2,6 @@
 
 abstract class PhabricatorSearchDocumentIndexer extends Phobject {
 
-  private $context;
-
-  protected function setContext($context) {
-    $this->context = $context;
-    return $this;
-  }
-
-  protected function getContext() {
-    return $this->context;
-  }
-
   abstract public function getIndexableObject();
   abstract protected function buildAbstractDocumentByPHID($phid);
 
@@ -41,9 +30,7 @@ abstract class PhabricatorSearchDocumentIndexer extends Phobject {
     return $object;
   }
 
-  public function indexDocumentByPHID($phid, $context) {
+  public function indexDocumentByPHID($phid) {
-    $this->setContext($context);
-
     $document = $this->buildAbstractDocumentByPHID($phid);
     if ($document === null) {
       // This indexer doesn't build a document index, so we're done.

src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php

@@ -29,6 +29,13 @@ final class PhabricatorSearchManagementIndexWorkflow
               'the daemons. This can improve performance, but makes '.
               'it more difficult to debug search indexing.'),
           ),
+          array(
+            'name' => 'force',
+            'short' => 'f',
+            'help' => pht(
+              'Force a complete rebuild of the entire index instead of an '.
+              'incremental update.'),
+          ),
           array(
             'name'      => 'objects',
             'wildcard'  => true,
@@ -41,6 +48,7 @@ final class PhabricatorSearchManagementIndexWorkflow
 
     $is_all = $args->getArg('all');
     $is_type = $args->getArg('type');
+    $is_force = $args->getArg('force');
 
     $obj_names = $args->getArg('objects');
 
@@ -93,10 +101,14 @@ final class PhabricatorSearchManagementIndexWorkflow
     $bar = id(new PhutilConsoleProgressBar())
       ->setTotal(count($phids));
 
+    $parameters = array(
+      'force' => $is_force,
+    );
+
     $any_success = false;
     foreach ($phids as $phid) {
       try {
-        PhabricatorSearchWorker::queueDocumentForIndexing($phid);
+        PhabricatorSearchWorker::queueDocumentForIndexing($phid, $parameters);
         $any_success = true;
       } catch (Exception $ex) {
         phlog($ex);

src/applications/search/worker/PhabricatorSearchWorker.php

@@ -2,12 +2,16 @@
 
 final class PhabricatorSearchWorker extends PhabricatorWorker {
 
-  public static function queueDocumentForIndexing($phid, $context = null) {
+  public static function queueDocumentForIndexing($phid, $parameters = null) {
+    if ($parameters === null) {
+      $parameters = array();
+    }
+
     parent::scheduleTask(
       __CLASS__,
       array(
         'documentPHID' => $phid,
-        'context' => $context,
+        'parameters' => $parameters,
       ),
       array(
         'priority' => parent::PRIORITY_IMPORT,
@@ -17,9 +21,18 @@ final class PhabricatorSearchWorker extends PhabricatorWorker {
   protected function doWork() {
     $data = $this->getTaskData();
     $object_phid = idx($data, 'documentPHID');
-    $context = idx($data, 'context');
 
-    $engine = new PhabricatorIndexEngine();
+    $object = $this->loadObjectForIndexing($object_phid);
+
+    $engine = id(new PhabricatorIndexEngine())
+      ->setObject($object);
+
+    $parameters = idx($data, 'parameters', array());
+    $engine->setParameters($parameters);
+
+    if (!$engine->shouldIndexObject()) {
+      return;
+    }
 
     $key = "index.{$object_phid}";
     $lock = PhabricatorGlobalLock::newLock($key);
@@ -27,10 +40,15 @@ final class PhabricatorSearchWorker extends PhabricatorWorker {
     $lock->lock(1);
 
     try {
-      $object = $this->loadObjectForIndexing($object_phid);
+      // Reload the object now that we have a lock, to make sure we have the
+      // most current version.
+      $object = $this->loadObjectForIndexing($object->getPHID());
 
-      $engine->indexDocumentByPHID($object->getPHID(), $context);
+      $engine->setObject($object);
 
+      $engine->indexObject();
+
+      $engine->indexDocumentByPHID($object->getPHID());
     } catch (Exception $ex) {
       $lock->unlock();
 

src/applications/transactions/editor/PhabricatorApplicationTransactionEditor.php

@@ -1095,7 +1095,9 @@ abstract class PhabricatorApplicationTransactionEditor
     if ($this->supportsSearch()) {
       PhabricatorSearchWorker::queueDocumentForIndexing(
         $object->getPHID(),
-        $this->getSearchContextParameter($object, $xactions));
+        array(
+          'transactionPHIDs' => mpull($xactions, 'getPHID'),
+        ));
     }
 
     if ($this->shouldPublishFeedStory($object, $xactions)) {
@@ -2862,15 +2864,6 @@ abstract class PhabricatorApplicationTransactionEditor
     return false;
   }
 
-  /**
-   * @task search
-   */
-  protected function getSearchContextParameter(
-    PhabricatorLiskDAO $object,
-    array $xactions) {
-    return null;
-  }
-
 
 /* -(  Herald Integration )-------------------------------------------------- */