From cd080b092e20352e43bc947a4fa2f43b306ca036 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 26 Feb 2014 11:18:06 -0800 Subject: [PATCH] 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 --- .../customfield/DifferentialSummaryField.php | 11 +++ .../customfield/DifferentialTestPlanField.php | 12 ++++ .../search/DifferentialSearchIndexer.php | 72 +++++-------------- .../search/ManiphestSearchIndexer.php | 2 - .../search/PhabricatorUserSearchIndexer.php | 2 - .../ponder/search/PonderSearchIndexer.php | 2 - .../PhabricatorProjectSearchIndexer.php | 3 - .../PhabricatorSearchDocumentIndexer.php | 26 +++++-- .../field/PhabricatorCustomField.php | 50 ++++++++++--- .../field/PhabricatorCustomFieldList.php | 19 ++++- 10 files changed, 118 insertions(+), 81 deletions(-) diff --git a/src/applications/differential/customfield/DifferentialSummaryField.php b/src/applications/differential/customfield/DifferentialSummaryField.php index 9db2651f6f..4d95a56eb3 100644 --- a/src/applications/differential/customfield/DifferentialSummaryField.php +++ b/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()); + } + } + } diff --git a/src/applications/differential/customfield/DifferentialTestPlanField.php b/src/applications/differential/customfield/DifferentialTestPlanField.php index ba22675c7c..3c4dbdadc5 100644 --- a/src/applications/differential/customfield/DifferentialTestPlanField.php +++ b/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()); + } + } + } diff --git a/src/applications/differential/search/DifferentialSearchIndexer.php b/src/applications/differential/search/DifferentialSearchIndexer.php index 2dee73de54..9bc27402fe 100644 --- a/src/applications/differential/search/DifferentialSearchIndexer.php +++ b/src/applications/differential/search/DifferentialSearchIndexer.php @@ -1,8 +1,5 @@ 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; } } diff --git a/src/applications/maniphest/search/ManiphestSearchIndexer.php b/src/applications/maniphest/search/ManiphestSearchIndexer.php index 971488f64c..41e699d762 100644 --- a/src/applications/maniphest/search/ManiphestSearchIndexer.php +++ b/src/applications/maniphest/search/ManiphestSearchIndexer.php @@ -81,8 +81,6 @@ final class ManiphestSearchIndexer time()); } - $this->indexCustomFields($doc, $task); - return $doc; } } diff --git a/src/applications/people/search/PhabricatorUserSearchIndexer.php b/src/applications/people/search/PhabricatorUserSearchIndexer.php index 8d74334ee9..bb0a2a337c 100644 --- a/src/applications/people/search/PhabricatorUserSearchIndexer.php +++ b/src/applications/people/search/PhabricatorUserSearchIndexer.php @@ -25,8 +25,6 @@ final class PhabricatorUserSearchIndexer PhabricatorPeoplePHIDTypeUser::TYPECONST, time()); - $this->indexCustomFields($doc, $user); - return $doc; } } diff --git a/src/applications/ponder/search/PonderSearchIndexer.php b/src/applications/ponder/search/PonderSearchIndexer.php index 854d4fd9d7..82cc13dc9a 100644 --- a/src/applications/ponder/search/PonderSearchIndexer.php +++ b/src/applications/ponder/search/PonderSearchIndexer.php @@ -46,8 +46,6 @@ final class PonderSearchIndexer new PonderAnswerTransactionQuery(), mpull($answers, 'getPHID')); - $this->indexSubscribers($doc); - return $doc; } } diff --git a/src/applications/project/search/PhabricatorProjectSearchIndexer.php b/src/applications/project/search/PhabricatorProjectSearchIndexer.php index a5c8d5dca3..17a0ac7be4 100644 --- a/src/applications/project/search/PhabricatorProjectSearchIndexer.php +++ b/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 diff --git a/src/applications/search/index/PhabricatorSearchDocumentIndexer.php b/src/applications/search/index/PhabricatorSearchDocumentIndexer.php index 1965c55809..4781a8fdd2 100644 --- a/src/applications/search/index/PhabricatorSearchDocumentIndexer.php +++ b/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( diff --git a/src/infrastructure/customfield/field/PhabricatorCustomField.php b/src/infrastructure/customfield/field/PhabricatorCustomField.php index 133bf382b8..3cd46b0690 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomField.php +++ b/src/infrastructure/customfield/field/PhabricatorCustomField.php @@ -1,16 +1,17 @@ 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; + } + + } diff --git a/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php b/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php index ffe96bc285..dae4bd507a 100644 --- a/src/infrastructure/customfield/field/PhabricatorCustomFieldList.php +++ b/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; + } + + }