From 16a8ed72bddaf459a02ee7cd95f232ad0045c05c Mon Sep 17 00:00:00 2001 From: Joshua Spence Date: Wed, 20 May 2015 06:59:58 +1000 Subject: [PATCH] Modernize search engine selection Summary: Remove the `PhabricatorDefaultSearchEngineSelector` class. This is quite similar to D12053. Test Plan: Went to `/view/PhabricatorSearchApplication/` and saw the storage engine configuration. Set `search.elastic.host` and saw the highlighted storage engine change. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin, epriestley Differential Revision: https://secure.phabricator.com/D12670 --- src/__phutil_library_map__.php | 9 +- ...=> PhabricatorElasticSearchSetupCheck.php} | 13 ++- .../PhabricatorExtraConfigSetupCheck.php | 4 + .../check/PhabricatorMySQLSetupCheck.php | 13 ++- ...atorFilesApplicationStorageEnginePanel.php | 2 - .../maniphest/query/ManiphestTaskQuery.php | 2 +- ...torSearchApplicationStorageEnginePanel.php | 82 +++++++++++++ .../config/PhabricatorSearchConfigOptions.php | 15 --- .../engine/PhabricatorElasticSearchEngine.php | 30 ++++- .../engine/PhabricatorMySQLSearchEngine.php | 22 +++- .../search/engine/PhabricatorSearchEngine.php | 109 +++++++++++++++++- .../PhabricatorSearchDocumentIndexer.php | 2 +- ...habricatorSearchManagementInitWorkflow.php | 2 +- .../query/PhabricatorSearchDocumentQuery.php | 2 +- ...PhabricatorDefaultSearchEngineSelector.php | 19 --- .../PhabricatorSearchEngineSelector.php | 15 --- 16 files changed, 263 insertions(+), 78 deletions(-) rename src/applications/config/check/{PhabricatorElasticSetupCheck.php => PhabricatorElasticSearchSetupCheck.php} (76%) create mode 100644 src/applications/search/applicationpanel/PhabricatorSearchApplicationStorageEnginePanel.php delete mode 100644 src/applications/search/selector/PhabricatorDefaultSearchEngineSelector.php delete mode 100644 src/applications/search/selector/PhabricatorSearchEngineSelector.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0c4b286fe7..79e98ed18a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -1744,7 +1744,6 @@ phutil_register_library_map(array( 'PhabricatorDatabaseSetupCheck' => 'applications/config/check/PhabricatorDatabaseSetupCheck.php', 'PhabricatorDateTimeSettingsPanel' => 'applications/settings/panel/PhabricatorDateTimeSettingsPanel.php', 'PhabricatorDebugController' => 'applications/system/controller/PhabricatorDebugController.php', - 'PhabricatorDefaultSearchEngineSelector' => 'applications/search/selector/PhabricatorDefaultSearchEngineSelector.php', 'PhabricatorDestructibleInterface' => 'applications/system/interface/PhabricatorDestructibleInterface.php', 'PhabricatorDestructionEngine' => 'applications/system/engine/PhabricatorDestructionEngine.php', 'PhabricatorDeveloperConfigOptions' => 'applications/config/option/PhabricatorDeveloperConfigOptions.php', @@ -1776,7 +1775,7 @@ phutil_register_library_map(array( 'PhabricatorEdgeType' => 'infrastructure/edges/type/PhabricatorEdgeType.php', 'PhabricatorEditor' => 'infrastructure/PhabricatorEditor.php', 'PhabricatorElasticSearchEngine' => 'applications/search/engine/PhabricatorElasticSearchEngine.php', - 'PhabricatorElasticSetupCheck' => 'applications/config/check/PhabricatorElasticSetupCheck.php', + 'PhabricatorElasticSearchSetupCheck' => 'applications/config/check/PhabricatorElasticSearchSetupCheck.php', 'PhabricatorEmailAddressesSettingsPanel' => 'applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php', 'PhabricatorEmailFormatSettingsPanel' => 'applications/settings/panel/PhabricatorEmailFormatSettingsPanel.php', 'PhabricatorEmailLoginController' => 'applications/auth/controller/PhabricatorEmailLoginController.php', @@ -2501,6 +2500,7 @@ phutil_register_library_map(array( 'PhabricatorSearchAbstractDocument' => 'applications/search/index/PhabricatorSearchAbstractDocument.php', 'PhabricatorSearchApplication' => 'applications/search/application/PhabricatorSearchApplication.php', 'PhabricatorSearchApplicationSearchEngine' => 'applications/search/query/PhabricatorSearchApplicationSearchEngine.php', + 'PhabricatorSearchApplicationStorageEnginePanel' => 'applications/search/applicationpanel/PhabricatorSearchApplicationStorageEnginePanel.php', 'PhabricatorSearchAttachController' => 'applications/search/controller/PhabricatorSearchAttachController.php', 'PhabricatorSearchBaseController' => 'applications/search/controller/PhabricatorSearchBaseController.php', 'PhabricatorSearchConfigOptions' => 'applications/search/config/PhabricatorSearchConfigOptions.php', @@ -2516,7 +2516,6 @@ phutil_register_library_map(array( 'PhabricatorSearchDocumentTypeDatasource' => 'applications/search/typeahead/PhabricatorSearchDocumentTypeDatasource.php', 'PhabricatorSearchEditController' => 'applications/search/controller/PhabricatorSearchEditController.php', 'PhabricatorSearchEngine' => 'applications/search/engine/PhabricatorSearchEngine.php', - 'PhabricatorSearchEngineSelector' => 'applications/search/selector/PhabricatorSearchEngineSelector.php', 'PhabricatorSearchField' => 'applications/search/constants/PhabricatorSearchField.php', 'PhabricatorSearchHovercardController' => 'applications/search/controller/PhabricatorSearchHovercardController.php', 'PhabricatorSearchIndexer' => 'applications/search/index/PhabricatorSearchIndexer.php', @@ -5140,7 +5139,6 @@ phutil_register_library_map(array( 'PhabricatorDatabaseSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorDateTimeSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorDebugController' => 'PhabricatorController', - 'PhabricatorDefaultSearchEngineSelector' => 'PhabricatorSearchEngineSelector', 'PhabricatorDestructionEngine' => 'Phobject', 'PhabricatorDeveloperConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorDeveloperPreferencesSettingsPanel' => 'PhabricatorSettingsPanel', @@ -5169,7 +5167,7 @@ phutil_register_library_map(array( 'PhabricatorEdgeType' => 'Phobject', 'PhabricatorEditor' => 'Phobject', 'PhabricatorElasticSearchEngine' => 'PhabricatorSearchEngine', - 'PhabricatorElasticSetupCheck' => 'PhabricatorSetupCheck', + 'PhabricatorElasticSearchSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorEmailAddressesSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorEmailFormatSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorEmailLoginController' => 'PhabricatorAuthController', @@ -5973,6 +5971,7 @@ phutil_register_library_map(array( 'PhabricatorScheduleTaskTriggerAction' => 'PhabricatorTriggerAction', 'PhabricatorSearchApplication' => 'PhabricatorApplication', 'PhabricatorSearchApplicationSearchEngine' => 'PhabricatorApplicationSearchEngine', + 'PhabricatorSearchApplicationStorageEnginePanel' => 'PhabricatorApplicationConfigurationPanel', 'PhabricatorSearchAttachController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchBaseController' => 'PhabricatorController', 'PhabricatorSearchConfigOptions' => 'PhabricatorApplicationConfigOptions', diff --git a/src/applications/config/check/PhabricatorElasticSetupCheck.php b/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php similarity index 76% rename from src/applications/config/check/PhabricatorElasticSetupCheck.php rename to src/applications/config/check/PhabricatorElasticSearchSetupCheck.php index 7b60444a88..a573c14490 100644 --- a/src/applications/config/check/PhabricatorElasticSetupCheck.php +++ b/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php @@ -1,14 +1,15 @@ newEngine(); + if ($this->shouldUseElasticSearchEngine()) { + $engine = new PhabricatorElasticSearchEngine(); + if (!$engine->indexExists()) { $summary = pht( 'You enabled Elasticsearch but the index does not exist.'); @@ -40,4 +41,10 @@ final class PhabricatorElasticSetupCheck extends PhabricatorSetupCheck { } } } + + protected function shouldUseElasticSearchEngine() { + $search_engine = PhabricatorSearchEngine::loadEngine(); + return $search_engine instanceof PhabricatorElasticSearchEngine; + } + } diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index a95ec0e1c9..2d69334c47 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -252,6 +252,10 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'style.monospace' => $monospace_reason, 'style.monospace.windows' => $monospace_reason, + + 'search.engine-selector' => pht( + 'Phabricator now automatically discovers available search engines '. + 'at runtime.'), ); return $ancient_config; diff --git a/src/applications/config/check/PhabricatorMySQLSetupCheck.php b/src/applications/config/check/PhabricatorMySQLSetupCheck.php index 6747158b85..a5178cc9ed 100644 --- a/src/applications/config/check/PhabricatorMySQLSetupCheck.php +++ b/src/applications/config/check/PhabricatorMySQLSetupCheck.php @@ -116,7 +116,8 @@ final class PhabricatorMySQLSetupCheck extends PhabricatorSetupCheck { } $stopword_file = self::loadRawConfigValue('ft_stopword_file'); - if (!PhabricatorDefaultSearchEngineSelector::shouldUseElasticSearch()) { + + if ($this->shouldUseMySQLSearchEngine()) { if ($stopword_file === null) { $summary = pht( 'Your version of MySQL does not support configuration of a '. @@ -190,7 +191,7 @@ final class PhabricatorMySQLSetupCheck extends PhabricatorSetupCheck { $min_len = self::loadRawConfigValue('ft_min_word_len'); if ($min_len >= 4) { - if (!PhabricatorDefaultSearchEngineSelector::shouldUseElasticSearch()) { + if ($this->shouldUseMySQLSearchEngine()) { $namespace = PhabricatorEnv::getEnvConfig('storage.default-namespace'); $summary = pht( @@ -235,8 +236,7 @@ final class PhabricatorMySQLSetupCheck extends PhabricatorSetupCheck { $bool_syntax = self::loadRawConfigValue('ft_boolean_syntax'); if ($bool_syntax != ' |-><()~*:""&^') { - if (!PhabricatorDefaultSearchEngineSelector::shouldUseElasticSearch()) { - + if ($this->shouldUseMySQLSearchEngine()) { $summary = pht( 'MySQL is configured to search on fulltext indexes using "OR" by '. 'default. Using "AND" is usually the desired behaviour.'); @@ -340,4 +340,9 @@ final class PhabricatorMySQLSetupCheck extends PhabricatorSetupCheck { } + protected function shouldUseMySQLSearchEngine() { + $search_engine = PhabricatorSearchEngine::loadEngine(); + return $search_engine instanceof PhabricatorMySQLSearchEngine; + } + } diff --git a/src/applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php b/src/applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php index a174102c18..a92f3cf763 100644 --- a/src/applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php +++ b/src/applications/files/applicationpanel/PhabricatorFilesApplicationStorageEnginePanel.php @@ -61,8 +61,6 @@ final class PhabricatorFilesApplicationStorageEnginePanel ); } - $table = - $table = id(new AphrontTableView($rows)) ->setNoDataString(pht('No storage engines available.')) ->setHeaders( diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index 4a6381581b..4da078c004 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -563,7 +563,7 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { $fulltext_query->setParameter('types', array(ManiphestTaskPHIDType::TYPECONST)); - $engine = PhabricatorSearchEngineSelector::newSelector()->newEngine(); + $engine = PhabricatorSearchEngine::loadEngine(); $fulltext_results = $engine->executeSearch($fulltext_query); if (empty($fulltext_results)) { diff --git a/src/applications/search/applicationpanel/PhabricatorSearchApplicationStorageEnginePanel.php b/src/applications/search/applicationpanel/PhabricatorSearchApplicationStorageEnginePanel.php new file mode 100644 index 0000000000..b54a818398 --- /dev/null +++ b/src/applications/search/applicationpanel/PhabricatorSearchApplicationStorageEnginePanel.php @@ -0,0 +1,82 @@ +getViewer(); + $application = $this->getApplication(); + + $active_engine = PhabricatorSearchEngine::loadEngine(); + $engines = PhabricatorSearchEngine::loadAllEngines(); + + $rows = array(); + $rowc = array(); + + foreach ($engines as $key => $engine) { + try { + $index_exists = $engine->indexExists() ? pht('Yes') : pht('No'); + } catch (Exception $ex) { + $index_exists = pht('N/A'); + } + + try { + $index_is_sane = $engine->indexIsSane() ? pht('Yes') : pht('No'); + } catch (Exception $ex) { + $index_is_sane = pht('N/A'); + } + + if ($engine == $active_engine) { + $rowc[] = 'highlighted'; + } else { + $rowc[] = null; + } + + $rows[] = array( + $key, + get_class($engine), + $index_exists, + $index_is_sane, + ); + } + + $table = id(new AphrontTableView($rows)) + ->setNoDataString(pht('No search engines available.')) + ->setHeaders( + array( + pht('Key'), + pht('Class'), + pht('Index Exists'), + pht('Index Is Sane'), + )) + ->setRowClasses($rowc) + ->setColumnClasses( + array( + '', + 'wide', + '', + )); + + $box = id(new PHUIObjectBoxView()) + ->setHeaderText(pht('Search Engines')) + ->appendChild($table); + + return $box; + } + + public function handlePanelRequest( + AphrontRequest $request, + PhabricatorController $controller) { + return new Aphront404Response(); + } + +} diff --git a/src/applications/search/config/PhabricatorSearchConfigOptions.php b/src/applications/search/config/PhabricatorSearchConfigOptions.php index f8cb143b94..453821bc49 100644 --- a/src/applications/search/config/PhabricatorSearchConfigOptions.php +++ b/src/applications/search/config/PhabricatorSearchConfigOptions.php @@ -21,21 +21,6 @@ final class PhabricatorSearchConfigOptions public function getOptions() { return array( - $this->newOption( - 'search.engine-selector', - 'class', - 'PhabricatorDefaultSearchEngineSelector') - ->setBaseClass('PhabricatorSearchEngineSelector') - ->setSummary(pht('Search engine selector.')) - ->setDescription( - pht( - 'Phabricator uses a search engine selector to choose which '. - 'search engine to use when indexing and reconstructing '. - 'documents, and when executing queries. You can override the '. - 'engine selector to provide a new selector class which can '. - 'select some custom engine you implement, if you want to store '. - 'your documents in some search engine which does not have '. - 'default support.')), $this->newOption('search.elastic.host', 'string', null) ->setLocked(true) ->setDescription(pht('Elastic Search host.')) diff --git a/src/applications/search/engine/PhabricatorElasticSearchEngine.php b/src/applications/search/engine/PhabricatorElasticSearchEngine.php index 4b30fb4c66..f6efc17044 100644 --- a/src/applications/search/engine/PhabricatorElasticSearchEngine.php +++ b/src/applications/search/engine/PhabricatorElasticSearchEngine.php @@ -1,13 +1,31 @@ uri = $uri; + return $this; + } + + public function setIndex($index) { $this->index = $index; + return $this; } public function setTimeout($timeout) { @@ -15,6 +33,14 @@ final class PhabricatorElasticSearchEngine extends PhabricatorSearchEngine { return $this; } + public function getURI() { + return $this->uri; + } + + public function getIndex() { + return $this->index; + } + public function getTimeout() { return $this->timeout; } @@ -99,7 +125,7 @@ final class PhabricatorElasticSearchEngine extends PhabricatorSearchEngine { $spec[] = array( 'simple_query_string' => array( 'query' => $query->getParameter('query'), - 'fields' => array( 'field.corpus' ), + 'fields' => array('field.corpus'), ), ); diff --git a/src/applications/search/engine/PhabricatorMySQLSearchEngine.php b/src/applications/search/engine/PhabricatorMySQLSearchEngine.php index 42ed21d2e8..3e006399ea 100644 --- a/src/applications/search/engine/PhabricatorMySQLSearchEngine.php +++ b/src/applications/search/engine/PhabricatorMySQLSearchEngine.php @@ -2,12 +2,24 @@ final class PhabricatorMySQLSearchEngine extends PhabricatorSearchEngine { + public function getEngineIdentifier() { + return 'mysql'; + } + + public function getEnginePriority() { + return 100; + } + + public function isEnabled() { + return true; + } + public function reindexAbstractDocument( PhabricatorSearchAbstractDocument $doc) { $phid = $doc->getPHID(); if (!$phid) { - throw new Exception('Document has no PHID!'); + throw new Exception(pht('Document has no PHID!')); } $store = new PhabricatorSearchDocument(); @@ -31,7 +43,7 @@ final class PhabricatorMySQLSearchEngine extends PhabricatorSearchEngine { queryfx( $conn_w, 'INSERT INTO %T (phid, phidType, field, auxPHID, corpus) '. - ' VALUES (%s, %s, %s, %ns, %s)', + 'VALUES (%s, %s, %s, %ns, %s)', $field_dao->getTableName(), $phid, $doc->getDocumentType(), @@ -63,9 +75,9 @@ final class PhabricatorMySQLSearchEngine extends PhabricatorSearchEngine { if ($sql) { queryfx( $conn_w, - 'INSERT INTO %T'. - ' (phid, relatedPHID, relation, relatedType, relatedTime) '. - ' VALUES %Q', + 'INSERT INTO %T '. + '(phid, relatedPHID, relation, relatedType, relatedTime) '. + 'VALUES %Q', $rship_dao->getTableName(), implode(', ', $sql)); } diff --git a/src/applications/search/engine/PhabricatorSearchEngine.php b/src/applications/search/engine/PhabricatorSearchEngine.php index 1521294b13..0ba8e321d8 100644 --- a/src/applications/search/engine/PhabricatorSearchEngine.php +++ b/src/applications/search/engine/PhabricatorSearchEngine.php @@ -1,6 +1,5 @@ setAncestorClass(__CLASS__) + ->loadObjects(); + + $map = array(); + foreach ($objects as $engine) { + $key = $engine->getEngineIdentifier(); + if (empty($map[$key])) { + $map[$key] = $engine; + } else { + throw new Exception( + pht( + 'Search engines "%s" and "%s" have the same engine identifier '. + '"%s". Each storage engine must have a unique identifier.', + get_class($engine), + get_class($map[$key]), + $key)); + } + } + + $map = msort($map, 'getEnginePriority'); + + $engines = $map; + } + + return $engines; + } + + /** + * @task load + */ + public static function loadActiveEngines() { + $engines = self::loadAllEngines(); + + $active = array(); + foreach ($engines as $key => $engine) { + if (!$engine->isEnabled()) { + continue; + } + + $active[$key] = $engine; + } + + return $active; + } + + public static function loadEngine() { + return head(self::loadActiveEngines()); + } + } diff --git a/src/applications/search/index/PhabricatorSearchDocumentIndexer.php b/src/applications/search/index/PhabricatorSearchDocumentIndexer.php index 4e5089aa8c..18d07765ad 100644 --- a/src/applications/search/index/PhabricatorSearchDocumentIndexer.php +++ b/src/applications/search/index/PhabricatorSearchDocumentIndexer.php @@ -69,7 +69,7 @@ abstract class PhabricatorSearchDocumentIndexer extends Phobject { $this->indexProjects($document, $object); } - $engine = PhabricatorSearchEngineSelector::newSelector()->newEngine(); + $engine = PhabricatorSearchEngine::loadEngine(); try { $engine->reindexAbstractDocument($document); } catch (Exception $ex) { diff --git a/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php index 6e4811dd7f..e4a8ddb693 100644 --- a/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php @@ -13,7 +13,7 @@ final class PhabricatorSearchManagementInitWorkflow public function execute(PhutilArgumentParser $args) { $console = PhutilConsole::getConsole(); - $engine = PhabricatorSearchEngineSelector::newSelector()->newEngine(); + $engine = PhabricatorSearchEngine::loadEngine(); $work_done = false; if (!$engine->indexExists()) { diff --git a/src/applications/search/query/PhabricatorSearchDocumentQuery.php b/src/applications/search/query/PhabricatorSearchDocumentQuery.php index 903d40dd5a..1770f4b046 100644 --- a/src/applications/search/query/PhabricatorSearchDocumentQuery.php +++ b/src/applications/search/query/PhabricatorSearchDocumentQuery.php @@ -74,7 +74,7 @@ final class PhabricatorSearchDocumentQuery ->setParameter('offset', $this->getOffset()) ->setParameter('limit', $this->getRawResultLimit()); - $engine = PhabricatorSearchEngineSelector::newSelector()->newEngine(); + $engine = PhabricatorSearchEngine::loadEngine(); return $engine->executeSearch($query); } diff --git a/src/applications/search/selector/PhabricatorDefaultSearchEngineSelector.php b/src/applications/search/selector/PhabricatorDefaultSearchEngineSelector.php deleted file mode 100644 index 9e06b1182a..0000000000 --- a/src/applications/search/selector/PhabricatorDefaultSearchEngineSelector.php +++ /dev/null @@ -1,19 +0,0 @@ - - } - - abstract public function newEngine(); - - final public static function newSelector() { - return PhabricatorEnv::newObjectFromConfig('search.engine-selector'); - } - -}