From 2cda280cde20cfa23f110f09f45197cb8166fe31 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 25 Mar 2017 04:14:32 -0700 Subject: [PATCH 01/39] Make the default Trigger hibernation 3 minutes instead of 5 seconds The `min()` vs `max()` fix in D17560 meant that the Trigger daemon only hibernates for 5 seconds, so we do a full GC sweep every 5 seconds. This ends up eating a fair amount of CPU for no real benefit. The GC cursors should move to persistent storage, but just bump this default up in the meantime. Auditors: chad --- src/infrastructure/daemon/workers/PhabricatorTriggerDaemon.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/infrastructure/daemon/workers/PhabricatorTriggerDaemon.php b/src/infrastructure/daemon/workers/PhabricatorTriggerDaemon.php index 02ac55a160..9561f3d18a 100644 --- a/src/infrastructure/daemon/workers/PhabricatorTriggerDaemon.php +++ b/src/infrastructure/daemon/workers/PhabricatorTriggerDaemon.php @@ -275,7 +275,7 @@ final class PhabricatorTriggerDaemon * @return int Number of seconds to sleep for. */ private function getSleepDuration() { - $sleep = 5; + $sleep = phutil_units('3 minutes in seconds'); $next_triggers = id(new PhabricatorWorkerTriggerQuery()) ->setViewer($this->getViewer()) From a41d158490c0cd0a0454653473c39f7ad2b5954f Mon Sep 17 00:00:00 2001 From: epriestley Date: Sat, 25 Mar 2017 05:01:32 -0700 Subject: [PATCH 02/39] Only hibernate the Taskmaster after 15 seconds of inactivity Under some workloads, the taskmaster may hibernate and launch more rapidly than it should. Require 15 seconds of inactivity before hibernating. Also hibernate for longer. Auditors: chad --- .../daemon/workers/PhabricatorTaskmasterDaemon.php | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php b/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php index 6cbbd8698e..57a69843a4 100644 --- a/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php +++ b/src/infrastructure/daemon/workers/PhabricatorTaskmasterDaemon.php @@ -44,8 +44,11 @@ final class PhabricatorTaskmasterDaemon extends PhabricatorDaemon { $sleep = 0; } else { - if ($this->shouldHibernate(60)) { - break; + if ($this->getIdleDuration() > 15) { + $hibernate_duration = phutil_units('3 minutes in seconds'); + if ($this->shouldHibernate($hibernate_duration)) { + break; + } } // When there's no work, sleep for one second. The pool will From e41c25de5050d69b720424dadbe3d8680362ceaf Mon Sep 17 00:00:00 2001 From: Mukunda Modell Date: Sun, 26 Mar 2017 08:16:47 +0000 Subject: [PATCH 03/39] Support multiple fulltext search clusters with 'cluster.search' config Summary: The goal is to make fulltext search back-ends more extensible, configurable and robust. When this is finished it will be possible to have multiple search storage back-ends and potentially multiple instances of each. Individual instances can be configured with roles such as 'read', 'write' which control which hosts will receive writes to the index and which hosts will respond to queries. These two roles make it possible to have any combination of: * read-only * write-only * read-write * disabled This 'roles' mechanism is extensible to add new roles should that be needed in the future. In addition to supporting multiple elasticsearch and mysql search instances, this refactors the connection health monitoring infrastructure from PhabricatorDatabaseHealthRecord and utilizes the same system for monitoring the health of elasticsearch nodes. This will allow Wikimedia's phabricator to be redundant across data centers (mysql already is, elasticsearch should be as well). The real-world use-case I have in mind here is writing to two indexes (two elasticsearch clusters in different data centers) but reading from only one. Then toggling the 'read' property when we want to migrate to the other data center (and when we migrate from elasticsearch 2.x to 5.x) Hopefully this is useful in the upstream as well. Remaining TODO: * test cases * documentation Test Plan: (WARNING) This will most likely require the elasticsearch index to be deleted and re-created due to schema changes. Tested with elasticsearch versions 2.4 and 5.2 using the following config: ```lang=json "cluster.search": [ { "type": "elasticsearch", "hosts": [ { "host": "localhost", "roles": { "read": true, "write": true } } ], "port": 9200, "protocol": "http", "path": "/phabricator", "version": 5 }, { "type": "mysql", "roles": { "write": true } } ] Also deployed the same changes to Wikimedia's production Phabricator instance without any issues whatsoever. ``` Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin, epriestley Tags: #elasticsearch, #clusters, #wikimedia Differential Revision: https://secure.phabricator.com/D17384 --- .../20161130.search.02.rebuild.php | 12 +- src/__phutil_library_map__.php | 33 +- .../PhabricatorConfigApplication.php | 1 + .../PhabricatorElasticSearchSetupCheck.php | 113 ++--- .../PhabricatorExtraConfigSetupCheck.php | 8 + .../check/PhabricatorMySQLSetupCheck.php | 9 +- ...abricatorConfigClusterSearchController.php | 129 ++++++ .../PhabricatorConfigController.php | 3 + .../PhabricatorClusterConfigOptions.php | 26 ++ .../maniphest/query/ManiphestTaskQuery.php | 8 +- .../PhabricatorProjectFulltextEngine.php | 10 +- .../config/PhabricatorSearchConfigOptions.php | 35 -- .../PhabricatorSearchDocumentFieldType.php | 1 + .../PhabricatorSearchEngineTestCase.php | 4 +- ...habricatorElasticFulltextStorageEngine.php | 413 +++++++++++------- .../PhabricatorElasticSearchQueryBuilder.php | 78 ++++ .../PhabricatorFulltextStorageEngine.php | 98 ++--- .../PhabricatorMySQLFulltextStorageEngine.php | 13 +- .../index/PhabricatorFulltextEngine.php | 3 +- ...habricatorSearchManagementInitWorkflow.php | 50 ++- .../query/PhabricatorSearchDocumentQuery.php | 5 +- src/docs/user/cluster/cluster_search.diviner | 76 ++++ ...PhabricatorClusterServiceHealthRecord.php} | 22 +- .../cluster/PhabricatorDatabaseRef.php | 12 +- ...icatorClusterDatabasesConfigOptionType.php | 0 ...abricatorClusterSearchConfigOptionType.php | 79 ++++ .../PhabricatorClusterException.php | 0 .../PhabricatorClusterExceptionHandler.php | 0 ...ricatorClusterImpossibleWriteException.php | 0 ...abricatorClusterImproperWriteException.php | 0 ...abricatorClusterNoHostForRoleException.php | 10 + .../PhabricatorClusterStrandedException.php | 0 .../search/PhabricatorElasticSearchHost.php | 82 ++++ .../search/PhabricatorMySQLSearchHost.php | 34 ++ .../cluster/search/PhabricatorSearchHost.php | 163 +++++++ .../search/PhabricatorSearchService.php | 259 +++++++++++ 36 files changed, 1411 insertions(+), 378 deletions(-) create mode 100644 src/applications/config/controller/PhabricatorConfigClusterSearchController.php delete mode 100644 src/applications/search/config/PhabricatorSearchConfigOptions.php create mode 100644 src/applications/search/fulltextstorage/PhabricatorElasticSearchQueryBuilder.php create mode 100644 src/docs/user/cluster/cluster_search.diviner rename src/infrastructure/cluster/{PhabricatorDatabaseHealthRecord.php => PhabricatorClusterServiceHealthRecord.php} (89%) rename src/infrastructure/cluster/{ => config}/PhabricatorClusterDatabasesConfigOptionType.php (100%) create mode 100644 src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php rename src/infrastructure/cluster/{ => exception}/PhabricatorClusterException.php (100%) rename src/infrastructure/cluster/{ => exception}/PhabricatorClusterExceptionHandler.php (100%) rename src/infrastructure/cluster/{ => exception}/PhabricatorClusterImpossibleWriteException.php (100%) rename src/infrastructure/cluster/{ => exception}/PhabricatorClusterImproperWriteException.php (100%) create mode 100644 src/infrastructure/cluster/exception/PhabricatorClusterNoHostForRoleException.php rename src/infrastructure/cluster/{ => exception}/PhabricatorClusterStrandedException.php (100%) create mode 100644 src/infrastructure/cluster/search/PhabricatorElasticSearchHost.php create mode 100644 src/infrastructure/cluster/search/PhabricatorMySQLSearchHost.php create mode 100644 src/infrastructure/cluster/search/PhabricatorSearchHost.php create mode 100644 src/infrastructure/cluster/search/PhabricatorSearchService.php diff --git a/resources/sql/autopatches/20161130.search.02.rebuild.php b/resources/sql/autopatches/20161130.search.02.rebuild.php index a5a9755839..d179c44c30 100644 --- a/resources/sql/autopatches/20161130.search.02.rebuild.php +++ b/resources/sql/autopatches/20161130.search.02.rebuild.php @@ -1,7 +1,15 @@ getEngine(); + if ($engine instanceof PhabricatorMySQLFulltextStorageEngine) { + $use_mysql = true; + } +} if ($use_mysql) { $field = new PhabricatorSearchDocumentField(); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 270fb84194..7c233cb93a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2259,12 +2259,15 @@ phutil_register_library_map(array( 'PhabricatorChatLogQuery' => 'applications/chatlog/query/PhabricatorChatLogQuery.php', 'PhabricatorChunkedFileStorageEngine' => 'applications/files/engine/PhabricatorChunkedFileStorageEngine.php', 'PhabricatorClusterConfigOptions' => 'applications/config/option/PhabricatorClusterConfigOptions.php', - 'PhabricatorClusterDatabasesConfigOptionType' => 'infrastructure/cluster/PhabricatorClusterDatabasesConfigOptionType.php', - 'PhabricatorClusterException' => 'infrastructure/cluster/PhabricatorClusterException.php', - 'PhabricatorClusterExceptionHandler' => 'infrastructure/cluster/PhabricatorClusterExceptionHandler.php', - 'PhabricatorClusterImpossibleWriteException' => 'infrastructure/cluster/PhabricatorClusterImpossibleWriteException.php', - 'PhabricatorClusterImproperWriteException' => 'infrastructure/cluster/PhabricatorClusterImproperWriteException.php', - 'PhabricatorClusterStrandedException' => 'infrastructure/cluster/PhabricatorClusterStrandedException.php', + 'PhabricatorClusterDatabasesConfigOptionType' => 'infrastructure/cluster/config/PhabricatorClusterDatabasesConfigOptionType.php', + 'PhabricatorClusterException' => 'infrastructure/cluster/exception/PhabricatorClusterException.php', + 'PhabricatorClusterExceptionHandler' => 'infrastructure/cluster/exception/PhabricatorClusterExceptionHandler.php', + 'PhabricatorClusterImpossibleWriteException' => 'infrastructure/cluster/exception/PhabricatorClusterImpossibleWriteException.php', + 'PhabricatorClusterImproperWriteException' => 'infrastructure/cluster/exception/PhabricatorClusterImproperWriteException.php', + 'PhabricatorClusterNoHostForRoleException' => 'infrastructure/cluster/exception/PhabricatorClusterNoHostForRoleException.php', + 'PhabricatorClusterSearchConfigOptionType' => 'infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php', + 'PhabricatorClusterServiceHealthRecord' => 'infrastructure/cluster/PhabricatorClusterServiceHealthRecord.php', + 'PhabricatorClusterStrandedException' => 'infrastructure/cluster/exception/PhabricatorClusterStrandedException.php', 'PhabricatorColumnProxyInterface' => 'applications/project/interface/PhabricatorColumnProxyInterface.php', 'PhabricatorColumnsEditField' => 'applications/transactions/editfield/PhabricatorColumnsEditField.php', 'PhabricatorCommentEditEngineExtension' => 'applications/transactions/engineextension/PhabricatorCommentEditEngineExtension.php', @@ -2310,6 +2313,7 @@ phutil_register_library_map(array( 'PhabricatorConfigClusterDatabasesController' => 'applications/config/controller/PhabricatorConfigClusterDatabasesController.php', 'PhabricatorConfigClusterNotificationsController' => 'applications/config/controller/PhabricatorConfigClusterNotificationsController.php', 'PhabricatorConfigClusterRepositoriesController' => 'applications/config/controller/PhabricatorConfigClusterRepositoriesController.php', + 'PhabricatorConfigClusterSearchController' => 'applications/config/controller/PhabricatorConfigClusterSearchController.php', 'PhabricatorConfigCollectorsModule' => 'applications/config/module/PhabricatorConfigCollectorsModule.php', 'PhabricatorConfigColumnSchema' => 'applications/config/schema/PhabricatorConfigColumnSchema.php', 'PhabricatorConfigConfigPHIDType' => 'applications/config/phid/PhabricatorConfigConfigPHIDType.php', @@ -2543,7 +2547,6 @@ phutil_register_library_map(array( 'PhabricatorDashboardViewController' => 'applications/dashboard/controller/PhabricatorDashboardViewController.php', 'PhabricatorDataCacheSpec' => 'applications/cache/spec/PhabricatorDataCacheSpec.php', 'PhabricatorDataNotAttachedException' => 'infrastructure/storage/lisk/PhabricatorDataNotAttachedException.php', - 'PhabricatorDatabaseHealthRecord' => 'infrastructure/cluster/PhabricatorDatabaseHealthRecord.php', 'PhabricatorDatabaseRef' => 'infrastructure/cluster/PhabricatorDatabaseRef.php', 'PhabricatorDatabaseRefParser' => 'infrastructure/cluster/PhabricatorDatabaseRefParser.php', 'PhabricatorDatabaseSetupCheck' => 'applications/config/check/PhabricatorDatabaseSetupCheck.php', @@ -2651,6 +2654,8 @@ phutil_register_library_map(array( 'PhabricatorEditorMultipleSetting' => 'applications/settings/setting/PhabricatorEditorMultipleSetting.php', 'PhabricatorEditorSetting' => 'applications/settings/setting/PhabricatorEditorSetting.php', 'PhabricatorElasticFulltextStorageEngine' => 'applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php', + 'PhabricatorElasticSearchHost' => 'infrastructure/cluster/search/PhabricatorElasticSearchHost.php', + 'PhabricatorElasticSearchQueryBuilder' => 'applications/search/fulltextstorage/PhabricatorElasticSearchQueryBuilder.php', 'PhabricatorElasticSearchSetupCheck' => 'applications/config/check/PhabricatorElasticSearchSetupCheck.php', 'PhabricatorEmailAddressesSettingsPanel' => 'applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php', 'PhabricatorEmailContentSource' => 'applications/metamta/contentsource/PhabricatorEmailContentSource.php', @@ -3073,6 +3078,7 @@ phutil_register_library_map(array( 'PhabricatorMySQLConfigOptions' => 'applications/config/option/PhabricatorMySQLConfigOptions.php', 'PhabricatorMySQLFileStorageEngine' => 'applications/files/engine/PhabricatorMySQLFileStorageEngine.php', 'PhabricatorMySQLFulltextStorageEngine' => 'applications/search/fulltextstorage/PhabricatorMySQLFulltextStorageEngine.php', + 'PhabricatorMySQLSearchHost' => 'infrastructure/cluster/search/PhabricatorMySQLSearchHost.php', 'PhabricatorMySQLSetupCheck' => 'applications/config/check/PhabricatorMySQLSetupCheck.php', 'PhabricatorNamedQuery' => 'applications/search/storage/PhabricatorNamedQuery.php', 'PhabricatorNamedQueryQuery' => 'applications/search/query/PhabricatorNamedQueryQuery.php', @@ -3762,7 +3768,6 @@ phutil_register_library_map(array( 'PhabricatorSearchApplicationStorageEnginePanel' => 'applications/search/applicationpanel/PhabricatorSearchApplicationStorageEnginePanel.php', 'PhabricatorSearchBaseController' => 'applications/search/controller/PhabricatorSearchBaseController.php', 'PhabricatorSearchCheckboxesField' => 'applications/search/field/PhabricatorSearchCheckboxesField.php', - 'PhabricatorSearchConfigOptions' => 'applications/search/config/PhabricatorSearchConfigOptions.php', 'PhabricatorSearchConstraintException' => 'applications/search/exception/PhabricatorSearchConstraintException.php', 'PhabricatorSearchController' => 'applications/search/controller/PhabricatorSearchController.php', 'PhabricatorSearchCustomFieldProxyField' => 'applications/search/field/PhabricatorSearchCustomFieldProxyField.php', @@ -3785,6 +3790,7 @@ phutil_register_library_map(array( 'PhabricatorSearchEngineExtensionModule' => 'applications/search/engineextension/PhabricatorSearchEngineExtensionModule.php', 'PhabricatorSearchEngineTestCase' => 'applications/search/engine/__tests__/PhabricatorSearchEngineTestCase.php', 'PhabricatorSearchField' => 'applications/search/field/PhabricatorSearchField.php', + 'PhabricatorSearchHost' => 'infrastructure/cluster/search/PhabricatorSearchHost.php', 'PhabricatorSearchHovercardController' => 'applications/search/controller/PhabricatorSearchHovercardController.php', 'PhabricatorSearchIndexVersion' => 'applications/search/storage/PhabricatorSearchIndexVersion.php', 'PhabricatorSearchIndexVersionDestructionEngineExtension' => 'applications/search/engineextension/PhabricatorSearchIndexVersionDestructionEngineExtension.php', @@ -3804,6 +3810,7 @@ phutil_register_library_map(array( 'PhabricatorSearchSchemaSpec' => 'applications/search/storage/PhabricatorSearchSchemaSpec.php', 'PhabricatorSearchScopeSetting' => 'applications/settings/setting/PhabricatorSearchScopeSetting.php', 'PhabricatorSearchSelectField' => 'applications/search/field/PhabricatorSearchSelectField.php', + 'PhabricatorSearchService' => 'infrastructure/cluster/search/PhabricatorSearchService.php', 'PhabricatorSearchStringListField' => 'applications/search/field/PhabricatorSearchStringListField.php', 'PhabricatorSearchSubscribersField' => 'applications/search/field/PhabricatorSearchSubscribersField.php', 'PhabricatorSearchTextField' => 'applications/search/field/PhabricatorSearchTextField.php', @@ -7303,6 +7310,9 @@ phutil_register_library_map(array( 'PhabricatorClusterExceptionHandler' => 'PhabricatorRequestExceptionHandler', 'PhabricatorClusterImpossibleWriteException' => 'PhabricatorClusterException', 'PhabricatorClusterImproperWriteException' => 'PhabricatorClusterException', + 'PhabricatorClusterNoHostForRoleException' => 'Exception', + 'PhabricatorClusterSearchConfigOptionType' => 'PhabricatorConfigJSONOptionType', + 'PhabricatorClusterServiceHealthRecord' => 'Phobject', 'PhabricatorClusterStrandedException' => 'PhabricatorClusterException', 'PhabricatorColumnsEditField' => 'PhabricatorPHIDListEditField', 'PhabricatorCommentEditEngineExtension' => 'PhabricatorEditEngineExtension', @@ -7354,6 +7364,7 @@ phutil_register_library_map(array( 'PhabricatorConfigClusterDatabasesController' => 'PhabricatorConfigController', 'PhabricatorConfigClusterNotificationsController' => 'PhabricatorConfigController', 'PhabricatorConfigClusterRepositoriesController' => 'PhabricatorConfigController', + 'PhabricatorConfigClusterSearchController' => 'PhabricatorConfigController', 'PhabricatorConfigCollectorsModule' => 'PhabricatorConfigModule', 'PhabricatorConfigColumnSchema' => 'PhabricatorConfigStorageSchema', 'PhabricatorConfigConfigPHIDType' => 'PhabricatorPHIDType', @@ -7624,7 +7635,6 @@ phutil_register_library_map(array( 'PhabricatorDashboardViewController' => 'PhabricatorDashboardProfileController', 'PhabricatorDataCacheSpec' => 'PhabricatorCacheSpec', 'PhabricatorDataNotAttachedException' => 'Exception', - 'PhabricatorDatabaseHealthRecord' => 'Phobject', 'PhabricatorDatabaseRef' => 'Phobject', 'PhabricatorDatabaseRefParser' => 'Phobject', 'PhabricatorDatabaseSetupCheck' => 'PhabricatorSetupCheck', @@ -7738,6 +7748,7 @@ phutil_register_library_map(array( 'PhabricatorEditorMultipleSetting' => 'PhabricatorSelectSetting', 'PhabricatorEditorSetting' => 'PhabricatorStringSetting', 'PhabricatorElasticFulltextStorageEngine' => 'PhabricatorFulltextStorageEngine', + 'PhabricatorElasticSearchHost' => 'PhabricatorSearchHost', 'PhabricatorElasticSearchSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorEmailAddressesSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorEmailContentSource' => 'PhabricatorContentSource', @@ -8208,6 +8219,7 @@ phutil_register_library_map(array( 'PhabricatorMySQLConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorMySQLFileStorageEngine' => 'PhabricatorFileStorageEngine', 'PhabricatorMySQLFulltextStorageEngine' => 'PhabricatorFulltextStorageEngine', + 'PhabricatorMySQLSearchHost' => 'PhabricatorSearchHost', 'PhabricatorMySQLSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorNamedQuery' => array( 'PhabricatorSearchDAO', @@ -9074,7 +9086,6 @@ phutil_register_library_map(array( 'PhabricatorSearchApplicationStorageEnginePanel' => 'PhabricatorApplicationConfigurationPanel', 'PhabricatorSearchBaseController' => 'PhabricatorController', 'PhabricatorSearchCheckboxesField' => 'PhabricatorSearchField', - 'PhabricatorSearchConfigOptions' => 'PhabricatorApplicationConfigOptions', 'PhabricatorSearchConstraintException' => 'Exception', 'PhabricatorSearchController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchCustomFieldProxyField' => 'PhabricatorSearchField', @@ -9097,6 +9108,7 @@ phutil_register_library_map(array( 'PhabricatorSearchEngineExtensionModule' => 'PhabricatorConfigModule', 'PhabricatorSearchEngineTestCase' => 'PhabricatorTestCase', 'PhabricatorSearchField' => 'Phobject', + 'PhabricatorSearchHost' => 'Phobject', 'PhabricatorSearchHovercardController' => 'PhabricatorSearchBaseController', 'PhabricatorSearchIndexVersion' => 'PhabricatorSearchDAO', 'PhabricatorSearchIndexVersionDestructionEngineExtension' => 'PhabricatorDestructionEngineExtension', @@ -9116,6 +9128,7 @@ phutil_register_library_map(array( 'PhabricatorSearchSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'PhabricatorSearchScopeSetting' => 'PhabricatorInternalSetting', 'PhabricatorSearchSelectField' => 'PhabricatorSearchField', + 'PhabricatorSearchService' => 'Phobject', 'PhabricatorSearchStringListField' => 'PhabricatorSearchField', 'PhabricatorSearchSubscribersField' => 'PhabricatorSearchTokenizerField', 'PhabricatorSearchTextField' => 'PhabricatorSearchField', diff --git a/src/applications/config/application/PhabricatorConfigApplication.php b/src/applications/config/application/PhabricatorConfigApplication.php index 6b2704b0b4..510cb6f76d 100644 --- a/src/applications/config/application/PhabricatorConfigApplication.php +++ b/src/applications/config/application/PhabricatorConfigApplication.php @@ -69,6 +69,7 @@ final class PhabricatorConfigApplication extends PhabricatorApplication { 'databases/' => 'PhabricatorConfigClusterDatabasesController', 'notifications/' => 'PhabricatorConfigClusterNotificationsController', 'repositories/' => 'PhabricatorConfigClusterRepositoriesController', + 'search/' => 'PhabricatorConfigClusterSearchController', ), ), ); diff --git a/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php b/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php index f137f2527f..d8864b5740 100644 --- a/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php +++ b/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php @@ -7,71 +7,74 @@ final class PhabricatorElasticSearchSetupCheck extends PhabricatorSetupCheck { } protected function executeChecks() { - if (!$this->shouldUseElasticSearchEngine()) { - return; - } + $services = PhabricatorSearchService::getAllServices(); - $engine = new PhabricatorElasticFulltextStorageEngine(); - - $index_exists = null; - $index_sane = null; - try { - $index_exists = $engine->indexExists(); - if ($index_exists) { - $index_sane = $engine->indexIsSane(); + foreach ($services as $service) { + try { + $host = $service->getAnyHostForRole('read'); + } catch (PhabricatorClusterNoHostForRoleException $e) { + // ignore the error + continue; } - } catch (Exception $ex) { - $summary = pht('Elasticsearch is not reachable as configured.'); - $message = pht( - 'Elasticsearch is configured (with the %s setting) but Phabricator '. - 'encountered an exception when trying to test the index.'. - "\n\n". - '%s', - phutil_tag('tt', array(), 'search.elastic.host'), - phutil_tag('pre', array(), $ex->getMessage())); + if ($host instanceof PhabricatorElasticSearchHost) { + $index_exists = null; + $index_sane = null; + try { + $engine = $host->getEngine(); + $index_exists = $engine->indexExists(); + if ($index_exists) { + $index_sane = $engine->indexIsSane(); + } + } catch (Exception $ex) { + $summary = pht('Elasticsearch is not reachable as configured.'); + $message = pht( + 'Elasticsearch is configured (with the %s setting) but Phabricator'. + ' encountered an exception when trying to test the index.'. + "\n\n". + '%s', + phutil_tag('tt', array(), 'cluster.search'), + phutil_tag('pre', array(), $ex->getMessage())); - $this->newIssue('elastic.misconfigured') - ->setName(pht('Elasticsearch Misconfigured')) - ->setSummary($summary) - ->setMessage($message) - ->addRelatedPhabricatorConfig('search.elastic.host'); - return; - } + $this->newIssue('elastic.misconfigured') + ->setName(pht('Elasticsearch Misconfigured')) + ->setSummary($summary) + ->setMessage($message) + ->addRelatedPhabricatorConfig('cluster.search'); + return; + } - if (!$index_exists) { - $summary = pht( - 'You enabled Elasticsearch but the index does not exist.'); + if (!$index_exists) { + $summary = pht( + 'You enabled Elasticsearch but the index does not exist.'); - $message = pht( - 'You likely enabled search.elastic.host without creating the '. - 'index. Run `./bin/search init` to correct the index.'); + $message = pht( + 'You likely enabled cluster.search without creating the '. + 'index. Run `./bin/search init` to correct the index.'); - $this - ->newIssue('elastic.missing-index') - ->setName(pht('Elasticsearch index Not Found')) - ->setSummary($summary) - ->setMessage($message) - ->addRelatedPhabricatorConfig('search.elastic.host'); - } else if (!$index_sane) { - $summary = pht( - 'Elasticsearch index exists but needs correction.'); + $this + ->newIssue('elastic.missing-index') + ->setName(pht('Elasticsearch index Not Found')) + ->setSummary($summary) + ->setMessage($message) + ->addRelatedPhabricatorConfig('cluster.search'); + } else if (!$index_sane) { + $summary = pht( + 'Elasticsearch index exists but needs correction.'); - $message = pht( - 'Either the Phabricator schema for Elasticsearch has changed '. - 'or Elasticsearch created the index automatically. Run '. - '`./bin/search init` to correct the index.'); + $message = pht( + 'Either the Phabricator schema for Elasticsearch has changed '. + 'or Elasticsearch created the index automatically. Run '. + '`./bin/search init` to correct the index.'); - $this - ->newIssue('elastic.broken-index') - ->setName(pht('Elasticsearch index Incorrect')) - ->setSummary($summary) - ->setMessage($message); + $this + ->newIssue('elastic.broken-index') + ->setName(pht('Elasticsearch index Incorrect')) + ->setSummary($summary) + ->setMessage($message); + } + } } } - protected function shouldUseElasticSearchEngine() { - $search_engine = PhabricatorFulltextStorageEngine::loadEngine(); - return ($search_engine instanceof PhabricatorElasticFulltextStorageEngine); - } } diff --git a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php index f607610684..84fd5bedf2 100644 --- a/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php +++ b/src/applications/config/check/PhabricatorExtraConfigSetupCheck.php @@ -198,6 +198,10 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'This option has been removed, you can use Dashboards to provide '. 'homepage customization. See T11533 for more details.'); + $elastic_reason = pht( + 'Elasticsearch is now configured with "%s".', + 'cluster.search'); + $ancient_config += array( 'phid.external-loaders' => pht( @@ -348,6 +352,10 @@ final class PhabricatorExtraConfigSetupCheck extends PhabricatorSetupCheck { 'mysql.configuration-provider' => pht( 'Phabricator now has application-level management of partitioning '. 'and replicas.'), + + 'search.elastic.host' => $elastic_reason, + 'search.elastic.namespace' => $elastic_reason, + ); return $ancient_config; diff --git a/src/applications/config/check/PhabricatorMySQLSetupCheck.php b/src/applications/config/check/PhabricatorMySQLSetupCheck.php index 152af61bf4..a9f6a77cb7 100644 --- a/src/applications/config/check/PhabricatorMySQLSetupCheck.php +++ b/src/applications/config/check/PhabricatorMySQLSetupCheck.php @@ -379,8 +379,13 @@ final class PhabricatorMySQLSetupCheck extends PhabricatorSetupCheck { } protected function shouldUseMySQLSearchEngine() { - $search_engine = PhabricatorFulltextStorageEngine::loadEngine(); - return ($search_engine instanceof PhabricatorMySQLFulltextStorageEngine); + $services = PhabricatorSearchService::getAllServices(); + foreach ($services as $service) { + if ($service instanceof PhabricatorMySQLSearchHost) { + return true; + } + } + return false; } } diff --git a/src/applications/config/controller/PhabricatorConfigClusterSearchController.php b/src/applications/config/controller/PhabricatorConfigClusterSearchController.php new file mode 100644 index 0000000000..4d3ce407ab --- /dev/null +++ b/src/applications/config/controller/PhabricatorConfigClusterSearchController.php @@ -0,0 +1,129 @@ +buildSideNavView(); + $nav->selectFilter('cluster/search/'); + + $title = pht('Cluster Search'); + $doc_href = PhabricatorEnv::getDoclink('Cluster: Search'); + + $header = id(new PHUIHeaderView()) + ->setHeader($title) + ->setProfileHeader(true) + ->addActionLink( + id(new PHUIButtonView()) + ->setIcon('fa-book') + ->setHref($doc_href) + ->setTag('a') + ->setText(pht('Documentation'))); + + $crumbs = $this + ->buildApplicationCrumbs($nav) + ->addTextCrumb($title) + ->setBorder(true); + + $search_status = $this->buildClusterSearchStatus(); + + $content = id(new PhabricatorConfigPageView()) + ->setHeader($header) + ->setContent($search_status); + + return $this->newPage() + ->setTitle($title) + ->setCrumbs($crumbs) + ->setNavigation($nav) + ->appendChild($content) + ->addClass('white-background'); + } + + private function buildClusterSearchStatus() { + $viewer = $this->getViewer(); + + $services = PhabricatorSearchService::getAllServices(); + Javelin::initBehavior('phabricator-tooltips'); + + $view = array(); + foreach ($services as $service) { + $view[] = $this->renderStatusView($service); + } + return $view; + } + + private function renderStatusView($service) { + $head = array_merge( + array(pht('Type')), + array_keys($service->getStatusViewColumns()), + array(pht('Status'))); + + $rows = array(); + + $status_map = PhabricatorSearchService::getConnectionStatusMap(); + $stats = false; + $stats_view = false; + + foreach ($service->getHosts() as $host) { + try { + $status = $host->getConnectionStatus(); + $status = idx($status_map, $status, array()); + } catch (Exception $ex) { + $status['icon'] = 'fa-times'; + $status['label'] = pht('Connection Error'); + $status['color'] = 'red'; + $host->didHealthCheck(false); + } + + if (!$stats_view) { + try { + $stats = $host->getEngine()->getIndexStats($host); + $stats_view = $this->renderIndexStats($stats); + } catch (Exception $e) { + $stats_view = false; + } + } + + $type_icon = 'fa-search sky'; + $type_tip = $host->getDisplayName(); + + $type_icon = id(new PHUIIconView()) + ->setIcon($type_icon); + $status_view = array( + id(new PHUIIconView())->setIcon($status['icon'].' '.$status['color']), + ' ', + $status['label'], + ); + $row = array(array($type_icon, ' ', $type_tip)); + $row = array_merge($row, array_values( + $host->getStatusViewColumns())); + $row[] = $status_view; + $rows[] = $row; + } + + $table = id(new AphrontTableView($rows)) + ->setNoDataString(pht('No search servers are configured.')) + ->setHeaders($head); + + $view = id(new PHUIObjectBoxView()) + ->setHeaderText($service->getDisplayName()) + ->setBackground(PHUIObjectBoxView::BLUE_PROPERTY) + ->setTable($table); + + if ($stats_view) { + $view->addPropertyList($stats_view); + } + return $view; + } + + private function renderIndexStats($stats) { + $view = id(new PHUIPropertyListView()); + if ($stats !== false) { + foreach ($stats as $label => $val) { + $view->addProperty($label, $val); + } + } + return $view; + } + +} diff --git a/src/applications/config/controller/PhabricatorConfigController.php b/src/applications/config/controller/PhabricatorConfigController.php index 5ad0ecbbf8..2abf2b3b31 100644 --- a/src/applications/config/controller/PhabricatorConfigController.php +++ b/src/applications/config/controller/PhabricatorConfigController.php @@ -42,8 +42,11 @@ abstract class PhabricatorConfigController extends PhabricatorController { pht('Notification Servers'), null, 'fa-bell-o'); $nav->addFilter('cluster/repositories/', pht('Repository Servers'), null, 'fa-code'); + $nav->addFilter('cluster/search/', + pht('Search Servers'), null, 'fa-search'); $nav->addLabel(pht('Modules')); + $modules = PhabricatorConfigModule::getAllModules(); foreach ($modules as $key => $module) { $nav->addFilter('module/'.$key.'/', diff --git a/src/applications/config/option/PhabricatorClusterConfigOptions.php b/src/applications/config/option/PhabricatorClusterConfigOptions.php index bcf498c32b..c3636c31e0 100644 --- a/src/applications/config/option/PhabricatorClusterConfigOptions.php +++ b/src/applications/config/option/PhabricatorClusterConfigOptions.php @@ -38,6 +38,17 @@ EOTEXT $intro_href = PhabricatorEnv::getDoclink('Clustering Introduction'); $intro_name = pht('Clustering Introduction'); + $search_type = 'custom:PhabricatorClusterSearchConfigOptionType'; + $search_help = $this->deformat(pht(<<newOption('cluster.addresses', 'list', array()) ->setLocked(true) @@ -114,6 +125,21 @@ EOTEXT ->setSummary( pht('Configure database read replicas.')) ->setDescription($databases_help), + $this->newOption('cluster.search', $search_type, array()) + ->setLocked(true) + ->setSummary( + pht('Configure full-text search services.')) + ->setDescription($search_help) + ->setDefault( + array( + array( + 'type' => 'mysql', + 'roles' => array( + 'read' => true, + 'write' => true, + ), + ), + )), ); } diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php index d95fd8c2cf..0f55ce8bdd 100644 --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -513,14 +513,14 @@ final class ManiphestTaskQuery extends PhabricatorCursorPagedPolicyAwareQuery { ->setEngineClassName('PhabricatorSearchApplicationSearchEngine') ->setParameter('query', $this->fullTextSearch); - // NOTE: Setting this to something larger than 2^53 will raise errors in + // NOTE: Setting this to something larger than 10,000 will raise errors in // ElasticSearch, and billions of results won't fit in memory anyway. - $fulltext_query->setParameter('limit', 100000); + $fulltext_query->setParameter('limit', 10000); $fulltext_query->setParameter('types', array(ManiphestTaskPHIDType::TYPECONST)); - $engine = PhabricatorFulltextStorageEngine::loadEngine(); - $fulltext_results = $engine->executeSearch($fulltext_query); + $fulltext_results = PhabricatorSearchService::executeSearch( + $fulltext_query); if (empty($fulltext_results)) { $fulltext_results = array(null); diff --git a/src/applications/project/search/PhabricatorProjectFulltextEngine.php b/src/applications/project/search/PhabricatorProjectFulltextEngine.php index f0940286e5..14314c3436 100644 --- a/src/applications/project/search/PhabricatorProjectFulltextEngine.php +++ b/src/applications/project/search/PhabricatorProjectFulltextEngine.php @@ -10,7 +10,15 @@ final class PhabricatorProjectFulltextEngine $project = $object; $project->updateDatasourceTokens(); - $document->setDocumentTitle($project->getName()); + $document->setDocumentTitle($project->getDisplayName()); + $document->addField(PhabricatorSearchDocumentFieldType::FIELD_KEYWORDS, + $project->getPrimarySlug()); + try { + $slugs = $project->getSlugs(); + foreach ($slugs as $slug) {} + } catch (PhabricatorDataNotAttachedException $e) { + // ignore + } $document->addRelationship( $project->isArchived() diff --git a/src/applications/search/config/PhabricatorSearchConfigOptions.php b/src/applications/search/config/PhabricatorSearchConfigOptions.php deleted file mode 100644 index 2f2cc4f902..0000000000 --- a/src/applications/search/config/PhabricatorSearchConfigOptions.php +++ /dev/null @@ -1,35 +0,0 @@ -newOption('search.elastic.host', 'string', null) - ->setLocked(true) - ->setDescription(pht('Elastic Search host.')) - ->addExample('http://elastic.example.com:9200/', pht('Valid Setting')), - $this->newOption('search.elastic.namespace', 'string', 'phabricator') - ->setLocked(true) - ->setDescription(pht('Elastic Search index.')) - ->addExample('phabricator2', pht('Valid Setting')), - ); - } - -} diff --git a/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php b/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php index 10dbf0ca65..12c90f8469 100644 --- a/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php +++ b/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php @@ -5,5 +5,6 @@ final class PhabricatorSearchDocumentFieldType extends Phobject { const FIELD_TITLE = 'titl'; const FIELD_BODY = 'body'; const FIELD_COMMENT = 'cmnt'; + const FIELD_KEYWORDS = 'kwrd'; } diff --git a/src/applications/search/engine/__tests__/PhabricatorSearchEngineTestCase.php b/src/applications/search/engine/__tests__/PhabricatorSearchEngineTestCase.php index b535d4f5cf..f5dbd9ef9c 100644 --- a/src/applications/search/engine/__tests__/PhabricatorSearchEngineTestCase.php +++ b/src/applications/search/engine/__tests__/PhabricatorSearchEngineTestCase.php @@ -3,8 +3,8 @@ final class PhabricatorSearchEngineTestCase extends PhabricatorTestCase { public function testLoadAllEngines() { - PhabricatorFulltextStorageEngine::loadAllEngines(); - $this->assertTrue(true); + $services = PhabricatorSearchService::getAllServices(); + $this->assertTrue(!empty($services)); } } diff --git a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php index ee067b942d..bc32da5ef4 100644 --- a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php +++ b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php @@ -1,37 +1,52 @@ uri = PhabricatorEnv::getEnvConfig('search.elastic.host'); - $this->index = PhabricatorEnv::getEnvConfig('search.elastic.namespace'); + public function setService(PhabricatorSearchService $service) { + $this->service = $service; + $config = $service->getConfig(); + $index = idx($config, 'path', '/phabricator'); + $this->index = str_replace('/', '', $index); + $this->timeout = idx($config, 'timeout', 15); + $this->version = (int)idx($config, 'version', 5); + return $this; } public function getEngineIdentifier() { return 'elasticsearch'; } - public function getEnginePriority() { - return 10; + public function getTimestampField() { + return $this->version < 2 ? + '_timestamp' : 'lastModified'; } - public function isEnabled() { - return (bool)$this->uri; + public function getTextFieldType() { + return $this->version >= 5 + ? 'text' : 'string'; } - public function setURI($uri) { - $this->uri = $uri; - return $this; + public function getHostType() { + return new PhabricatorElasticSearchHost($this); } - public function setIndex($index) { - $this->index = $index; - return $this; + /** + * @return PhabricatorElasticSearchHost + */ + public function getHostForRead() { + return $this->getService()->getAnyHostForRole('read'); + } + + /** + * @return PhabricatorElasticSearchHost + */ + public function getHostForWrite() { + return $this->getService()->getAnyHostForRole('write'); } public function setTimeout($timeout) { @@ -39,21 +54,21 @@ final class PhabricatorElasticFulltextStorageEngine return $this; } - public function getURI() { - return $this->uri; - } - - public function getIndex() { - return $this->index; - } - public function getTimeout() { return $this->timeout; } + public function getTypeConstants($class) { + $relationship_class = new ReflectionClass($class); + $typeconstants = $relationship_class->getConstants(); + return array_unique(array_values($typeconstants)); + } + public function reindexAbstractDocument( PhabricatorSearchAbstractDocument $doc) { + $host = $this->getHostForWrite(); + $type = $doc->getDocumentType(); $phid = $doc->getPHID(); $handle = id(new PhabricatorHandleQuery()) @@ -61,36 +76,47 @@ final class PhabricatorElasticFulltextStorageEngine ->withPHIDs(array($phid)) ->executeOne(); + $timestamp_key = $this->getTimestampField(); + // URL is not used internally but it can be useful externally. $spec = array( 'title' => $doc->getDocumentTitle(), 'url' => PhabricatorEnv::getProductionURI($handle->getURI()), 'dateCreated' => $doc->getDocumentCreated(), - '_timestamp' => $doc->getDocumentModified(), - 'field' => array(), - 'relationship' => array(), + $timestamp_key => $doc->getDocumentModified(), ); foreach ($doc->getFieldData() as $field) { - $spec['field'][] = array_combine(array('type', 'corpus', 'aux'), $field); + list($field_name, $corpus, $aux) = $field; + if (!isset($spec[$field_name])) { + $spec[$field_name] = array($corpus); + } else { + $spec[$field_name][] = $corpus; + } + if ($aux != null) { + $spec[$field_name][] = $aux; + } } - foreach ($doc->getRelationshipData() as $relationship) { - list($rtype, $to_phid, $to_type, $time) = $relationship; - $spec['relationship'][$rtype][] = array( - 'phid' => $to_phid, - 'phidType' => $to_type, - 'when' => (int)$time, - ); + foreach ($doc->getRelationshipData() as $field) { + list($field_name, $related_phid, $rtype, $time) = $field; + if (!isset($spec[$field_name])) { + $spec[$field_name] = array($related_phid); + } else { + $spec[$field_name][] = $related_phid; + } + if ($time) { + $spec[$field_name.'_ts'] = $time; + } } - $this->executeRequest("/{$type}/{$phid}/", $spec, 'PUT'); + $this->executeRequest($host, "/{$type}/{$phid}/", $spec, 'PUT'); } public function reconstructDocument($phid) { $type = phid_get_type($phid); - - $response = $this->executeRequest("/{$type}/{$phid}", array()); + $host = $this->getHostForRead(); + $response = $this->executeRequest($host, "/{$type}/{$phid}", array()); if (empty($response['exists'])) { return null; @@ -103,10 +129,11 @@ final class PhabricatorElasticFulltextStorageEngine $doc->setDocumentType($response['_type']); $doc->setDocumentTitle($hit['title']); $doc->setDocumentCreated($hit['dateCreated']); - $doc->setDocumentModified($hit['_timestamp']); + $doc->setDocumentModified($hit[$this->getTimestampField()]); foreach ($hit['field'] as $fdef) { - $doc->addField($fdef['type'], $fdef['corpus'], $fdef['aux']); + $field_type = $fdef['type']; + $doc->addField($field_type, $hit[$field_type], $fdef['aux']); } foreach ($hit['relationship'] as $rtype => $rships) { @@ -123,35 +150,51 @@ final class PhabricatorElasticFulltextStorageEngine } private function buildSpec(PhabricatorSavedQuery $query) { - $spec = array(); - $filter = array(); - $title_spec = array(); + $q = new PhabricatorElasticSearchQueryBuilder('bool'); + $query_string = $query->getParameter('query'); + if (strlen($query_string)) { + $fields = $this->getTypeConstants('PhabricatorSearchDocumentFieldType'); - if (strlen($query->getParameter('query'))) { - $spec[] = array( + // Build a simple_query_string query over all fields that must match all + // of the words in the search string. + $q->addMustClause(array( 'simple_query_string' => array( - 'query' => $query->getParameter('query'), - 'fields' => array('field.corpus'), + 'query' => $query_string, + 'fields' => array( + '_all', + ), + 'default_operator' => 'OR', ), - ); + )); - $title_spec = array( + // This second query clause is "SHOULD' so it only affects ranking of + // documents which already matched the Must clause. This amplifies the + // score of documents which have an exact match on title, body + // or comments. + $q->addShouldClause(array( 'simple_query_string' => array( - 'query' => $query->getParameter('query'), - 'fields' => array('title'), + 'query' => $query_string, + 'fields' => array( + PhabricatorSearchDocumentFieldType::FIELD_TITLE.'^4', + PhabricatorSearchDocumentFieldType::FIELD_BODY.'^3', + PhabricatorSearchDocumentFieldType::FIELD_COMMENT.'^1.2', + ), + 'analyzer' => 'english_exact', + 'default_operator' => 'and', ), - ); + )); + } $exclude = $query->getParameter('exclude'); if ($exclude) { - $filter[] = array( + $q->addFilterClause(array( 'not' => array( 'ids' => array( 'values' => array($exclude), ), ), - ); + )); } $relationship_map = array( @@ -176,75 +219,59 @@ final class PhabricatorElasticFulltextStorageEngine $include_closed = !empty($statuses[$rel_closed]); if ($include_open && !$include_closed) { - $relationship_map[$rel_open] = true; + $q->addExistsClause($rel_open); } else if (!$include_open && $include_closed) { - $relationship_map[$rel_closed] = true; + $q->addExistsClause($rel_closed); } if ($query->getParameter('withUnowned')) { - $relationship_map[$rel_unowned] = true; + $q->addExistsClause($rel_unowned); } $rel_owner = PhabricatorSearchRelationship::RELATIONSHIP_OWNER; if ($query->getParameter('withAnyOwner')) { - $relationship_map[$rel_owner] = true; + $q->addExistsClause($rel_owner); } else { $owner_phids = $query->getParameter('ownerPHIDs', array()); - $relationship_map[$rel_owner] = $owner_phids; - } - - foreach ($relationship_map as $field => $param) { - if (is_array($param) && $param) { - $should = array(); - foreach ($param as $val) { - $should[] = array( - 'match' => array( - "relationship.{$field}.phid" => array( - 'query' => $val, - 'type' => 'phrase', - ), - ), - ); - } - // We couldn't solve it by minimum_number_should_match because it can - // match multiple owners without matching author. - $spec[] = array('bool' => array('should' => $should)); - } else if ($param) { - $filter[] = array( - 'exists' => array( - 'field' => "relationship.{$field}.phid", - ), - ); + if (count($owner_phids)) { + $q->addTermsClause($rel_owner, $owner_phids); } } - if ($spec) { - $spec = array('query' => array('bool' => array('must' => $spec))); - if ($title_spec) { - $spec['query']['bool']['should'] = $title_spec; + foreach ($relationship_map as $field => $phids) { + if (is_array($phids) && !empty($phids)) { + $q->addTermsClause($field, $phids); } } - if ($filter) { - $filter = array('filter' => array('and' => $filter)); - if (!$spec) { - $spec = array('query' => array('match_all' => new stdClass())); - } - $spec = array( - 'query' => array( - 'filtered' => $spec + $filter, - ), - ); + if (!$q->getClauseCount('must')) { + $q->addMustClause(array('match_all' => array('boost' => 1 ))); } + $spec = array( + '_source' => false, + 'query' => array( + 'bool' => $q->toArray(), + ), + ); + + if (!$query->getParameter('query')) { $spec['sort'] = array( array('dateCreated' => 'desc'), ); } - $spec['from'] = (int)$query->getParameter('offset', 0); - $spec['size'] = (int)$query->getParameter('limit', 25); + $offset = (int)$query->getParameter('offset', 0); + $limit = (int)$query->getParameter('limit', 101); + if ($offset + $limit > 10000) { + throw new Exception(pht( + 'Query offset is too large. offset+limit=%s (max=%s)', + $offset + $limit, + 10000)); + } + $spec['from'] = $offset; + $spec['size'] = $limit; return $spec; } @@ -261,30 +288,36 @@ final class PhabricatorElasticFulltextStorageEngine // some bigger index). Use '/$types/_search' instead. $uri = '/'.implode(',', $types).'/_search'; - try { - $response = $this->executeRequest($uri, $this->buildSpec($query)); - } catch (HTTPFutureHTTPResponseStatus $ex) { - // elasticsearch probably uses Lucene query syntax: - // http://lucene.apache.org/core/3_6_1/queryparsersyntax.html - // Try literal search if operator search fails. - if (!strlen($query->getParameter('query'))) { - throw $ex; - } - $query = clone $query; - $query->setParameter( - 'query', - addcslashes( - $query->getParameter('query'), '+-&|!(){}[]^"~*?:\\')); - $response = $this->executeRequest($uri, $this->buildSpec($query)); - } + $spec = $this->buildSpec($query); + $exceptions = array(); - $phids = ipull($response['hits']['hits'], '_id'); - return $phids; + foreach ($this->service->getAllHostsForRole('read') as $host) { + try { + $response = $this->executeRequest($host, $uri, $spec); + $phids = ipull($response['hits']['hits'], '_id'); + return $phids; + } catch (Exception $e) { + $exceptions[] = $e; + } + } + throw new PhutilAggregateException('All search hosts failed:', $exceptions); } - public function indexExists() { + public function indexExists(PhabricatorElasticSearchHost $host = null) { + if (!$host) { + $host = $this->getHostForRead(); + } try { - return (bool)$this->executeRequest('/_status/', array()); + if ($this->version >= 5) { + $uri = '/_stats/'; + $res = $this->executeRequest($host, $uri, array()); + return isset($res['indices']['phabricator']); + } else if ($this->version >= 2) { + $uri = ''; + } else { + $uri = '/_status/'; + } + return (bool)$this->executeRequest($host, $uri, array()); } catch (HTTPFutureHTTPResponseStatus $e) { if ($e->getStatusCode() == 404) { return false; @@ -299,53 +332,85 @@ final class PhabricatorElasticFulltextStorageEngine 'index' => array( 'auto_expand_replicas' => '0-2', 'analysis' => array( - 'filter' => array( - 'trigrams_filter' => array( - 'min_gram' => 3, - 'type' => 'ngram', - 'max_gram' => 3, - ), - ), 'analyzer' => array( - 'custom_trigrams' => array( - 'type' => 'custom', - 'filter' => array( - 'lowercase', - 'kstem', - 'trigrams_filter', - ), + 'english_exact' => array( 'tokenizer' => 'standard', + 'filter' => array('lowercase'), ), ), ), ), ); - $types = array_keys( + $fields = $this->getTypeConstants('PhabricatorSearchDocumentFieldType'); + $relationships = $this->getTypeConstants('PhabricatorSearchRelationship'); + + $doc_types = array_keys( PhabricatorSearchApplicationSearchEngine::getIndexableDocumentTypes()); - foreach ($types as $type) { - // Use the custom trigram analyzer for the corpus of text - $data['mappings'][$type]['properties']['field']['properties']['corpus'] = - array('type' => 'string', 'analyzer' => 'custom_trigrams'); + + $text_type = $this->getTextFieldType(); + + foreach ($doc_types as $type) { + $properties = array(); + foreach ($fields as $field) { + // Use the custom analyzer for the corpus of text + $properties[$field] = array( + 'type' => $text_type, + 'analyzer' => 'english_exact', + 'search_analyzer' => 'english', + 'search_quote_analyzer' => 'english_exact', + ); + } + + if ($this->version < 5) { + foreach ($relationships as $rel) { + $properties[$rel] = array( + 'type' => 'string', + 'index' => 'not_analyzed', + 'include_in_all' => false, + ); + $properties[$rel.'_ts'] = array( + 'type' => 'date', + 'include_in_all' => false, + ); + } + } else { + foreach ($relationships as $rel) { + $properties[$rel] = array( + 'type' => 'keyword', + 'include_in_all' => false, + 'doc_values' => false, + ); + $properties[$rel.'_ts'] = array( + 'type' => 'date', + 'include_in_all' => false, + ); + } + } // Ensure we have dateCreated since the default query requires it - $data['mappings'][$type]['properties']['dateCreated']['type'] = 'string'; - } + $properties['dateCreated']['type'] = 'date'; + $properties['lastModified']['type'] = 'date'; + $data['mappings'][$type]['properties'] = $properties; + } return $data; } - public function indexIsSane() { - if (!$this->indexExists()) { + public function indexIsSane(PhabricatorElasticSearchHost $host = null) { + if (!$host) { + $host = $this->getHostForRead(); + } + if (!$this->indexExists($host)) { return false; } - - $cur_mapping = $this->executeRequest('/_mapping/', array()); - $cur_settings = $this->executeRequest('/_settings/', array()); + $cur_mapping = $this->executeRequest($host, '/_mapping/', array()); + $cur_settings = $this->executeRequest($host, '/_settings/', array()); $actual = array_merge($cur_settings[$this->index], $cur_mapping[$this->index]); - return $this->check($actual, $this->getIndexConfiguration()); + $res = $this->check($actual, $this->getIndexConfiguration()); + return $res; } /** @@ -355,7 +420,7 @@ final class PhabricatorElasticFulltextStorageEngine * @param $required array * @return bool */ - private function check($actual, $required) { + private function check($actual, $required, $path = '') { foreach ($required as $key => $value) { if (!array_key_exists($key, $actual)) { if ($key === '_all') { @@ -369,7 +434,7 @@ final class PhabricatorElasticFulltextStorageEngine if (!is_array($actual[$key])) { return false; } - if (!$this->check($actual[$key], $value)) { + if (!$this->check($actual[$key], $value, $path.'.'.$key)) { return false; } continue; @@ -403,19 +468,44 @@ final class PhabricatorElasticFulltextStorageEngine } public function initIndex() { + $host = $this->getHostForWrite(); if ($this->indexExists()) { - $this->executeRequest('/', array(), 'DELETE'); + $this->executeRequest($host, '/', array(), 'DELETE'); } $data = $this->getIndexConfiguration(); - $this->executeRequest('/', $data, 'PUT'); + $this->executeRequest($host, '/', $data, 'PUT'); } - private function executeRequest($path, array $data, $method = 'GET') { - $uri = new PhutilURI($this->uri); - $uri->setPath($this->index); - $uri->appendPath($path); - $data = json_encode($data); + public function getIndexStats(PhabricatorElasticSearchHost $host = null) { + if ($this->version < 2) { + return false; + } + if (!$host) { + $host = $this->getHostForRead(); + } + $uri = '/_stats/'; + $host = $this->getHostForRead(); + $res = $this->executeRequest($host, $uri, array()); + $stats = $res['indices'][$this->index]; + return array( + pht('Queries') => + idxv($stats, array('primaries', 'search', 'query_total')), + pht('Documents') => + idxv($stats, array('total', 'docs', 'count')), + pht('Deleted') => + idxv($stats, array('total', 'docs', 'deleted')), + pht('Storage Used') => + phutil_format_bytes(idxv($stats, + array('total', 'store', 'size_in_bytes'))), + ); + } + + private function executeRequest(PhabricatorElasticSearchHost $host, $path, + array $data, $method = 'GET') { + + $uri = $host->getURI($path); + $data = json_encode($data); $future = new HTTPSFuture($uri, $data); if ($method != 'GET') { $future->setMethod($method); @@ -423,19 +513,30 @@ final class PhabricatorElasticFulltextStorageEngine if ($this->getTimeout()) { $future->setTimeout($this->getTimeout()); } - list($body) = $future->resolvex(); + try { + list($body) = $future->resolvex(); + } catch (HTTPFutureResponseStatus $ex) { + if ($ex->isTimeout() || (int)$ex->getStatusCode() > 499) { + $host->didHealthCheck(false); + } + throw $ex; + } if ($method != 'GET') { return null; } try { - return phutil_json_decode($body); + $data = phutil_json_decode($body); + $host->didHealthCheck(true); + return $data; } catch (PhutilJSONParserException $ex) { + $host->didHealthCheck(false); throw new PhutilProxyException( pht('ElasticSearch server returned invalid JSON!'), $ex); } + } } diff --git a/src/applications/search/fulltextstorage/PhabricatorElasticSearchQueryBuilder.php b/src/applications/search/fulltextstorage/PhabricatorElasticSearchQueryBuilder.php new file mode 100644 index 0000000000..659660d813 --- /dev/null +++ b/src/applications/search/fulltextstorage/PhabricatorElasticSearchQueryBuilder.php @@ -0,0 +1,78 @@ +clauses; + if ($termkey == null) { + return $clauses; + } + if (isset($clauses[$termkey])) { + return $clauses[$termkey]; + } + return array(); + } + + public function getClauseCount($clausekey) { + if (isset($this->clauses[$clausekey])) { + return count($this->clauses[$clausekey]); + } else { + return 0; + } + } + + public function addExistsClause($field) { + return $this->addClause('filter', array( + 'exists' => array( + 'field' => $field, + ), + )); + } + + public function addTermsClause($field, $values) { + return $this->addClause('filter', array( + 'terms' => array( + $field => array_values($values), + ), + )); + } + + public function addMustClause($clause) { + return $this->addClause('must', $clause); + } + + public function addFilterClause($clause) { + return $this->addClause('filter', $clause); + } + + public function addShouldClause($clause) { + return $this->addClause('should', $clause); + } + + public function addMustNotClause($clause) { + return $this->addClause('must_not', $clause); + } + + public function addClause($clause, $terms) { + $this->clauses[$clause][] = $terms; + return $this; + } + + public function toArray() { + $clauses = $this->getClauses(); + return $clauses; + $cleaned = array(); + foreach ($clauses as $clause => $subclauses) { + if (is_array($subclauses) && count($subclauses) == 1) { + $cleaned[$clause] = array_shift($subclauses); + } else { + $cleaned[$clause] = $subclauses; + } + } + return $cleaned; + } + +} diff --git a/src/applications/search/fulltextstorage/PhabricatorFulltextStorageEngine.php b/src/applications/search/fulltextstorage/PhabricatorFulltextStorageEngine.php index beae237168..5e919258bd 100644 --- a/src/applications/search/fulltextstorage/PhabricatorFulltextStorageEngine.php +++ b/src/applications/search/fulltextstorage/PhabricatorFulltextStorageEngine.php @@ -7,6 +7,31 @@ */ abstract class PhabricatorFulltextStorageEngine extends Phobject { + protected $service; + + public function getHosts() { + return $this->service->getHosts(); + } + + public function setService(PhabricatorSearchService $service) { + $this->service = $service; + return $this; + } + + /** + * @return PhabricatorSearchService + */ + public function getService() { + return $this->service; + } + + /** + * Implementations must return a prototype host instance which is cloned + * by the PhabricatorSearchService infrastructure to configure each engine. + * @return PhabricatorSearchHost + */ + abstract public function getHostType(); + /* -( Engine Metadata )---------------------------------------------------- */ /** @@ -17,37 +42,6 @@ abstract class PhabricatorFulltextStorageEngine extends Phobject { */ abstract public function getEngineIdentifier(); - /** - * Prioritize this engine relative to other engines. - * - * Engines with a smaller priority number get an opportunity to write files - * first. Generally, lower-latency filestores should have lower priority - * numbers, and higher-latency filestores should have higher priority - * numbers. Setting priority to approximately the number of milliseconds of - * read latency will generally produce reasonable results. - * - * In conjunction with filesize limits, the goal is to store small files like - * profile images, thumbnails, and text snippets in lower-latency engines, - * and store large files in higher-capacity engines. - * - * @return float Engine priority. - * @task meta - */ - abstract public function getEnginePriority(); - - /** - * Return `true` if the engine is currently writable. - * - * Engines that are disabled or missing configuration should return `false` - * to prevent new writes. If writes were made with this engine in the past, - * the application may still try to perform reads. - * - * @return bool True if this engine can support new writes. - * @task meta - */ - abstract public function isEnabled(); - - /* -( Managing Documents )------------------------------------------------- */ /** @@ -83,6 +77,13 @@ abstract class PhabricatorFulltextStorageEngine extends Phobject { */ abstract public function indexExists(); + /** + * Implementations should override this method to return a dictionary of + * stats which are suitable for display in the admin UI. + */ + abstract public function getIndexStats(); + + /** * Is the index in a usable state? * @@ -100,39 +101,4 @@ abstract class PhabricatorFulltextStorageEngine extends Phobject { public function initIndex() {} -/* -( Loading Storage Engines )-------------------------------------------- */ - - /** - * @task load - */ - public static function loadAllEngines() { - return id(new PhutilClassMapQuery()) - ->setAncestorClass(__CLASS__) - ->setUniqueMethod('getEngineIdentifier') - ->setSortMethod('getEnginePriority') - ->execute(); - } - - /** - * @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/fulltextstorage/PhabricatorMySQLFulltextStorageEngine.php b/src/applications/search/fulltextstorage/PhabricatorMySQLFulltextStorageEngine.php index c30c74139e..72c49576f0 100644 --- a/src/applications/search/fulltextstorage/PhabricatorMySQLFulltextStorageEngine.php +++ b/src/applications/search/fulltextstorage/PhabricatorMySQLFulltextStorageEngine.php @@ -7,12 +7,8 @@ final class PhabricatorMySQLFulltextStorageEngine return 'mysql'; } - public function getEnginePriority() { - return 100; - } - - public function isEnabled() { - return true; + public function getHostType() { + return new PhabricatorMySQLSearchHost($this); } public function reindexAbstractDocument( @@ -415,4 +411,9 @@ final class PhabricatorMySQLFulltextStorageEngine public function indexExists() { return true; } + + public function getIndexStats() { + return false; + } + } diff --git a/src/applications/search/index/PhabricatorFulltextEngine.php b/src/applications/search/index/PhabricatorFulltextEngine.php index 64cbe4ebb5..9f20917b3f 100644 --- a/src/applications/search/index/PhabricatorFulltextEngine.php +++ b/src/applications/search/index/PhabricatorFulltextEngine.php @@ -40,8 +40,7 @@ abstract class PhabricatorFulltextEngine $extension->indexFulltextObject($object, $document); } - $storage_engine = PhabricatorFulltextStorageEngine::loadEngine(); - $storage_engine->reindexAbstractDocument($document); + PhabricatorSearchService::reindexAbstractDocument($document); } protected function newAbstractDocument($object) { diff --git a/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php index 4c35b61dd5..1b5da49e66 100644 --- a/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php @@ -13,27 +13,41 @@ final class PhabricatorSearchManagementInitWorkflow public function execute(PhutilArgumentParser $args) { $console = PhutilConsole::getConsole(); - $engine = PhabricatorFulltextStorageEngine::loadEngine(); - $work_done = false; - if (!$engine->indexExists()) { - $console->writeOut( - '%s', - pht('Index does not exist, creating...')); - $engine->initIndex(); + foreach (PhabricatorSearchService::getAllServices() as $service) { $console->writeOut( "%s\n", - pht('done.')); - $work_done = true; - } else if (!$engine->indexIsSane()) { - $console->writeOut( - '%s', - pht('Index exists but is incorrect, fixing...')); - $engine->initIndex(); - $console->writeOut( - "%s\n", - pht('done.')); - $work_done = true; + pht('Initializing search service "%s"', $service->getDisplayName())); + + try { + $host = $service->getAnyHostForRole('write'); + } catch (PhabricatorClusterNoHostForRoleException $e) { + // If there are no writable hosts for a given cluster, skip it + $console->writeOut("%s\n", $e->getMessage()); + continue; + } + + $engine = $host->getEngine(); + + if (!$engine->indexExists()) { + $console->writeOut( + '%s', + pht('Index does not exist, creating...')); + $engine->initIndex(); + $console->writeOut( + "%s\n", + pht('done.')); + $work_done = true; + } else if (!$engine->indexIsSane()) { + $console->writeOut( + '%s', + pht('Index exists but is incorrect, fixing...')); + $engine->initIndex(); + $console->writeOut( + "%s\n", + pht('done.')); + $work_done = true; + } } if ($work_done) { diff --git a/src/applications/search/query/PhabricatorSearchDocumentQuery.php b/src/applications/search/query/PhabricatorSearchDocumentQuery.php index 002c3364af..d4700904c9 100644 --- a/src/applications/search/query/PhabricatorSearchDocumentQuery.php +++ b/src/applications/search/query/PhabricatorSearchDocumentQuery.php @@ -73,10 +73,7 @@ final class PhabricatorSearchDocumentQuery $query = id(clone($this->savedQuery)) ->setParameter('offset', $this->getOffset()) ->setParameter('limit', $this->getRawResultLimit()); - - $engine = PhabricatorFulltextStorageEngine::loadEngine(); - - return $engine->executeSearch($query); + return PhabricatorSearchService::executeSearch($query); } public function getQueryApplicationClass() { diff --git a/src/docs/user/cluster/cluster_search.diviner b/src/docs/user/cluster/cluster_search.diviner new file mode 100644 index 0000000000..662abecbc3 --- /dev/null +++ b/src/docs/user/cluster/cluster_search.diviner @@ -0,0 +1,76 @@ +@title Cluster: Search +@group cluster + +Overview +======== + +You can configure phabricator to connect to one or more fulltext search clusters +running either Elasticsearch or MySQL. By default and without further +configuration, Phabricator will use MySQL for fulltext search. This will be +adequate for the vast majority of users. Installs with a very large number of +objects or specialized search needs can consider enabling Elasticsearch for +better scalability and potentially better search results. + +Configuring Search Services +=========================== + +To configure an Elasticsearch service, use the `cluster.search` configuration +option. A typical Elasticsearch configuration will probably look similar to +the following example: + +```lang=json +{ + "cluster.search": [ + { + "type": "elasticsearch", + "hosts": [ + { + "host": "127.0.0.1", + "roles": { "write": true, "read": true } + } + ], + "port": 9200, + "protocol": "http", + "path": "/phabricator", + "version": 5 + }, + ], +} +``` + +Supported Options +----------------- +| Key | Type |Comments| +|`type` | String |Engine type. Currently, 'elasticsearch' or 'mysql'| +|`protocol`| String |Either 'http' or 'https'| +|`port`| Int |The TCP port that Elasticsearch is bound to| +|`path`| String |The path portion of the url for phabricator's index.| +|`version`| Int |The version of Elasticsearch server. Supports either 2 or 5.| +|`hosts`| List |A list of one or more Elasticsearch host names / addresses.| + +Host Configuration +------------------ +Each search service must have one or more hosts associated with it. Each host +entry consists of a `host` key, a dictionary of roles and can optionally +override any of the options that are valid at the service level (see above). + +Currently supported roles are `read` and `write`. These can be individually +enabled or disabled on a per-host basis. A typical setup might include two +elasticsearch clusters in two separate datacenters. You can configure one +cluster for reads and both for writes. When one cluster is down for maintenance +you can simply swap the read role over to the backup cluster and then proceed +with maintenance without any service interruption. + +Monitoring Search Services +========================== + +You can monitor fulltext search in {nav Config > Search Servers}. This interface +shows you a quick overview of services and their health. + +The table on this page shows some basic stats for each configured service, +followed by the configuration and current status of each host. + +NOTE: This page runs its diagnostics //from the web server that is serving the +request//. If you are recovering from a disaster, the view this page shows +may be partial or misleading, and two requests served by different servers may +see different views of the cluster. diff --git a/src/infrastructure/cluster/PhabricatorDatabaseHealthRecord.php b/src/infrastructure/cluster/PhabricatorClusterServiceHealthRecord.php similarity index 89% rename from src/infrastructure/cluster/PhabricatorDatabaseHealthRecord.php rename to src/infrastructure/cluster/PhabricatorClusterServiceHealthRecord.php index 580b3f1b27..252c116653 100644 --- a/src/infrastructure/cluster/PhabricatorDatabaseHealthRecord.php +++ b/src/infrastructure/cluster/PhabricatorClusterServiceHealthRecord.php @@ -1,20 +1,19 @@ ref = $ref; + public function __construct($cache_key) { + $this->cacheKey = $cache_key; $this->readState(); } - /** * Is the database currently healthy? */ @@ -153,18 +152,13 @@ final class PhabricatorDatabaseHealthRecord } } - private function getHealthRecordCacheKey() { - $ref = $this->ref; - - $host = $ref->getHost(); - $port = $ref->getPort(); - - return "cluster.db.health({$host}, {$port})"; + public function getCacheKey() { + return $this->cacheKey; } private function readHealthRecord() { $cache = PhabricatorCaches::getSetupCache(); - $cache_key = $this->getHealthRecordCacheKey(); + $cache_key = $this->getCacheKey(); $health_record = $cache->getKey($cache_key); if (!is_array($health_record)) { @@ -180,7 +174,7 @@ final class PhabricatorDatabaseHealthRecord private function writeHealthRecord(array $record) { $cache = PhabricatorCaches::getSetupCache(); - $cache_key = $this->getHealthRecordCacheKey(); + $cache_key = $this->getCacheKey(); $cache->setKey($cache_key, $record); } diff --git a/src/infrastructure/cluster/PhabricatorDatabaseRef.php b/src/infrastructure/cluster/PhabricatorDatabaseRef.php index 337d139df8..5a32ef7c11 100644 --- a/src/infrastructure/cluster/PhabricatorDatabaseRef.php +++ b/src/infrastructure/cluster/PhabricatorDatabaseRef.php @@ -14,6 +14,7 @@ final class PhabricatorDatabaseRef const REPLICATION_SLOW = 'replica-slow'; const REPLICATION_NOT_REPLICATING = 'not-replicating'; + const KEY_HEALTH = 'cluster.db.health'; const KEY_REFS = 'cluster.db.refs'; const KEY_INDIVIDUAL = 'cluster.db.individual'; @@ -489,9 +490,18 @@ final class PhabricatorDatabaseRef return $this; } + private function getHealthRecordCacheKey() { + $host = $this->getHost(); + $port = $this->getPort(); + $key = self::KEY_HEALTH; + + return "{$key}({$host}, {$port})"; + } + public function getHealthRecord() { if (!$this->healthRecord) { - $this->healthRecord = new PhabricatorDatabaseHealthRecord($this); + $this->healthRecord = new PhabricatorClusterServiceHealthRecord( + $this->getHealthRecordCacheKey()); } return $this->healthRecord; } diff --git a/src/infrastructure/cluster/PhabricatorClusterDatabasesConfigOptionType.php b/src/infrastructure/cluster/config/PhabricatorClusterDatabasesConfigOptionType.php similarity index 100% rename from src/infrastructure/cluster/PhabricatorClusterDatabasesConfigOptionType.php rename to src/infrastructure/cluster/config/PhabricatorClusterDatabasesConfigOptionType.php diff --git a/src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php b/src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php new file mode 100644 index 0000000000..4a5f7ea6c5 --- /dev/null +++ b/src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php @@ -0,0 +1,79 @@ + $spec) { + if (!is_array($spec)) { + throw new Exception( + pht( + 'Search cluster configuration is not valid: each entry in the '. + 'list must be a dictionary describing a search service, but '. + 'the value with index "%s" is not a dictionary.', + $index)); + } + + try { + PhutilTypeSpec::checkMap( + $spec, + array( + 'type' => 'string', + 'hosts' => 'optional list>', + 'roles' => 'optional map', + 'port' => 'optional int', + 'protocol' => 'optional string', + 'path' => 'optional string', + 'version' => 'optional int', + )); + } catch (Exception $ex) { + throw new Exception( + pht( + 'Search engine configuration has an invalid service '. + 'specification (at index "%s"): %s.', + $index, + $ex->getMessage())); + } + + if (!array_key_exists($spec['type'], $engines)) { + throw new Exception( + pht('Invalid search engine type: %s. Valid types include: %s', + $spec['type'], + implode(', ', array_keys($engines)))); + } + + if (isset($spec['hosts'])) { + foreach ($spec['hosts'] as $hostindex => $host) { + try { + PhutilTypeSpec::checkMap( + $host, + array( + 'host' => 'string', + 'roles' => 'optional map', + 'port' => 'optional int', + 'protocol' => 'optional string', + 'path' => 'optional string', + 'version' => 'optional int', + )); + } catch (Exception $ex) { + throw new Exception( + pht( + 'Search cluster configuration has an invalid host '. + 'specification (at index "%s"): %s.', + $hostindex, + $ex->getMessage())); + } + } + } + } + } +} diff --git a/src/infrastructure/cluster/PhabricatorClusterException.php b/src/infrastructure/cluster/exception/PhabricatorClusterException.php similarity index 100% rename from src/infrastructure/cluster/PhabricatorClusterException.php rename to src/infrastructure/cluster/exception/PhabricatorClusterException.php diff --git a/src/infrastructure/cluster/PhabricatorClusterExceptionHandler.php b/src/infrastructure/cluster/exception/PhabricatorClusterExceptionHandler.php similarity index 100% rename from src/infrastructure/cluster/PhabricatorClusterExceptionHandler.php rename to src/infrastructure/cluster/exception/PhabricatorClusterExceptionHandler.php diff --git a/src/infrastructure/cluster/PhabricatorClusterImpossibleWriteException.php b/src/infrastructure/cluster/exception/PhabricatorClusterImpossibleWriteException.php similarity index 100% rename from src/infrastructure/cluster/PhabricatorClusterImpossibleWriteException.php rename to src/infrastructure/cluster/exception/PhabricatorClusterImpossibleWriteException.php diff --git a/src/infrastructure/cluster/PhabricatorClusterImproperWriteException.php b/src/infrastructure/cluster/exception/PhabricatorClusterImproperWriteException.php similarity index 100% rename from src/infrastructure/cluster/PhabricatorClusterImproperWriteException.php rename to src/infrastructure/cluster/exception/PhabricatorClusterImproperWriteException.php diff --git a/src/infrastructure/cluster/exception/PhabricatorClusterNoHostForRoleException.php b/src/infrastructure/cluster/exception/PhabricatorClusterNoHostForRoleException.php new file mode 100644 index 0000000000..f7e9cb5550 --- /dev/null +++ b/src/infrastructure/cluster/exception/PhabricatorClusterNoHostForRoleException.php @@ -0,0 +1,10 @@ +setRoles(idx($config, 'roles', $this->getRoles())) + ->setHost(idx($config, 'host', $this->host)) + ->setPort(idx($config, 'port', $this->port)) + ->setProtocol(idx($config, 'protocol', $this->protocol)) + ->setPath(idx($config, 'path', $this->path)) + ->setVersion(idx($config, 'version', $this->version)); + return $this; + } + + public function getDisplayName() { + return pht('ElasticSearch'); + } + + public function getStatusViewColumns() { + return array( + pht('Protocol') => $this->getProtocol(), + pht('Host') => $this->getHost(), + pht('Port') => $this->getPort(), + pht('Index Path') => $this->getPath(), + pht('Elastic Version') => $this->getVersion(), + pht('Roles') => implode(', ', array_keys($this->getRoles())), + ); + } + + public function setProtocol($protocol) { + $this->protocol = $protocol; + return $this; + } + + public function getProtocol() { + return $this->protocol; + } + + public function setPath($path) { + $this->path = $path; + return $this; + } + + public function getPath() { + return $this->path; + } + + public function setVersion($version) { + $this->version = $version; + return $this; + } + + public function getVersion() { + return $this->version; + } + + public function getURI($to_path = null) { + $uri = id(new PhutilURI('http://'.$this->getHost())) + ->setProtocol($this->getProtocol()) + ->setPort($this->getPort()) + ->setPath($this->getPath()); + + if ($to_path) { + $uri->appendPath($to_path); + } + return $uri; + } + + public function getConnectionStatus() { + $status = $this->getEngine()->indexIsSane($this); + return $status ? parent::STATUS_OKAY : parent::STATUS_FAIL; + } + +} diff --git a/src/infrastructure/cluster/search/PhabricatorMySQLSearchHost.php b/src/infrastructure/cluster/search/PhabricatorMySQLSearchHost.php new file mode 100644 index 0000000000..ced23cd542 --- /dev/null +++ b/src/infrastructure/cluster/search/PhabricatorMySQLSearchHost.php @@ -0,0 +1,34 @@ +setRoles(idx($config, 'roles', + array('read' => true, 'write' => true))); + return $this; + } + + public function getDisplayName() { + return 'MySQL'; + } + + public function getStatusViewColumns() { + return array( + pht('Protocol') => 'mysql', + pht('Roles') => implode(', ', array_keys($this->getRoles())), + ); + } + + public function getProtocol() { + return 'mysql'; + } + + public function getConnectionStatus() { + PhabricatorDatabaseRef::queryAll(); + $ref = PhabricatorDatabaseRef::getMasterDatabaseRefForApplication('search'); + $status = $ref->getConnectionStatus(); + return $status; + } + +} diff --git a/src/infrastructure/cluster/search/PhabricatorSearchHost.php b/src/infrastructure/cluster/search/PhabricatorSearchHost.php new file mode 100644 index 0000000000..834e786789 --- /dev/null +++ b/src/infrastructure/cluster/search/PhabricatorSearchHost.php @@ -0,0 +1,163 @@ +engine = $engine; + } + + public function setDisabled($disabled) { + $this->disabled = $disabled; + return $this; + } + + public function getDisabled() { + return $this->disabled; + } + + /** + * @return PhabricatorFulltextStorageEngine + */ + public function getEngine() { + return $this->engine; + } + + public function isWritable() { + return $this->hasRole('write'); + } + + public function isReadable() { + return $this->hasRole('read'); + } + + public function hasRole($role) { + return isset($this->roles[$role]) && $this->roles[$role] === true; + } + + public function setRoles(array $roles) { + foreach ($roles as $role => $val) { + $this->roles[$role] = $val; + } + return $this; + } + + public function getRoles() { + $roles = array(); + foreach ($this->roles as $key => $val) { + if ($val) { + $roles[$key] = $val; + } + } + return $roles; + } + + public function setPort($value) { + $this->port = $value; + return $this; + } + + public function getPort() { + return $this->port; + } + + public function setHost($value) { + $this->host = $value; + return $this; + } + + public function getHost() { + return $this->host; + } + + + public function getHealthRecordCacheKey() { + $host = $this->getHost(); + $port = $this->getPort(); + $key = self::KEY_HEALTH; + + return "{$key}({$host}, {$port})"; + } + +/** + * @return PhabricatorClusterServiceHealthRecord + */ + public function getHealthRecord() { + if (!$this->healthRecord) { + $this->healthRecord = new PhabricatorClusterServiceHealthRecord( + $this->getHealthRecordCacheKey()); + } + return $this->healthRecord; + } + + public function didHealthCheck($reachable) { + $record = $this->getHealthRecord(); + $should_check = $record->getShouldCheck(); + + if ($should_check) { + $record->didHealthCheck($reachable); + } + } + + /** + * @return string[] Get a list of fields to show in the status overview UI + */ + abstract public function getStatusViewColumns(); + + abstract public function getConnectionStatus(); + + public static function reindexAbstractDocument( + PhabricatorSearchAbstractDocument $doc) { + + $services = self::getAllServices(); + $indexed = 0; + foreach (self::getWritableHostForEachService() as $host) { + $host->getEngine()->reindexAbstractDocument($doc); + $indexed++; + } + if ($indexed == 0) { + throw new PhabricatorClusterNoHostForRoleException('write'); + } + } + + public static function executeSearch(PhabricatorSavedQuery $query) { + $services = self::getAllServices(); + foreach ($services as $service) { + $hosts = $service->getAllHostsForRole('read'); + // try all hosts until one succeeds + foreach ($hosts as $host) { + $last_exception = null; + try { + $res = $host->getEngine()->executeSearch($query); + // return immediately if we get results without an exception + $host->didHealthCheck(true); + return $res; + } catch (Exception $ex) { + // try each server in turn, only throw if none succeed + $last_exception = $ex; + $host->didHealthCheck(false); + } + } + } + if ($last_exception) { + throw $last_exception; + } + return $res; + } + +} diff --git a/src/infrastructure/cluster/search/PhabricatorSearchService.php b/src/infrastructure/cluster/search/PhabricatorSearchService.php new file mode 100644 index 0000000000..20c0664456 --- /dev/null +++ b/src/infrastructure/cluster/search/PhabricatorSearchService.php @@ -0,0 +1,259 @@ +engine = $engine; + $this->hostType = $engine->getHostType(); + } + + /** + * @throws Exception + */ + public function newHost($config) { + $host = clone($this->hostType); + $host_config = $this->config + $config; + $host->setConfig($host_config); + $this->hosts[] = $host; + return $host; + } + + public function getEngine() { + return $this->engine; + } + + public function getDisplayName() { + return $this->hostType->getDisplayName(); + } + + public function getStatusViewColumns() { + return $this->hostType->getStatusViewColumns(); + } + + public function setConfig($config) { + $this->config = $config; + + if (!isset($config['hosts'])) { + $config['hosts'] = array( + array( + 'host' => idx($config, 'host'), + 'port' => idx($config, 'port'), + 'protocol' => idx($config, 'protocol'), + 'roles' => idx($config, 'roles'), + ), + ); + } + foreach ($config['hosts'] as $host) { + $this->newHost($host); + } + + } + + public function getConfig() { + return $this->config; + } + + public function setDisabled($disabled) { + $this->disabled = $disabled; + return $this; + } + + public function getDisabled() { + return $this->disabled; + } + + public static function getConnectionStatusMap() { + return array( + self::STATUS_OKAY => array( + 'icon' => 'fa-exchange', + 'color' => 'green', + 'label' => pht('Okay'), + ), + self::STATUS_FAIL => array( + 'icon' => 'fa-times', + 'color' => 'red', + 'label' => pht('Failed'), + ), + ); + } + + public function isWritable() { + return $this->hasRole('write'); + } + + public function isReadable() { + return $this->hasRole('read'); + } + + public function hasRole($role) { + return isset($this->roles[$role]) && $this->roles[$role] === true; + } + + public function setRoles(array $roles) { + foreach ($roles as $role => $val) { + if ($val === false && isset($this->roles[$role])) { + unset($this->roles[$role]); + } else { + $this->roles[$role] = $val; + } + } + return $this; + } + + public function getRoles() { + return $this->roles; + } + + public function getPort() { + return idx($this->config, 'port'); + } + + public function getProtocol() { + return idx($this->config, 'protocol'); + } + + + public function getVersion() { + return idx($this->config, 'version'); + } + + public function getHosts() { + return $this->hosts; + } + + + /** + * Get a random host reference with the specified role, skipping hosts which + * failed recent health checks. + * @throws PhabricatorClusterNoHostForRoleException if no healthy hosts match. + * @return PhabricatorSearchHost + */ + public function getAnyHostForRole($role) { + $hosts = $this->getAllHostsForRole($role); + shuffle($hosts); + foreach ($hosts as $host) { + $health = $host->getHealthRecord(); + if ($health->getIsHealthy()) { + return $host; + } + } + throw new PhabricatorClusterNoHostForRoleException($role); + } + + + /** + * Get all configured hosts for this service which have the specified role. + * @return PhabricatorSearchHost[] + */ + public function getAllHostsForRole($role) { + $hosts = array(); + foreach ($this->hosts as $host) { + if ($host->hasRole($role)) { + $hosts[] = $host; + } + } + return $hosts; + } + + /** + * Get a reference to all configured fulltext search cluster services + * @return PhabricatorSearchService[] + */ + public static function getAllServices() { + $cache = PhabricatorCaches::getRequestCache(); + + $refs = $cache->getKey(self::KEY_REFS); + if (!$refs) { + $refs = self::newRefs(); + $cache->setKey(self::KEY_REFS, $refs); + } + + return $refs; + } + + /** + * Load all valid PhabricatorFulltextStorageEngine subclasses + */ + public static function loadAllFulltextStorageEngines() { + return id(new PhutilClassMapQuery()) + ->setAncestorClass('PhabricatorFulltextStorageEngine') + ->setUniqueMethod('getEngineIdentifier') + ->execute(); + } + + /** + * Create instances of PhabricatorSearchService based on configuration + * @return PhabricatorSearchService[] + */ + public static function newRefs() { + $services = PhabricatorEnv::getEnvConfig('cluster.search'); + $engines = self::loadAllFulltextStorageEngines(); + $refs = array(); + + foreach ($services as $config) { + $engine = $engines[$config['type']]; + $cluster = new self($engine); + $cluster->setConfig($config); + $engine->setService($cluster); + $refs[] = $cluster; + } + + return $refs; + } + + + /** + * (re)index the document: attempt to pass the document to all writable + * fulltext search hosts + * @throws PhabricatorClusterNoHostForRoleException + */ + public static function reindexAbstractDocument( + PhabricatorSearchAbstractDocument $doc) { + $indexed = 0; + foreach (self::getAllServices() as $service) { + $service->getEngine()->reindexAbstractDocument($doc); + $indexed++; + } + if ($indexed == 0) { + throw new PhabricatorClusterNoHostForRoleException('write'); + } + } + + /** + * Execute a full-text query and return a list of PHIDs of matching objects. + * @return string[] + * @throws PhutilAggregateException + */ + public static function executeSearch(PhabricatorSavedQuery $query) { + $services = self::getAllServices(); + $exceptions = array(); + foreach ($services as $service) { + $engine = $service->getEngine(); + // try all hosts until one succeeds + try { + $res = $engine->executeSearch($query); + // return immediately if we get results without an exception + return $res; + } catch (Exception $ex) { + $exceptions[] = $ex; + } + } + throw new PhutilAggregateException('All search engines failed:', + $exceptions); + } + +} From 76404c5fdb6a51f7b04ca3afd1b81239be2d88e7 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 27 Mar 2017 09:19:12 -0700 Subject: [PATCH 04/39] Cleaner fullscreen / preview states for Remarkup bar Summary: General CSS and usability touchup of the Remarkup bar states for fullscreen and preview. Larger fonts, more spacing, some hint of the underlying page. Disable buttons that can't be used in preview mode. Test Plan: Formal test coming with mobile, browsers. This is a kick the tires upload. {F4283448} Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D17563 --- resources/celerity/map.php | 37 +-- .../control/PhabricatorRemarkupControl.php | 9 - webroot/rsrc/css/core/remarkup.css | 217 ++++++++++++------ webroot/rsrc/css/phui/phui-comment-form.css | 2 +- .../behavior-phabricator-remarkup-assist.js | 5 + 5 files changed, 177 insertions(+), 93 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 4ca4c327f9..4d9406ac5f 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,8 +9,8 @@ return array( 'names' => array( 'conpherence.pkg.css' => '82aca405', 'conpherence.pkg.js' => '6249a1cf', - 'core.pkg.css' => 'dc689e29', - 'core.pkg.js' => '1fa7c0c5', + 'core.pkg.css' => 'acd257dc', + 'core.pkg.js' => '021685f1', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '90b30783', 'differential.pkg.js' => 'ddfeb49b', @@ -108,7 +108,7 @@ return array( 'rsrc/css/application/tokens/tokens.css' => '3d0f239e', 'rsrc/css/application/uiexample/example.css' => '528b19de', 'rsrc/css/core/core.css' => '9f4cb463', - 'rsrc/css/core/remarkup.css' => '2d793c5b', + 'rsrc/css/core/remarkup.css' => '17c0fb37', 'rsrc/css/core/syntax.css' => '769d3498', 'rsrc/css/core/z-index.css' => '5e72c4e0', 'rsrc/css/diviner/diviner-shared.css' => '896f1d43', @@ -136,7 +136,7 @@ return array( 'rsrc/css/phui/phui-button.css' => '14bfba79', 'rsrc/css/phui/phui-chart.css' => '6bf6f78e', 'rsrc/css/phui/phui-cms.css' => '504b4b23', - 'rsrc/css/phui/phui-comment-form.css' => '48fbd65d', + 'rsrc/css/phui/phui-comment-form.css' => '7d903c2d', 'rsrc/css/phui/phui-comment-panel.css' => 'f50152ad', 'rsrc/css/phui/phui-crumbs-view.css' => '6ece3bbb', 'rsrc/css/phui/phui-curtain-view.css' => '947bf1a4', @@ -503,7 +503,7 @@ return array( 'rsrc/js/core/behavior-object-selector.js' => 'e0ec7f2f', 'rsrc/js/core/behavior-oncopy.js' => '2926fff2', 'rsrc/js/core/behavior-phabricator-nav.js' => '08675c6d', - 'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => '0c61d4e3', + 'rsrc/js/core/behavior-phabricator-remarkup-assist.js' => 'a0777ea3', 'rsrc/js/core/behavior-read-only-warning.js' => 'ba158207', 'rsrc/js/core/behavior-refresh-csrf.js' => 'ab2f381b', 'rsrc/js/core/behavior-remarkup-preview.js' => '4b700e9e', @@ -664,7 +664,7 @@ return array( 'javelin-behavior-phabricator-notification-example' => '8ce821c5', 'javelin-behavior-phabricator-object-selector' => 'e0ec7f2f', 'javelin-behavior-phabricator-oncopy' => '2926fff2', - 'javelin-behavior-phabricator-remarkup-assist' => '0c61d4e3', + 'javelin-behavior-phabricator-remarkup-assist' => 'a0777ea3', 'javelin-behavior-phabricator-reveal-content' => '60821bc7', 'javelin-behavior-phabricator-search-typeahead' => '06c32383', 'javelin-behavior-phabricator-show-older-transactions' => '94c65b72', @@ -793,7 +793,7 @@ return array( 'phabricator-object-selector-css' => '85ee8ce6', 'phabricator-phtize' => 'd254d646', 'phabricator-prefab' => '8d40ae75', - 'phabricator-remarkup-css' => '2d793c5b', + 'phabricator-remarkup-css' => '17c0fb37', 'phabricator-search-results-css' => '64ad079a', 'phabricator-shaped-request' => '7cbe244b', 'phabricator-slowvote-css' => 'a94b7230', @@ -836,7 +836,7 @@ return array( 'phui-calendar-month-css' => '8e10e92c', 'phui-chart-css' => '6bf6f78e', 'phui-cms-css' => '504b4b23', - 'phui-comment-form-css' => '48fbd65d', + 'phui-comment-form-css' => '7d903c2d', 'phui-comment-panel-css' => 'f50152ad', 'phui-crumbs-view-css' => '6ece3bbb', 'phui-curtain-view-css' => '947bf1a4', @@ -988,16 +988,6 @@ return array( 'javelin-dom', 'javelin-router', ), - '0c61d4e3' => array( - 'javelin-behavior', - 'javelin-stratcom', - 'javelin-dom', - 'phabricator-phtize', - 'phabricator-textareautils', - 'javelin-workflow', - 'javelin-vector', - 'phuix-autocomplete', - ), '0f764c35' => array( 'javelin-install', 'javelin-util', @@ -1705,6 +1695,17 @@ return array( 'javelin-dom', 'javelin-vector', ), + 'a0777ea3' => array( + 'javelin-behavior', + 'javelin-stratcom', + 'javelin-dom', + 'phabricator-phtize', + 'phabricator-textareautils', + 'javelin-workflow', + 'javelin-vector', + 'phuix-autocomplete', + 'javelin-mask', + ), 'a0b57eb8' => array( 'javelin-behavior', 'javelin-dom', diff --git a/src/view/form/control/PhabricatorRemarkupControl.php b/src/view/form/control/PhabricatorRemarkupControl.php index 35a1bcc8df..52fb44617d 100644 --- a/src/view/form/control/PhabricatorRemarkupControl.php +++ b/src/view/form/control/PhabricatorRemarkupControl.php @@ -172,11 +172,6 @@ final class PhabricatorRemarkupControl extends AphrontFormTextAreaControl { 'align' => 'right', ); - $actions[] = array( - 'spacer' => true, - 'align' => 'right', - ); - $actions['fa-book'] = array( 'tip' => pht('Help'), 'align' => 'right', @@ -200,10 +195,6 @@ final class PhabricatorRemarkupControl extends AphrontFormTextAreaControl { } if ($mode_actions) { - $actions[] = array( - 'spacer' => true, - 'align' => 'right', - ); $actions += $mode_actions; } diff --git a/webroot/rsrc/css/core/remarkup.css b/webroot/rsrc/css/core/remarkup.css index 03479398b3..a986409227 100644 --- a/webroot/rsrc/css/core/remarkup.css +++ b/webroot/rsrc/css/core/remarkup.css @@ -558,9 +558,13 @@ var.remarkup-assist-textarea { .remarkup-assist-button { display: block; - margin-top: 2px; - padding: 4px 5px; + margin-top: 4px; + height: 20px; + padding: 2px 5px 3px; + line-height: 18px; + width: 16px; float: left; + border-radius: 3px; } .remarkup-assist-button:hover .phui-icon-view.phui-font-fa { @@ -617,37 +621,6 @@ var.remarkup-assist-textarea { opacity: 1.0; } -.remarkup-control-fullscreen-mode { - position: fixed; - top: -1px; - bottom: -1px; - left: -1px; - right: -1px; -} - -.remarkup-control-fullscreen-mode textarea.remarkup-assist-textarea { - position: absolute; - top: 32px; - left: 0; - right: 0; - bottom: 0; - /* NOTE: This doesn't work in Firefox, there's a JS behavior to correct it. */ - height: auto; - border-width: 1px 0 0 0; - outline: none; - resize: none; - background: #fff !important; -} - -.remarkup-control-fullscreen-mode textarea.remarkup-assist-textarea:focus { - border-color: none; - box-shadow: none; -} - -.remarkup-control-fullscreen-mode .remarkup-assist-button .fa-arrows-alt { - color: {$sky}; -} - .phabricator-image-macro-hero { margin: auto; max-width: 95%; @@ -673,42 +646,12 @@ var.remarkup-assist-textarea { padding: 0 4px; } -.remarkup-inline-preview { - display: block; - position: relative; - background: #fff; - overflow-y: auto; - box-sizing: border-box; - width: 100%; - border: 1px solid {$sky}; - resize: vertical; - padding: 4px 6px; -} - -.remarkup-control-fullscreen-mode .remarkup-inline-preview { - resize: none; -} - -.remarkup-inline-preview * { - resize: none; -} - -.remarkup-assist-button.preview-active { - background: {$sky}; -} - -.remarkup-assist-button.preview-active .phui-icon-view { - color: #ffffff; -} - -.remarkup-assist-button.preview-active:hover .phui-icon-view { - color: {$lightsky}; -} - .device .remarkup-assist-nodevice { display: none; } +/* - Autocomplete ----------------------------------------------------------- */ + .phuix-autocomplete { position: absolute; width: 300px; @@ -764,6 +707,9 @@ var.remarkup-assist-textarea { color: #000; } + +/* - Pinned ----------------------------------------------------------------- */ + .phui-box.phui-object-box.phui-comment-form-view.remarkup-assist-pinned { position: fixed; background-color: #ffffff; @@ -783,3 +729,144 @@ var.remarkup-assist-textarea { .remarkup-assist-pinned-spacer { position: relative; } + + +/* - Preview ---------------------------------------------------------------- */ + +.remarkup-inline-preview { + display: block; + position: relative; + background: #fff; + overflow-y: auto; + box-sizing: border-box; + width: 100%; + resize: vertical; + padding: 8px; + border: 1px solid {$lightblueborder}; + border-top: none; + -webkit-font-smoothing: antialiased; +} + +.remarkup-control-fullscreen-mode .remarkup-inline-preview { + resize: none; +} + +.remarkup-inline-preview * { + resize: none; +} + +.remarkup-assist-button.preview-active { + background: {$sky}; +} + +.remarkup-assist-button.preview-active .phui-icon-view { + color: #fff; +} + +.remarkup-assist-button.preview-active:hover { + text-decoration: none; +} + +.remarkup-assist-button.preview-active:hover .phui-icon-view { + color: #fff; +} + +.remarkup-preview-active .remarkup-assist, +.remarkup-preview-active .remarkup-assist-separator { + opacity: .2; + transition: all 100ms cubic-bezier(0.250, 0.250, 0.750, 0.750); + transition-timing-function: cubic-bezier(0.250, 0.250, 0.750, 0.750); +} + +.remarkup-preview-active .remarkup-assist-button { + pointer-events: none; + cursor: default; +} + +.remarkup-preview-active .remarkup-assist-button.preview-active { + pointer-events: inherit; + cursor: pointer; +} + +.remarkup-preview-active .remarkup-assist.fa-eye { + opacity: 1; + transition: all 100ms cubic-bezier(0.250, 0.250, 0.750, 0.750); + transition-timing-function: cubic-bezier(0.250, 0.250, 0.750, 0.750); +} + + +/* - Fullscreen ------------------------------------------------------------- */ + +.remarkup-fullscreen-mode { + overflow: hidden; +} + +.remarkup-control-fullscreen-mode { + position: fixed; + border: none; + top: 32px; + bottom: 32px; + left: 64px; + right: 64px; + border-radius: 3px; + box-shadow: 0px 4px 32px #555; +} + +.remarkup-control-fullscreen-mode .remarkup-assist-button { + padding: 1px 6px 4px; + font-size: 15px; +} + +.remarkup-control-fullscreen-mode .remarkup-assist-button .remarkup-assist { + height: 16px; + width: 16px; +} + +.aphront-form-input .remarkup-control-fullscreen-mode .remarkup-assist-bar { + border: none; + border-top-left-radius: 3px; + border-top-right-radius: 3px; + height: 32px; + padding: 4px 8px; + background: {$bluebackground}; +} + +.aphront-form-control .remarkup-control-fullscreen-mode + textarea.remarkup-assist-textarea { + position: absolute; + top: 39px; + left: 0; + right: 0; + height: calc(100% - 36px) !important; + padding: 16px; + font-size: {$biggerfontsize}; + line-height: 1.51em; + border-width: 1px 0 0 0; + outline: none; + resize: none; + background: #fff !important; +} + +.remarkup-control-fullscreen-mode textarea.remarkup-assist-textarea:focus { + border-color: {$thinblueborder}; + box-shadow: none; +} + +.remarkup-control-fullscreen-mode .remarkup-inline-preview { + font-size: {$biggerfontsize}; + border: none; + padding: 16px; + border-bottom-left-radius: 3px; + border-bottom-right-radius: 3px; +} + +.remarkup-control-fullscreen-mode .remarkup-assist-button .fa-arrows-alt { + color: {$sky}; +} + +.device-phone .remarkup-control-fullscreen-mode { + top: 0; + bottom: 0; + left: 0; + right: 0; +} diff --git a/webroot/rsrc/css/phui/phui-comment-form.css b/webroot/rsrc/css/phui/phui-comment-form.css index 3db0a82924..a49d033c3e 100644 --- a/webroot/rsrc/css/phui/phui-comment-form.css +++ b/webroot/rsrc/css/phui/phui-comment-form.css @@ -62,7 +62,7 @@ body.device .phui-box.phui-object-box.phui-comment-form-view { border-color: {$lightblueborder}; border-top: 1px solid {$thinblueborder}; padding: 8px; - height: 10em; + height: 12em; background-color: rgba({$alphablue},.02); } diff --git a/webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js b/webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js index 6e89a18d88..f4cbc4f4fa 100644 --- a/webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js +++ b/webroot/rsrc/js/core/behavior-phabricator-remarkup-assist.js @@ -8,6 +8,7 @@ * javelin-workflow * javelin-vector * phuix-autocomplete + * javelin-mask */ JX.behavior('phabricator-remarkup-assist', function(config) { @@ -39,6 +40,7 @@ JX.behavior('phabricator-remarkup-assist', function(config) { if (edit_mode == 'fa-arrows-alt') { JX.DOM.alterClass(edit_root, 'remarkup-control-fullscreen-mode', false); JX.DOM.alterClass(document.body, 'remarkup-fullscreen-mode', false); + JX.Mask.hide('jx-light-mask'); } area.style.height = ''; @@ -59,6 +61,7 @@ JX.behavior('phabricator-remarkup-assist', function(config) { if (mode == 'fa-arrows-alt') { JX.DOM.alterClass(edit_root, 'remarkup-control-fullscreen-mode', true); JX.DOM.alterClass(document.body, 'remarkup-fullscreen-mode', true); + JX.Mask.show('jx-light-mask'); // If we're in preview mode, expand the preview to full-size. if (preview) { @@ -275,6 +278,7 @@ JX.behavior('phabricator-remarkup-assist', function(config) { area.parentNode.insertBefore(preview, area); JX.DOM.alterClass(button, 'preview-active', true); + JX.DOM.alterClass(root, 'remarkup-preview-active', true); resize_preview(); JX.DOM.hide(area); @@ -286,6 +290,7 @@ JX.behavior('phabricator-remarkup-assist', function(config) { preview = null; JX.DOM.alterClass(button, 'preview-active', false); + JX.DOM.alterClass(root, 'remarkup-preview-active', false); } break; case 'fa-thumb-tack': From 9e2ab4f80e8c87b2cdb12b988d500ba0a5cc920b Mon Sep 17 00:00:00 2001 From: Chad Little Date: Mon, 27 Mar 2017 10:23:08 -0700 Subject: [PATCH 05/39] Scope syntax css rules to direct descendants only in diffs Summary: Fixes T11641. We're overbroad here (and this may need more scoping?) but this seems to resolve the immediate issue. Test Plan: Upload a few diffs and ask disabled accounts to comment on them inline. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T11641 Differential Revision: https://secure.phabricator.com/D17565 --- resources/celerity/map.php | 12 ++++++------ webroot/rsrc/css/core/syntax.css | 2 +- 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 4d9406ac5f..8a4146b0a3 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '82aca405', 'conpherence.pkg.js' => '6249a1cf', - 'core.pkg.css' => 'acd257dc', + 'core.pkg.css' => '87c434ee', 'core.pkg.js' => '021685f1', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '90b30783', @@ -109,7 +109,7 @@ return array( 'rsrc/css/application/uiexample/example.css' => '528b19de', 'rsrc/css/core/core.css' => '9f4cb463', 'rsrc/css/core/remarkup.css' => '17c0fb37', - 'rsrc/css/core/syntax.css' => '769d3498', + 'rsrc/css/core/syntax.css' => 'cae95e89', 'rsrc/css/core/z-index.css' => '5e72c4e0', 'rsrc/css/diviner/diviner-shared.css' => '896f1d43', 'rsrc/css/font/font-awesome.css' => 'e838e088', @@ -903,7 +903,7 @@ return array( 'sprite-login-css' => '587d92d7', 'sprite-tokens-css' => '9cdfd599', 'syntax-default-css' => '9923583c', - 'syntax-highlighting-css' => '769d3498', + 'syntax-highlighting-css' => 'cae95e89', 'tokens-css' => '3d0f239e', 'typeahead-browse-css' => '8904346a', 'unhandled-exception-css' => '4c96257a', @@ -1436,9 +1436,6 @@ return array( 'phabricator-shaped-request', 'conpherence-thread-manager', ), - '769d3498' => array( - 'syntax-default-css', - ), '76b9fc3e' => array( 'javelin-behavior', 'javelin-stratcom', @@ -2000,6 +1997,9 @@ return array( 'phabricator-title', 'phabricator-favicon', ), + 'cae95e89' => array( + 'syntax-default-css', + ), 'ccf1cbf8' => array( 'javelin-install', 'javelin-dom', diff --git a/webroot/rsrc/css/core/syntax.css b/webroot/rsrc/css/core/syntax.css index 5de3b5d4a4..a0b84ea2b3 100644 --- a/webroot/rsrc/css/core/syntax.css +++ b/webroot/rsrc/css/core/syntax.css @@ -11,7 +11,7 @@ margin-right: 1px; } -.remarkup-code td span { +.remarkup-code td > span { display: inline; word-break: break-all; } From 9e2f263bb49c3fdc433fbec63ca849ff9e1fa2b6 Mon Sep 17 00:00:00 2001 From: Mukunda Modell Date: Tue, 28 Mar 2017 07:58:22 +0000 Subject: [PATCH 06/39] Add repositories to fulltext search index. Summary: This implements a simplistic `PhabricatorRepositoryFulltextEngine` Currently only the repository name, description, timestamps and status are indexed. Note: I had to change the `search index` workflow to disambiguate PhabricatorRepository from PhabricatorRepositoryCommit Test Plan: * ran `./bin/search index --type PhabricatorRepository --force` * searched for some repositories. Saw reasonable results matching on either title or description. * Edited a repository in the web ui * Added unique key words to the repo description. * I was then able to find that repo by searching for the new keywords. Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: Korvin Tags: #search, #diffusion Differential Revision: https://secure.phabricator.com/D17300 --- src/__phutil_library_map__.php | 3 +++ .../PhabricatorRepositoryFulltextEngine.php | 27 +++++++++++++++++++ .../storage/PhabricatorRepository.php | 10 ++++++- ...abricatorSearchManagementIndexWorkflow.php | 6 +++++ 4 files changed, 45 insertions(+), 1 deletion(-) create mode 100644 src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 7c233cb93a..5d9e3de268 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3660,6 +3660,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryDiscoveryEngine' => 'applications/repository/engine/PhabricatorRepositoryDiscoveryEngine.php', 'PhabricatorRepositoryEditor' => 'applications/repository/editor/PhabricatorRepositoryEditor.php', 'PhabricatorRepositoryEngine' => 'applications/repository/engine/PhabricatorRepositoryEngine.php', + 'PhabricatorRepositoryFulltextEngine' => 'applications/repository/search/PhabricatorRepositoryFulltextEngine.php', 'PhabricatorRepositoryGitCommitChangeParserWorker' => 'applications/repository/worker/commitchangeparser/PhabricatorRepositoryGitCommitChangeParserWorker.php', 'PhabricatorRepositoryGitCommitMessageParserWorker' => 'applications/repository/worker/commitmessageparser/PhabricatorRepositoryGitCommitMessageParserWorker.php', 'PhabricatorRepositoryGitLFSRef' => 'applications/repository/storage/PhabricatorRepositoryGitLFSRef.php', @@ -8910,6 +8911,7 @@ phutil_register_library_map(array( 'PhabricatorProjectInterface', 'PhabricatorSpacesInterface', 'PhabricatorConduitResultInterface', + 'PhabricatorFulltextInterface', ), 'PhabricatorRepositoryAuditRequest' => array( 'PhabricatorRepositoryDAO', @@ -8951,6 +8953,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryDiscoveryEngine' => 'PhabricatorRepositoryEngine', 'PhabricatorRepositoryEditor' => 'PhabricatorApplicationTransactionEditor', 'PhabricatorRepositoryEngine' => 'Phobject', + 'PhabricatorRepositoryFulltextEngine' => 'PhabricatorFulltextEngine', 'PhabricatorRepositoryGitCommitChangeParserWorker' => 'PhabricatorRepositoryCommitChangeParserWorker', 'PhabricatorRepositoryGitCommitMessageParserWorker' => 'PhabricatorRepositoryCommitMessageParserWorker', 'PhabricatorRepositoryGitLFSRef' => array( diff --git a/src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php b/src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php new file mode 100644 index 0000000000..f666af552f --- /dev/null +++ b/src/applications/repository/search/PhabricatorRepositoryFulltextEngine.php @@ -0,0 +1,27 @@ +setDocumentTitle($repo->getName()); + $document->addField( + PhabricatorSearchDocumentFieldType::FIELD_BODY, + $repo->getRepositorySlug()."\n".$repo->getDetail('description')); + + $document->setDocumentCreated($repo->getDateCreated()); + $document->setDocumentModified($repo->getDateModified()); + + $document->addRelationship( + $repo->isTracked() + ? PhabricatorSearchRelationship::RELATIONSHIP_OPEN + : PhabricatorSearchRelationship::RELATIONSHIP_CLOSED, + $repo->getPHID(), + PhabricatorRepositoryRepositoryPHIDType::TYPECONST, + PhabricatorTime::getNow()); + } + +} diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 1a42eb8bd2..fd7413c392 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -14,7 +14,8 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO PhabricatorDestructibleInterface, PhabricatorProjectInterface, PhabricatorSpacesInterface, - PhabricatorConduitResultInterface { + PhabricatorConduitResultInterface, + PhabricatorFulltextInterface { /** * Shortest hash we'll recognize in raw "a829f32" form. @@ -2572,4 +2573,11 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO ); } +/* -( PhabricatorFulltextInterface )--------------------------------------- */ + + + public function newFulltextEngine() { + return new PhabricatorRepositoryFulltextEngine(); + } + } diff --git a/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php index 36996861a9..7838808f48 100644 --- a/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php @@ -158,9 +158,15 @@ final class PhabricatorSearchManagementIndexWorkflow $object_class = get_class($object); $normalized_class = phutil_utf8_strtolower($object_class); + if ($normalized_class === $normalized_type) { + $matches = array($object_class => $object); + break; + } + if (!strlen($type) || strpos($normalized_class, $normalized_type) !== false) { $matches[$object_class] = $object; + } } From 7d3956bec17f5cbd274b7c497cc596be78f0f6ed Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 09:58:47 -0700 Subject: [PATCH 07/39] Correct spelling of "Dasbhoard" Summary: Before the speling pollice lock us in prisun. Test Plan: Used a dicationairey. Reviewers: chad, jmeador Reviewed By: jmeador Differential Revision: https://secure.phabricator.com/D17570 --- .../controller/PhabricatorApplicationSearchController.php | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/applications/search/controller/PhabricatorApplicationSearchController.php b/src/applications/search/controller/PhabricatorApplicationSearchController.php index dd3508e6bb..b0cab13e07 100644 --- a/src/applications/search/controller/PhabricatorApplicationSearchController.php +++ b/src/applications/search/controller/PhabricatorApplicationSearchController.php @@ -622,7 +622,7 @@ final class PhabricatorApplicationSearchController $dashboard_uri = '/dashboard/install/'; $actions[] = id(new PhabricatorActionView()) ->setIcon('fa-dashboard') - ->setName(pht('Add to Dasbhoard')) + ->setName(pht('Add to Dashboard')) ->setWorkflow(true) ->setHref("/dashboard/panel/install/{$engine_class}/{$query_key}/"); } From aea46e55dac035e33080016c190c6c0765a84fa1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 05:23:42 -0700 Subject: [PATCH 08/39] Fix an issue where "Request Review" of a fully-accepted revision would transition to "Accepted" Summary: Ref T10967. This is explained in more detail in T10967#217125 When an author does "Request Review" on an accepted revision, void (in the sense of "cancel out", like a bank check) any "accepted" reviewers on the current diff. Test Plan: - Create a revision with author A and reviewer B. - Accept as B. - "Request Review" as A. - (With sticky accepts enabled.) - Before patch: revision swithced back to "accepted". - After patch: the earlier review is "voided" by te "Request Review", and the revision switches to "Review Requested". Reviewers: chad Reviewed By: chad Maniphest Tasks: T10967 Differential Revision: https://secure.phabricator.com/D17566 --- .../20170328.reviewers.01.void.sql | 2 + src/__phutil_library_map__.php | 2 + .../editor/DifferentialTransactionEditor.php | 13 ++-- .../storage/DifferentialReviewer.php | 9 +++ .../DifferentialRevisionReviewTransaction.php | 41 +++++++----- .../DifferentialRevisionVoidTransaction.php | 63 +++++++++++++++++++ 6 files changed, 109 insertions(+), 21 deletions(-) create mode 100644 resources/sql/autopatches/20170328.reviewers.01.void.sql create mode 100644 src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php diff --git a/resources/sql/autopatches/20170328.reviewers.01.void.sql b/resources/sql/autopatches/20170328.reviewers.01.void.sql new file mode 100644 index 0000000000..b46cb9351d --- /dev/null +++ b/resources/sql/autopatches/20170328.reviewers.01.void.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_differential.differential_reviewer + ADD voidedPHID VARBINARY(64); diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 5d9e3de268..0921fa21e9 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -571,6 +571,7 @@ phutil_register_library_map(array( 'DifferentialRevisionTransactionType' => 'applications/differential/xaction/DifferentialRevisionTransactionType.php', 'DifferentialRevisionUpdateHistoryView' => 'applications/differential/view/DifferentialRevisionUpdateHistoryView.php', 'DifferentialRevisionViewController' => 'applications/differential/controller/DifferentialRevisionViewController.php', + 'DifferentialRevisionVoidTransaction' => 'applications/differential/xaction/DifferentialRevisionVoidTransaction.php', 'DifferentialSchemaSpec' => 'applications/differential/storage/DifferentialSchemaSpec.php', 'DifferentialSetDiffPropertyConduitAPIMethod' => 'applications/differential/conduit/DifferentialSetDiffPropertyConduitAPIMethod.php', 'DifferentialStoredCustomField' => 'applications/differential/customfield/DifferentialStoredCustomField.php', @@ -5357,6 +5358,7 @@ phutil_register_library_map(array( 'DifferentialRevisionTransactionType' => 'PhabricatorModularTransactionType', 'DifferentialRevisionUpdateHistoryView' => 'AphrontView', 'DifferentialRevisionViewController' => 'DifferentialController', + 'DifferentialRevisionVoidTransaction' => 'DifferentialRevisionTransactionType', 'DifferentialSchemaSpec' => 'PhabricatorConfigSchemaSpec', 'DifferentialSetDiffPropertyConduitAPIMethod' => 'DifferentialConduitAPIMethod', 'DifferentialStoredCustomField' => 'DifferentialCustomField', diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 3f00f4b837..9b837876fb 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -357,6 +357,14 @@ final class DifferentialTransactionEditor } } + if ($downgrade_accepts) { + $void_type = DifferentialRevisionVoidTransaction::TRANSACTIONTYPE; + $results[] = id(new DifferentialTransaction()) + ->setTransactionType($void_type) + ->setIgnoreOnNoEffect(true) + ->setNewValue(true); + } + $is_commandeer = false; switch ($xaction->getTransactionType()) { case DifferentialTransaction::TYPE_UPDATE: @@ -685,11 +693,8 @@ final class DifferentialTransactionEditor break; case DifferentialReviewerStatus::STATUS_ACCEPTED: if ($reviewer->isUser()) { - $action_phid = $reviewer->getLastActionDiffPHID(); $active_phid = $active_diff->getPHID(); - $is_current = ($action_phid == $active_phid); - - if ($is_sticky_accept || $is_current) { + if ($reviewer->isAccepted($active_phid)) { $has_accepting_user = true; } } diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index d43f533b5c..6912e2af1e 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -9,6 +9,7 @@ final class DifferentialReviewer protected $lastActionDiffPHID; protected $lastCommentDiffPHID; protected $lastActorPHID; + protected $voidedPHID; private $authority = array(); @@ -19,6 +20,7 @@ final class DifferentialReviewer 'lastActionDiffPHID' => 'phid?', 'lastCommentDiffPHID' => 'phid?', 'lastActorPHID' => 'phid?', + 'voidedPHID' => 'phid?', ), self::CONFIG_KEY_SCHEMA => array( 'key_revision' => array( @@ -59,6 +61,13 @@ final class DifferentialReviewer return false; } + // If this accept has been voided (for example, but a reviewer using + // "Request Review"), don't count it as a real "Accept" even if it is + // against the current diff PHID. + if ($this->getVoidedPHID()) { + return false; + } + if (!$diff_phid) { return true; } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php index 3db289470a..c0971a5a2b 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php @@ -50,25 +50,19 @@ abstract class DifferentialRevisionReviewTransaction protected function isViewerFullyAccepted( DifferentialRevision $revision, PhabricatorUser $viewer) { - return $this->isViewerReviewerStatusFullyAmong( + return $this->isViewerReviewerStatusFully( $revision, $viewer, - array( - DifferentialReviewerStatus::STATUS_ACCEPTED, - ), - true); + DifferentialReviewerStatus::STATUS_ACCEPTED); } protected function isViewerFullyRejected( DifferentialRevision $revision, PhabricatorUser $viewer) { - return $this->isViewerReviewerStatusFullyAmong( + return $this->isViewerReviewerStatusFully( $revision, $viewer, - array( - DifferentialReviewerStatus::STATUS_REJECTED, - ), - true); + DifferentialReviewerStatus::STATUS_REJECTED); } protected function getViewerReviewerStatus( @@ -90,11 +84,10 @@ abstract class DifferentialRevisionReviewTransaction return null; } - protected function isViewerReviewerStatusFullyAmong( + private function isViewerReviewerStatusFully( DifferentialRevision $revision, PhabricatorUser $viewer, - array $status_list, - $require_current) { + $require_status) { // If the user themselves is not a reviewer, the reviews they have // authority over can not all be in any set of states since their own @@ -106,24 +99,33 @@ abstract class DifferentialRevisionReviewTransaction $active_phid = $this->getActiveDiffPHID($revision); + $status_accepted = DifferentialReviewerStatus::STATUS_ACCEPTED; + $is_accepted = ($require_status == $status_accepted); + // Otherwise, check that all reviews they have authority over are in // the desired set of states. - $status_map = array_fuse($status_list); foreach ($revision->getReviewers() as $reviewer) { if (!$reviewer->hasAuthority($viewer)) { continue; } $status = $reviewer->getReviewerStatus(); - if (!isset($status_map[$status])) { + if ($status != $require_status) { return false; } - if ($require_current) { - if ($reviewer->getLastActionDiffPHID() != $active_phid) { + // Here, we're primarily testing if we can remove a void on the review. + if ($is_accepted) { + if (!$reviewer->isAccepted($active_phid)) { return false; } } + + // This is a broader check to see if we can update the diff where the + // last action occurred. + if ($reviewer->getLastActionDiffPHID() != $active_phid) { + return false; + } } return true; @@ -223,6 +225,11 @@ abstract class DifferentialRevisionReviewTransaction $reviewer->setLastActorPHID($this->getActingAsPHID()); } + // Clear any outstanding void on this reviewer. A void may be placed + // by the author using "Request Review" when a reviewer has already + // accepted. + $reviewer->setVoidedPHID(null); + try { $reviewer->save(); } catch (AphrontDuplicateKeyQueryException $ex) { diff --git a/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php new file mode 100644 index 0000000000..e40cb8af62 --- /dev/null +++ b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php @@ -0,0 +1,63 @@ +getTableName(); + $conn = $table->establishConnection('w'); + + $rows = queryfx_all( + $conn, + 'SELECT reviewerPHID FROM %T + WHERE revisionPHID = %s + AND voidedPHID IS NULL + AND reviewerStatus = %s', + $table_name, + $object->getPHID(), + DifferentialReviewerStatus::STATUS_ACCEPTED); + + return ipull($rows, 'reviewerPHID'); + } + + public function getTransactionHasEffect($object, $old, $new) { + return (bool)$new; + } + + public function applyExternalEffects($object, $value) { + $table = new DifferentialReviewer(); + $table_name = $table->getTableName(); + $conn = $table->establishConnection('w'); + + queryfx( + $conn, + 'UPDATE %T SET voidedPHID = %s + WHERE revisionPHID = %s + AND voidedPHID IS NULL + AND reviewerStatus = %s', + $table_name, + $this->getActingAsPHID(), + $object->getPHID(), + DifferentialReviewerStatus::STATUS_ACCEPTED); + } + + public function shouldHide() { + // This is an internal transaction, so don't show it in feeds or + // transaction logs. + return true; + } + +} From 415ad784844e5fd05239dcd34b186c7561fc8860 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 06:07:08 -0700 Subject: [PATCH 09/39] Remove old code for "Request Review" action from Differential Summary: Ref T10967. This moves all remaining "request review" pathways (just `differential.createcomment`) to the new code, and removes the old action. Test Plan: Requested review on a revision, `grep`'d for the action constant. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10967 Differential Revision: https://secure.phabricator.com/D17567 --- ...ferentialCreateCommentConduitAPIMethod.php | 2 + .../editor/DifferentialTransactionEditor.php | 53 ------------------- .../storage/DifferentialTransaction.php | 2 - 3 files changed, 2 insertions(+), 55 deletions(-) diff --git a/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php index ee70644537..52736d6f3b 100644 --- a/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialCreateCommentConduitAPIMethod.php @@ -60,6 +60,8 @@ final class DifferentialCreateCommentConduitAPIMethod 'accept' => DifferentialRevisionAcceptTransaction::TRANSACTIONTYPE, 'reject' => DifferentialRevisionRejectTransaction::TRANSACTIONTYPE, 'resign' => DifferentialRevisionResignTransaction::TRANSACTIONTYPE, + 'request_review' => + DifferentialRevisionRequestReviewTransaction::TRANSACTIONTYPE, ); $action = $request->getValue('action'); diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 9b837876fb..e35257ff9b 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -140,8 +140,6 @@ final class DifferentialTransactionEditor return ($object->getStatus() == $status_closed); case DifferentialAction::ACTION_RETHINK: return ($object->getStatus() != $status_plan); - case DifferentialAction::ACTION_REQUEST: - return ($object->getStatus() != $status_review); case DifferentialAction::ACTION_CLAIM: return ($actor_phid != $object->getAuthorPHID()); } @@ -200,9 +198,6 @@ final class DifferentialTransactionEditor case DifferentialAction::ACTION_REOPEN: $object->setStatus($status_review); return; - case DifferentialAction::ACTION_REQUEST: - $object->setStatus($status_review); - return; case DifferentialAction::ACTION_CLOSE: $old_status = $object->getStatus(); $object->setStatus(ArcanistDifferentialRevisionStatus::CLOSED); @@ -294,19 +289,6 @@ final class DifferentialTransactionEditor $downgrade_accepts = true; } break; - - // TODO: Remove this, obsoleted by ModularTransactions above. - case DifferentialTransaction::TYPE_ACTION: - switch ($xaction->getNewValue()) { - case DifferentialAction::ACTION_REQUEST: - $downgrade_rejects = true; - if ((!$is_sticky_accept) || - ($object->getStatus() != $status_plan)) { - $downgrade_accepts = true; - } - break; - } - break; } } @@ -952,41 +934,6 @@ final class DifferentialTransactionEditor } break; - case DifferentialAction::ACTION_REQUEST: - if (!$actor_is_author) { - return pht( - 'You can not request review of this revision because you do '. - 'not own it. To request review of a revision, you must be its '. - 'owner.'); - } - - switch ($revision_status) { - case ArcanistDifferentialRevisionStatus::ACCEPTED: - case ArcanistDifferentialRevisionStatus::NEEDS_REVISION: - case ArcanistDifferentialRevisionStatus::CHANGES_PLANNED: - // These are OK. - break; - case ArcanistDifferentialRevisionStatus::NEEDS_REVIEW: - // This will be caught as "no effect" later on. - break; - case ArcanistDifferentialRevisionStatus::ABANDONED: - return pht( - 'You can not request review of this revision because it has '. - 'been abandoned. Instead, reclaim it.'); - case ArcanistDifferentialRevisionStatus::CLOSED: - return pht( - 'You can not request review of this revision because it has '. - 'already been closed.'); - default: - throw new Exception( - pht( - 'Encountered unexpected revision status ("%s") when '. - 'validating "%s" action.', - $revision_status, - $action)); - } - break; - case DifferentialAction::ACTION_CLOSE: // We force revisions closed when we discover a corresponding commit. // In this case, revisions are allowed to transition to closed from diff --git a/src/applications/differential/storage/DifferentialTransaction.php b/src/applications/differential/storage/DifferentialTransaction.php index 868a24ebb0..c6c55e0cdd 100644 --- a/src/applications/differential/storage/DifferentialTransaction.php +++ b/src/applications/differential/storage/DifferentialTransaction.php @@ -607,8 +607,6 @@ final class DifferentialTransaction 'not closed.'); case DifferentialAction::ACTION_RETHINK: return pht('This revision already requires changes.'); - case DifferentialAction::ACTION_REQUEST: - return pht('Review is already requested for this revision.'); case DifferentialAction::ACTION_CLAIM: return pht( 'You can not commandeer this revision because you already own '. From ddc02ce420a56d964eb3f38770d03a1bf4d35dac Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 06:22:19 -0700 Subject: [PATCH 10/39] When voiding "Accept" reviews, also void "Reject" reviews Summary: Ref T10967. This change is similar to D17566, but for rejects. Test Plan: - Create a revision as A, with reviewer B. - Reject as B. - Request review as A. - Before patch: stuck in "rejected". - After patch: transitions back to "needs review". Reviewers: chad Reviewed By: chad Maniphest Tasks: T10967 Differential Revision: https://secure.phabricator.com/D17568 --- .../editor/DifferentialTransactionEditor.php | 7 +--- .../storage/DifferentialReviewer.php | 41 +++++++++++++++---- .../DifferentialRevisionReviewTransaction.php | 9 ++++ .../DifferentialRevisionVoidTransaction.php | 15 +++++-- 4 files changed, 56 insertions(+), 16 deletions(-) diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index e35257ff9b..97ad60fc12 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -339,7 +339,7 @@ final class DifferentialTransactionEditor } } - if ($downgrade_accepts) { + if ($downgrade_accepts || $downgrade_rejects) { $void_type = DifferentialRevisionVoidTransaction::TRANSACTIONTYPE; $results[] = id(new DifferentialTransaction()) ->setTransactionType($void_type) @@ -659,11 +659,8 @@ final class DifferentialTransactionEditor $reviewer_status = $reviewer->getReviewerStatus(); switch ($reviewer_status) { case DifferentialReviewerStatus::STATUS_REJECTED: - $action_phid = $reviewer->getLastActionDiffPHID(); $active_phid = $active_diff->getPHID(); - $is_current = ($action_phid == $active_phid); - - if ($is_current) { + if ($reviewer->isRejected($active_phid)) { $has_rejecting_reviewer = true; } break; diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index 6912e2af1e..b202baddf2 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -54,6 +54,25 @@ final class DifferentialReviewer return ($this->getReviewerStatus() == $status_resigned); } + public function isRejected($diff_phid) { + $status_rejected = DifferentialReviewerStatus::STATUS_REJECTED; + + if ($this->getReviewerStatus() != $status_rejected) { + return false; + } + + if ($this->getVoidedPHID()) { + return false; + } + + if ($this->isCurrentAction($diff_phid)) { + return true; + } + + return false; + } + + public function isAccepted($diff_phid) { $status_accepted = DifferentialReviewerStatus::STATUS_ACCEPTED; @@ -68,6 +87,21 @@ final class DifferentialReviewer return false; } + if ($this->isCurrentAction($diff_phid)) { + return true; + } + + $sticky_key = 'differential.sticky-accept'; + $is_sticky = PhabricatorEnv::getEnvConfig($sticky_key); + + if ($is_sticky) { + return true; + } + + return false; + } + + private function isCurrentAction($diff_phid) { if (!$diff_phid) { return true; } @@ -82,13 +116,6 @@ final class DifferentialReviewer return true; } - $sticky_key = 'differential.sticky-accept'; - $is_sticky = PhabricatorEnv::getEnvConfig($sticky_key); - - if ($is_sticky) { - return true; - } - return false; } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php index c0971a5a2b..4e3e7b29e6 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php @@ -100,7 +100,10 @@ abstract class DifferentialRevisionReviewTransaction $active_phid = $this->getActiveDiffPHID($revision); $status_accepted = DifferentialReviewerStatus::STATUS_ACCEPTED; + $status_rejected = DifferentialReviewerStatus::STATUS_REJECTED; + $is_accepted = ($require_status == $status_accepted); + $is_rejected = ($require_status == $status_rejected); // Otherwise, check that all reviews they have authority over are in // the desired set of states. @@ -121,6 +124,12 @@ abstract class DifferentialRevisionReviewTransaction } } + if ($is_rejected) { + if (!$reviewer->isRejected($active_phid)) { + return false; + } + } + // This is a broader check to see if we can update the diff where the // last action occurred. if ($reviewer->getLastActionDiffPHID() != $active_phid) { diff --git a/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php index e40cb8af62..ae684d94fe 100644 --- a/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionVoidTransaction.php @@ -25,10 +25,10 @@ final class DifferentialRevisionVoidTransaction 'SELECT reviewerPHID FROM %T WHERE revisionPHID = %s AND voidedPHID IS NULL - AND reviewerStatus = %s', + AND reviewerStatus IN (%Ls)', $table_name, $object->getPHID(), - DifferentialReviewerStatus::STATUS_ACCEPTED); + $this->getVoidableStatuses()); return ipull($rows, 'reviewerPHID'); } @@ -47,11 +47,11 @@ final class DifferentialRevisionVoidTransaction 'UPDATE %T SET voidedPHID = %s WHERE revisionPHID = %s AND voidedPHID IS NULL - AND reviewerStatus = %s', + AND reviewerStatus IN (%Ls)', $table_name, $this->getActingAsPHID(), $object->getPHID(), - DifferentialReviewerStatus::STATUS_ACCEPTED); + $this->getVoidableStatuses()); } public function shouldHide() { @@ -60,4 +60,11 @@ final class DifferentialRevisionVoidTransaction return true; } + private function getVoidableStatuses() { + return array( + DifferentialReviewerStatus::STATUS_ACCEPTED, + DifferentialReviewerStatus::STATUS_REJECTED, + ); + } + } From 2fbc9a52da9bf2132df6b22bd6df370c67802ec9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 09:30:58 -0700 Subject: [PATCH 11/39] Allow users to "Force accept" package reviews if they own a more general package Summary: Ref T12272. If you own a package which owns "/", this allows you to force-accept package reviews for packages which own sub-paths, like "/src/adventure/". The default UI looks something like this: ``` [X] Accept as epriestley [X] Accept as Root Package [ ] Force accept as Adventure Package ``` By default, force-accepts are not selected. (I may do some UI cleanup and/or annotate "because you own X" in the future and/or mark these accepts specially in some way, particularly if this proves confusing along whatever dimension.) Test Plan: {F4314747} Reviewers: chad Reviewed By: chad Maniphest Tasks: T12272 Differential Revision: https://secure.phabricator.com/D17569 --- .../controller/DifferentialController.php | 6 +- .../editor/DifferentialTransactionEditor.php | 2 - .../storage/DifferentialChangeset.php | 17 ++ .../storage/DifferentialReviewer.php | 5 + .../storage/DifferentialRevision.php | 238 ++++++++++++++++++ .../DifferentialRevisionAcceptTransaction.php | 38 ++- .../DifferentialRevisionActionTransaction.php | 7 +- .../DifferentialRevisionReviewTransaction.php | 26 +- .../query/PhabricatorOwnersPackageQuery.php | 12 +- 9 files changed, 332 insertions(+), 19 deletions(-) diff --git a/src/applications/differential/controller/DifferentialController.php b/src/applications/differential/controller/DifferentialController.php index 1991580116..2258c4fdcb 100644 --- a/src/applications/differential/controller/DifferentialController.php +++ b/src/applications/differential/controller/DifferentialController.php @@ -54,9 +54,7 @@ abstract class DifferentialController extends PhabricatorController { $toc_view->setAuthorityPackages($packages); } - // TODO: For Subversion, we should adjust these paths to be relative to - // the repository root where possible. - $paths = mpull($changesets, 'getFilename'); + $paths = mpull($changesets, 'getOwnersFilename'); $control_query = id(new PhabricatorOwnersPackageQuery()) ->setViewer($viewer) @@ -83,7 +81,7 @@ abstract class DifferentialController extends PhabricatorController { if ($have_owners) { $packages = $control_query->getControllingPackagesForPath( $repository_phid, - $changeset->getFilename()); + $changeset->getOwnersFilename()); $item->setPackages($packages); } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index 97ad60fc12..bf7faa2700 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1857,6 +1857,4 @@ final class DifferentialTransactionEditor $acting_phid); } - - } diff --git a/src/applications/differential/storage/DifferentialChangeset.php b/src/applications/differential/storage/DifferentialChangeset.php index 448cdb6343..be83c5e73b 100644 --- a/src/applications/differential/storage/DifferentialChangeset.php +++ b/src/applications/differential/storage/DifferentialChangeset.php @@ -75,6 +75,23 @@ final class DifferentialChangeset extends DifferentialDAO return $name; } + public function getOwnersFilename() { + // TODO: For Subversion, we should adjust these paths to be relative to + // the repository root where possible. + + $path = $this->getFilename(); + + if (!isset($path[0])) { + return '/'; + } + + if ($path[0] != '/') { + $path = '/'.$path; + } + + return $path; + } + public function addUnsavedHunk(DifferentialHunk $hunk) { if ($this->hunks === self::ATTACHABLE) { $this->hunks = array(); diff --git a/src/applications/differential/storage/DifferentialReviewer.php b/src/applications/differential/storage/DifferentialReviewer.php index b202baddf2..3aa9bf6362 100644 --- a/src/applications/differential/storage/DifferentialReviewer.php +++ b/src/applications/differential/storage/DifferentialReviewer.php @@ -39,6 +39,11 @@ final class DifferentialReviewer return (phid_get_type($this->getReviewerPHID()) == $user_type); } + public function isPackage() { + $package_type = PhabricatorOwnersPackagePHIDType::TYPECONST; + return (phid_get_type($this->getReviewerPHID()) == $package_type); + } + public function attachAuthority(PhabricatorUser $user, $has_authority) { $this->authority[$user->getCacheFragment()] = $has_authority; return $this; diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php index 7189c5f5b4..72204256e5 100644 --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -48,6 +48,7 @@ final class DifferentialRevision extends DifferentialDAO private $customFields = self::ATTACHABLE; private $drafts = array(); private $flags = array(); + private $forceMap = array(); const TABLE_COMMIT = 'differential_commit'; @@ -245,6 +246,243 @@ final class DifferentialRevision extends DifferentialDAO return $this; } + public function canReviewerForceAccept( + PhabricatorUser $viewer, + DifferentialReviewer $reviewer) { + + if (!$reviewer->isPackage()) { + return false; + } + + $map = $this->getReviewerForceAcceptMap($viewer); + if (!$map) { + return false; + } + + if (isset($map[$reviewer->getReviewerPHID()])) { + return true; + } + + return false; + } + + private function getReviewerForceAcceptMap(PhabricatorUser $viewer) { + $fragment = $viewer->getCacheFragment(); + + if (!array_key_exists($fragment, $this->forceMap)) { + $map = $this->newReviewerForceAcceptMap($viewer); + $this->forceMap[$fragment] = $map; + } + + return $this->forceMap[$fragment]; + } + + private function newReviewerForceAcceptMap(PhabricatorUser $viewer) { + $diff = $this->getActiveDiff(); + if (!$diff) { + return null; + } + + $repository_phid = $diff->getRepositoryPHID(); + if (!$repository_phid) { + return null; + } + + $paths = array(); + + try { + $changesets = $diff->getChangesets(); + } catch (Exception $ex) { + $changesets = id(new DifferentialChangesetQuery()) + ->setViewer($viewer) + ->withDiffs(array($diff)) + ->execute(); + } + + foreach ($changesets as $changeset) { + $paths[] = $changeset->getOwnersFilename(); + } + + if (!$paths) { + return null; + } + + $reviewer_phids = array(); + foreach ($this->getReviewers() as $reviewer) { + if (!$reviewer->isPackage()) { + continue; + } + + $reviewer_phids[] = $reviewer->getReviewerPHID(); + } + + if (!$reviewer_phids) { + return null; + } + + // Load all the reviewing packages which have control over some of the + // paths in the change. These are packages which the actor may be able + // to force-accept on behalf of. + $control_query = id(new PhabricatorOwnersPackageQuery()) + ->setViewer($viewer) + ->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE)) + ->withPHIDs($reviewer_phids) + ->withControl($repository_phid, $paths); + $control_packages = $control_query->execute(); + if (!$control_packages) { + return null; + } + + // Load all the packages which have potential control over some of the + // paths in the change and are owned by the actor. These are packages + // which the actor may be able to use their authority over to gain the + // ability to force-accept for other packages. This query doesn't apply + // dominion rules yet, and we'll bypass those rules later on. + $authority_query = id(new PhabricatorOwnersPackageQuery()) + ->setViewer($viewer) + ->withStatuses(array(PhabricatorOwnersPackage::STATUS_ACTIVE)) + ->withAuthorityPHIDs(array($viewer->getPHID())) + ->withControl($repository_phid, $paths); + $authority_packages = $authority_query->execute(); + if (!$authority_packages) { + return null; + } + $authority_packages = mpull($authority_packages, null, 'getPHID'); + + // Build a map from each path in the revision to the reviewer packages + // which control it. + $control_map = array(); + foreach ($paths as $path) { + $control_packages = $control_query->getControllingPackagesForPath( + $repository_phid, + $path); + + // Remove packages which the viewer has authority over. We don't need + // to check these for force-accept because they can just accept them + // normally. + $control_packages = mpull($control_packages, null, 'getPHID'); + foreach ($control_packages as $phid => $control_package) { + if (isset($authority_packages[$phid])) { + unset($control_packages[$phid]); + } + } + + if (!$control_packages) { + continue; + } + + $control_map[$path] = $control_packages; + } + + if (!$control_map) { + return null; + } + + // From here on out, we only care about paths which we have at least one + // controlling package for. + $paths = array_keys($control_map); + + // Now, build a map from each path to the packages which would control it + // if there were no dominion rules. + $authority_map = array(); + foreach ($paths as $path) { + $authority_packages = $authority_query->getControllingPackagesForPath( + $repository_phid, + $path, + $ignore_dominion = true); + + $authority_map[$path] = mpull($authority_packages, null, 'getPHID'); + } + + // For each path, find the most general package that the viewer has + // authority over. For example, we'll prefer a package that owns "/" to a + // package that owns "/src/". + $force_map = array(); + foreach ($authority_map as $path => $package_map) { + $path_fragments = PhabricatorOwnersPackage::splitPath($path); + $fragment_count = count($path_fragments); + + // Find the package that we have authority over which has the most + // general match for this path. + $best_match = null; + $best_package = null; + foreach ($package_map as $package_phid => $package) { + $package_paths = $package->getPathsForRepository($repository_phid); + foreach ($package_paths as $package_path) { + + // NOTE: A strength of 0 means "no match". A strength of 1 means + // that we matched "/", so we can not possibly find another stronger + // match. + + $strength = $package_path->getPathMatchStrength( + $path_fragments, + $fragment_count); + if (!$strength) { + continue; + } + + if ($strength < $best_match || !$best_package) { + $best_match = $strength; + $best_package = $package; + if ($strength == 1) { + break 2; + } + } + } + } + + if ($best_package) { + $force_map[$path] = array( + 'strength' => $best_match, + 'package' => $best_package, + ); + } + } + + // For each path which the viewer owns a package for, find other packages + // which that authority can be used to force-accept. Once we find a way to + // force-accept a package, we don't need to keep loooking. + $has_control = array(); + foreach ($force_map as $path => $spec) { + $path_fragments = PhabricatorOwnersPackage::splitPath($path); + $fragment_count = count($path_fragments); + + $authority_strength = $spec['strength']; + + $control_packages = $control_map[$path]; + foreach ($control_packages as $control_phid => $control_package) { + if (isset($has_control[$control_phid])) { + continue; + } + + $control_paths = $control_package->getPathsForRepository( + $repository_phid); + foreach ($control_paths as $control_path) { + $strength = $control_path->getPathMatchStrength( + $path_fragments, + $fragment_count); + + if (!$strength) { + continue; + } + + if ($strength > $authority_strength) { + $authority = $spec['package']; + $has_control[$control_phid] = array( + 'authority' => $authority, + 'phid' => $authority->getPHID(), + ); + break; + } + } + } + } + + // Return a map from packages which may be force accepted to the packages + // which permit that forced acceptance. + return ipull($has_control, 'phid'); + } + /* -( PhabricatorPolicyInterface )----------------------------------------- */ diff --git a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php index 4f6bedd698..7d1f1a92ef 100644 --- a/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionAcceptTransaction.php @@ -84,11 +84,18 @@ final class DifferentialRevisionAcceptTransaction } } + $default_unchecked = array(); foreach ($reviewers as $reviewer) { + $reviewer_phid = $reviewer->getReviewerPHID(); + if (!$reviewer->hasAuthority($viewer)) { // If the viewer doesn't have authority to act on behalf of a reviewer, - // don't include that reviewer as an option. - continue; + // we check if they can accept by force. + if ($revision->canReviewerForceAccept($viewer, $reviewer)) { + $default_unchecked[$reviewer_phid] = true; + } else { + continue; + } } if ($reviewer->isAccepted($diff_phid)) { @@ -97,20 +104,37 @@ final class DifferentialRevisionAcceptTransaction continue; } - $reviewer_phid = $reviewer->getReviewerPHID(); $reviewer_phids[$reviewer_phid] = $reviewer_phid; } $handles = $viewer->loadHandles($reviewer_phids); + $head = array(); + $tail = array(); foreach ($reviewer_phids as $reviewer_phid) { - $options[$reviewer_phid] = pht( - 'Accept as %s', - $viewer->renderHandle($reviewer_phid)); + $is_force = isset($default_unchecked[$reviewer_phid]); - $value[] = $reviewer_phid; + if ($is_force) { + $tail[] = $reviewer_phid; + + $options[$reviewer_phid] = pht( + 'Force accept as %s', + $viewer->renderHandle($reviewer_phid)); + } else { + $head[] = $reviewer_phid; + $value[] = $reviewer_phid; + + $options[$reviewer_phid] = pht( + 'Accept as %s', + $viewer->renderHandle($reviewer_phid)); + } } + // Reorder reviewers so "force accept" reviewers come at the end. + $options = + array_select_keys($options, $head) + + array_select_keys($options, $tail); + return array($options, $value); } diff --git a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php index f232398533..8e1c437c53 100644 --- a/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionActionTransaction.php @@ -122,7 +122,12 @@ abstract class DifferentialRevisionActionTransaction $field->setActionConflictKey('revision.action'); list($options, $value) = $this->getActionOptions($viewer, $revision); - if (count($options) > 1) { + + // Show the options if the user can select on behalf of two or more + // reviewers, or can force-accept on behalf of one or more reviewers. + $can_multi = (count($options) > 1); + $can_force = (count($value) < count($options)); + if ($can_multi || $can_force) { $field->setOptions($options); $field->setValue($value); } diff --git a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php index 4e3e7b29e6..019c4c036f 100644 --- a/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionReviewTransaction.php @@ -109,7 +109,17 @@ abstract class DifferentialRevisionReviewTransaction // the desired set of states. foreach ($revision->getReviewers() as $reviewer) { if (!$reviewer->hasAuthority($viewer)) { - continue; + $can_force = false; + + if ($is_accepted) { + if ($revision->canReviewerForceAccept($viewer, $reviewer)) { + $can_force = true; + } + } + + if (!$can_force) { + continue; + } } $status = $reviewer->getReviewerStatus(); @@ -152,11 +162,21 @@ abstract class DifferentialRevisionReviewTransaction // reviewers you have authority for. When you resign, you only affect // yourself. $with_authority = ($status != DifferentialReviewerStatus::STATUS_RESIGNED); + $with_force = ($status == DifferentialReviewerStatus::STATUS_ACCEPTED); + if ($with_authority) { foreach ($revision->getReviewers() as $reviewer) { - if ($reviewer->hasAuthority($viewer)) { - $map[$reviewer->getReviewerPHID()] = $status; + if (!$reviewer->hasAuthority($viewer)) { + if (!$with_force) { + continue; + } + + if (!$revision->canReviewerForceAccept($viewer, $reviewer)) { + continue; + } } + + $map[$reviewer->getReviewerPHID()] = $status; } } diff --git a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php index 8194dd7a6a..a1c10cd5e9 100644 --- a/src/applications/owners/query/PhabricatorOwnersPackageQuery.php +++ b/src/applications/owners/query/PhabricatorOwnersPackageQuery.php @@ -348,7 +348,10 @@ final class PhabricatorOwnersPackageQuery * * @return list List of controlling packages. */ - public function getControllingPackagesForPath($repository_phid, $path) { + public function getControllingPackagesForPath( + $repository_phid, + $path, + $ignore_dominion = false) { $path = (string)$path; if (!isset($this->controlMap[$repository_phid][$path])) { @@ -382,9 +385,14 @@ final class PhabricatorOwnersPackageQuery } if ($best_match && $include) { + if ($ignore_dominion) { + $is_weak = false; + } else { + $is_weak = ($package->getDominion() == $weak_dominion); + } $matches[$package_id] = array( 'strength' => $best_match, - 'weak' => ($package->getDominion() == $weak_dominion), + 'weak' => $is_weak, 'package' => $package, ); } From 699228c73b74e2a3ea2e8355ed822c9314fb9f88 Mon Sep 17 00:00:00 2001 From: Mukunda Modell Date: Tue, 28 Mar 2017 20:19:38 +0000 Subject: [PATCH 12/39] Address some New Search Configuration Errata Summary: [ ] Write an "Upgrading: ..." guidance task with narrow instructions for installs that are upgrading. [ ] Do we need to add an indexing activity (T11932) for installs with ElasticSearch? [ ] We should more clearly detail exactly which versions of ElasticSearch are supported (for example, is ElasticSearch <2 no longer supported)? From T9893 it seems like we may //only// have supported ElasticSearch <2 before, so are the two regions of support totally nonoverlapping and all ElasticSearch users will need to upgrade? [ ] Documentation should provide stronger guidance toward MySQL and away from Elastic for the vast majority of installs, because we've historically seen users choosing Elastic when they aren't actually trying to solve any specific problem. [ ] When users search for fulltext results in Maniphest and hit too many documents, the current behavior is approximately silent failure (see T12443). D17384 has also lowered the ceiling for ElasticSearch, although previous changes lowered it for MySQL search. We should not fail silently, and ideally should build toward T12003. [ ] D17384 added a new "keywords" field, but MySQL does not search it (I think?). The behavior should be as consistent across MySQL and Elastic as we can make it. Likely cleaner is giving "Project" objects a body, with "slugs" and "description" separated by newlines? [ ] `PhabricatorSearchEngineTestCase` is now pointless and only detects local misconfigurations. [ ] It would be nice to build a practical test suite instead, where we put specific documents into the index and then search for them. The upstream test could run against MySQL, and some `bin/search test` could run against a configured engine like ElasticSearch. This would make it easier to make sure that behavior was as uniform as possible across engine implementations. [ ] Does every assigned task now match "user" in ElasticSearch? [x] `PhabricatorElasticFulltextStorageEngine` has a `json_encode()` which should be `phutil_json_encode()`. [ ] `PhabricatorSearchService` throws an untranslated exception. [ ] When a search cluster is down, we probably don't degrade with much grace (unhandled exception)? [ ] I haven't run bin/search init, but bin/search index doesn't warn me that I may want to. This might be worth adding. The UI does warn me. [ ] bin/search init warns me that the index is "incorrect". It might be more clear to distinguish between "missing" and "incorrect", since it's more comforting to users to see "everything is as we expect, doing normal first-time setup now" than "something is wrong, fixing it". [ ] CLI message "Initializing search service "ElasticSearch"" does not end with a period, which is inconsistent with other UI messages. [ ] It might be nice to let bin/search commands like init and index select a specific service (or even service + host) to act on, as bin/storage --ref ... now does. You can generally get the result you want by fiddling with config. [ ] When a service isn't writable, bin/search init reports "Search cluster has no hosts for role "write".". This is accurate but does not provide guidance: it might be more useful to the user to explain "This service is not writable, so we're skipping index check for it.". [x] Even with write off for MySQL, bin/search index --type task --trace still updates MySQL, I think? I may be misreading the trace output. But this behavior doesn't make sense if it is the actual behavior, and it seems like reindexAbstractDocument() uses "all services", not "writable services", and the MySQL engine doesn't make sure it's writable before indexing. [x] Searching or user fails to find task Grant users tokens when a mention is created, suggesting that stemming is not working. [x] Searching for users finds that task, but fails to find a task containing "per user per month" in a comment, also suggesting that stemming is not working. [x] Searching for maniphest fails to find task maniphest.query elephant, suggesting that tokenization in ElasticSearch is not as good as the MySQL tokenization for these words (see D17330). [x] The "index incorrect" warning UI uses inconsistent title case. [x] The "index incorrect" warning UI could format the command to be run more cleanly (with addCommand(), I think). refs T12450 Test Plan: * Stared blankly at the code. * Disabled 'write' role on mysql fulltext service. * Edited a task, ran search indexer, verified that the mysql index wasn't being updated. Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17564 --- .../PhabricatorElasticSearchSetupCheck.php | 16 +++--- ...habricatorElasticFulltextStorageEngine.php | 55 +++++++++++++++++-- .../search/PhabricatorMySQLSearchHost.php | 9 +++ .../cluster/search/PhabricatorSearchHost.php | 40 -------------- .../search/PhabricatorSearchService.php | 25 +++++---- 5 files changed, 80 insertions(+), 65 deletions(-) diff --git a/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php b/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php index d8864b5740..dab886b92a 100644 --- a/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php +++ b/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php @@ -49,26 +49,28 @@ final class PhabricatorElasticSearchSetupCheck extends PhabricatorSetupCheck { $message = pht( 'You likely enabled cluster.search without creating the '. - 'index. Run `./bin/search init` to correct the index.'); + 'index. Use the following command to create a new index.'); $this ->newIssue('elastic.missing-index') - ->setName(pht('Elasticsearch index Not Found')) + ->setName(pht('Elasticsearch Index Not Found')) + ->addCommand('./bin/search init') ->setSummary($summary) - ->setMessage($message) - ->addRelatedPhabricatorConfig('cluster.search'); + ->setMessage($message); + } else if (!$index_sane) { $summary = pht( 'Elasticsearch index exists but needs correction.'); $message = pht( 'Either the Phabricator schema for Elasticsearch has changed '. - 'or Elasticsearch created the index automatically. Run '. - '`./bin/search init` to correct the index.'); + 'or Elasticsearch created the index automatically. '. + 'Use the following command to rebuild the index.'); $this ->newIssue('elastic.broken-index') - ->setName(pht('Elasticsearch index Incorrect')) + ->setName(pht('Elasticsearch Index Schema Mismatch')) + ->addCommand('./bin/search init') ->setSummary($summary) ->setMessage($message); } diff --git a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php index bc32da5ef4..6d2cd9adc5 100644 --- a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php +++ b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php @@ -161,9 +161,11 @@ class PhabricatorElasticFulltextStorageEngine 'simple_query_string' => array( 'query' => $query_string, 'fields' => array( - '_all', + PhabricatorSearchDocumentFieldType::FIELD_TITLE.'.*', + PhabricatorSearchDocumentFieldType::FIELD_BODY.'.*', + PhabricatorSearchDocumentFieldType::FIELD_COMMENT.'.*', ), - 'default_operator' => 'OR', + 'default_operator' => 'AND', ), )); @@ -175,6 +177,7 @@ class PhabricatorElasticFulltextStorageEngine 'simple_query_string' => array( 'query' => $query_string, 'fields' => array( + '*.raw', PhabricatorSearchDocumentFieldType::FIELD_TITLE.'^4', PhabricatorSearchDocumentFieldType::FIELD_BODY.'^3', PhabricatorSearchDocumentFieldType::FIELD_COMMENT.'^1.2', @@ -332,11 +335,38 @@ class PhabricatorElasticFulltextStorageEngine 'index' => array( 'auto_expand_replicas' => '0-2', 'analysis' => array( + 'filter' => array( + 'english_stop' => array( + 'type' => 'stop', + 'stopwords' => '_english_', + ), + 'english_stemmer' => array( + 'type' => 'stemmer', + 'language' => 'english', + ), + 'english_possessive_stemmer' => array( + 'type' => 'stemmer', + 'language' => 'possessive_english', + ), + ), 'analyzer' => array( 'english_exact' => array( 'tokenizer' => 'standard', 'filter' => array('lowercase'), ), + 'letter_stop' => array( + 'tokenizer' => 'letter', + 'filter' => array('lowercase', 'english_stop'), + ), + 'english_stem' => array( + 'tokenizer' => 'standard', + 'filter' => array( + 'english_possessive_stemmer', + 'lowercase', + 'english_stop', + 'english_stemmer', + ), + ), ), ), ), @@ -356,9 +386,22 @@ class PhabricatorElasticFulltextStorageEngine // Use the custom analyzer for the corpus of text $properties[$field] = array( 'type' => $text_type, - 'analyzer' => 'english_exact', - 'search_analyzer' => 'english', - 'search_quote_analyzer' => 'english_exact', + 'fields' => array( + 'raw' => array( + 'type' => $text_type, + 'analyzer' => 'english_exact', + 'search_analyzer' => 'english', + 'search_quote_analyzer' => 'english_exact', + ), + 'keywords' => array( + 'type' => $text_type, + 'analyzer' => 'letter_stop', + ), + 'stems' => array( + 'type' => $text_type, + 'analyzer' => 'english_stem', + ), + ), ); } @@ -505,7 +548,7 @@ class PhabricatorElasticFulltextStorageEngine array $data, $method = 'GET') { $uri = $host->getURI($path); - $data = json_encode($data); + $data = phutil_json_encode($data); $future = new HTTPSFuture($uri, $data); if ($method != 'GET') { $future->setMethod($method); diff --git a/src/infrastructure/cluster/search/PhabricatorMySQLSearchHost.php b/src/infrastructure/cluster/search/PhabricatorMySQLSearchHost.php index ced23cd542..742b5713d3 100644 --- a/src/infrastructure/cluster/search/PhabricatorMySQLSearchHost.php +++ b/src/infrastructure/cluster/search/PhabricatorMySQLSearchHost.php @@ -24,6 +24,15 @@ final class PhabricatorMySQLSearchHost return 'mysql'; } + public function getHealthRecord() { + if (!$this->healthRecord) { + $ref = PhabricatorDatabaseRef::getMasterDatabaseRefForApplication( + 'search'); + $this->healthRecord = $ref->getHealthRecord(); + } + return $this->healthRecord; + } + public function getConnectionStatus() { PhabricatorDatabaseRef::queryAll(); $ref = PhabricatorDatabaseRef::getMasterDatabaseRefForApplication('search'); diff --git a/src/infrastructure/cluster/search/PhabricatorSearchHost.php b/src/infrastructure/cluster/search/PhabricatorSearchHost.php index 834e786789..93c3c4d938 100644 --- a/src/infrastructure/cluster/search/PhabricatorSearchHost.php +++ b/src/infrastructure/cluster/search/PhabricatorSearchHost.php @@ -13,7 +13,6 @@ abstract class PhabricatorSearchHost protected $disabled; protected $host; protected $port; - protected $hostRefs = array(); const STATUS_OKAY = 'okay'; const STATUS_FAIL = 'fail'; @@ -121,43 +120,4 @@ abstract class PhabricatorSearchHost abstract public function getConnectionStatus(); - public static function reindexAbstractDocument( - PhabricatorSearchAbstractDocument $doc) { - - $services = self::getAllServices(); - $indexed = 0; - foreach (self::getWritableHostForEachService() as $host) { - $host->getEngine()->reindexAbstractDocument($doc); - $indexed++; - } - if ($indexed == 0) { - throw new PhabricatorClusterNoHostForRoleException('write'); - } - } - - public static function executeSearch(PhabricatorSavedQuery $query) { - $services = self::getAllServices(); - foreach ($services as $service) { - $hosts = $service->getAllHostsForRole('read'); - // try all hosts until one succeeds - foreach ($hosts as $host) { - $last_exception = null; - try { - $res = $host->getEngine()->executeSearch($query); - // return immediately if we get results without an exception - $host->didHealthCheck(true); - return $res; - } catch (Exception $ex) { - // try each server in turn, only throw if none succeed - $last_exception = $ex; - $host->didHealthCheck(false); - } - } - } - if ($last_exception) { - throw $last_exception; - } - return $res; - } - } diff --git a/src/infrastructure/cluster/search/PhabricatorSearchService.php b/src/infrastructure/cluster/search/PhabricatorSearchService.php index 20c0664456..0a563207b1 100644 --- a/src/infrastructure/cluster/search/PhabricatorSearchService.php +++ b/src/infrastructure/cluster/search/PhabricatorSearchService.php @@ -46,6 +46,7 @@ class PhabricatorSearchService public function setConfig($config) { $this->config = $config; + $this->setRoles(idx($config, 'roles', array())); if (!isset($config['hosts'])) { $config['hosts'] = array( @@ -67,15 +68,6 @@ class PhabricatorSearchService return $this->config; } - public function setDisabled($disabled) { - $this->disabled = $disabled; - return $this; - } - - public function getDisabled() { - return $this->disabled; - } - public static function getConnectionStatusMap() { return array( self::STATUS_OKAY => array( @@ -100,7 +92,7 @@ class PhabricatorSearchService } public function hasRole($role) { - return isset($this->roles[$role]) && $this->roles[$role] === true; + return isset($this->roles[$role]) && $this->roles[$role] !== false; } public function setRoles(array $roles) { @@ -160,6 +152,12 @@ class PhabricatorSearchService * @return PhabricatorSearchHost[] */ public function getAllHostsForRole($role) { + // if the role is explicitly set to false at the top level, then all hosts + // have the role disabled. + if (idx($this->config, $role) === false) { + return array(); + } + $hosts = array(); foreach ($this->hosts as $host) { if ($host->hasRole($role)) { @@ -225,8 +223,11 @@ class PhabricatorSearchService PhabricatorSearchAbstractDocument $doc) { $indexed = 0; foreach (self::getAllServices() as $service) { - $service->getEngine()->reindexAbstractDocument($doc); - $indexed++; + $hosts = $service->getAllHostsForRole('write'); + if (count($hosts)) { + $service->getEngine()->reindexAbstractDocument($doc); + $indexed++; + } } if ($indexed == 0) { throw new PhabricatorClusterNoHostForRoleException('write'); From e7c76d92d546b2b331a3aa1b10fb182b4320f4f9 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 12:47:58 -0700 Subject: [PATCH 13/39] Make `bin/search init` messaging a little more consistent Summary: Ref T12450. This mostly just smooths out the text a little to improve consistency. Also: - Use `isWritable()`. - Make the "skipping because not writable" message more clear and tailored. - Try not to use the word "index" too much to avoid confusion with `bin/search index` -- instead, talk about "initialize a service". Test Plan: Ran `bin/search init` with a couple of different (writable / not writable) configs, saw slightly clearer messaging. Reviewers: chad, 20after4 Reviewed By: 20after4 Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17572 --- ...habricatorSearchManagementInitWorkflow.php | 67 ++++++++++--------- 1 file changed, 36 insertions(+), 31 deletions(-) diff --git a/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php index 1b5da49e66..c984b2ddbf 100644 --- a/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php @@ -6,60 +6,65 @@ final class PhabricatorSearchManagementInitWorkflow protected function didConstruct() { $this ->setName('init') - ->setSynopsis(pht('Initialize or repair an index.')) + ->setSynopsis(pht('Initialize or repair a search service.')) ->setExamples('**init**'); } public function execute(PhutilArgumentParser $args) { - $console = PhutilConsole::getConsole(); $work_done = false; foreach (PhabricatorSearchService::getAllServices() as $service) { - $console->writeOut( + echo tsprintf( "%s\n", - pht('Initializing search service "%s"', $service->getDisplayName())); + pht( + 'Initializing search service "%s".', + $service->getDisplayName())); - try { - $host = $service->getAnyHostForRole('write'); - } catch (PhabricatorClusterNoHostForRoleException $e) { - // If there are no writable hosts for a given cluster, skip it - $console->writeOut("%s\n", $e->getMessage()); + if (!$service->isWritable()) { + echo tsprintf( + "%s\n", + pht( + 'Skipping service "%s" because it is not writable.', + $service->getDisplayName())); continue; } - $engine = $host->getEngine(); + $engine = $service->getEngine(); if (!$engine->indexExists()) { - $console->writeOut( - '%s', - pht('Index does not exist, creating...')); - $engine->initIndex(); - $console->writeOut( + echo tsprintf( "%s\n", - pht('done.')); + pht('Service index does not exist, creating...')); + + $engine->initIndex(); $work_done = true; } else if (!$engine->indexIsSane()) { - $console->writeOut( - '%s', - pht('Index exists but is incorrect, fixing...')); - $engine->initIndex(); - $console->writeOut( + echo tsprintf( "%s\n", - pht('done.')); + pht('Service index is out of date, repairing...')); + + $engine->initIndex(); $work_done = true; + } else { + echo tsprintf( + "%s\n", + pht('Service index is already up to date.')); } + + echo tsprintf( + "%s\n", + pht('Done.')); } - if ($work_done) { - $console->writeOut( + if (!$work_done) { + echo tsprintf( "%s\n", - pht( - 'Index maintenance complete. Run `%s` to reindex documents', - './bin/search index')); - } else { - $console->writeOut( - "%s\n", - pht('Nothing to do.')); + pht('No services need initialization.')); + return 0; } + + echo tsprintf( + "%s\n", + pht('Service initialization complete.')); } } From c22693ff2915ac03c34d5e22ebaaa4fe2355b8b1 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 12:56:15 -0700 Subject: [PATCH 14/39] Remove PhabricatorSearchEngineTestCase Summary: Ref T12450. This is now pointless and just asserts that `cluster.search` has a default value. We might restore a fancier version of this eventually, but get rid of this for now. Test Plan: Scruitinized the test case. Reviewers: chad, 20after4 Reviewed By: 20after4 Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17573 --- src/__phutil_library_map__.php | 2 -- .../__tests__/PhabricatorSearchEngineTestCase.php | 10 ---------- 2 files changed, 12 deletions(-) delete mode 100644 src/applications/search/engine/__tests__/PhabricatorSearchEngineTestCase.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0921fa21e9..f9d013f497 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -3790,7 +3790,6 @@ phutil_register_library_map(array( 'PhabricatorSearchEngineAttachment' => 'applications/search/engineextension/PhabricatorSearchEngineAttachment.php', 'PhabricatorSearchEngineExtension' => 'applications/search/engineextension/PhabricatorSearchEngineExtension.php', 'PhabricatorSearchEngineExtensionModule' => 'applications/search/engineextension/PhabricatorSearchEngineExtensionModule.php', - 'PhabricatorSearchEngineTestCase' => 'applications/search/engine/__tests__/PhabricatorSearchEngineTestCase.php', 'PhabricatorSearchField' => 'applications/search/field/PhabricatorSearchField.php', 'PhabricatorSearchHost' => 'infrastructure/cluster/search/PhabricatorSearchHost.php', 'PhabricatorSearchHovercardController' => 'applications/search/controller/PhabricatorSearchHovercardController.php', @@ -9111,7 +9110,6 @@ phutil_register_library_map(array( 'PhabricatorSearchEngineAttachment' => 'Phobject', 'PhabricatorSearchEngineExtension' => 'Phobject', 'PhabricatorSearchEngineExtensionModule' => 'PhabricatorConfigModule', - 'PhabricatorSearchEngineTestCase' => 'PhabricatorTestCase', 'PhabricatorSearchField' => 'Phobject', 'PhabricatorSearchHost' => 'Phobject', 'PhabricatorSearchHovercardController' => 'PhabricatorSearchBaseController', diff --git a/src/applications/search/engine/__tests__/PhabricatorSearchEngineTestCase.php b/src/applications/search/engine/__tests__/PhabricatorSearchEngineTestCase.php deleted file mode 100644 index f5dbd9ef9c..0000000000 --- a/src/applications/search/engine/__tests__/PhabricatorSearchEngineTestCase.php +++ /dev/null @@ -1,10 +0,0 @@ -assertTrue(!empty($services)); - } - -} From c40be811ea9b2419335fce7fe16096a16e7d6b04 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 12:47:56 -0700 Subject: [PATCH 15/39] Fix isReadable() and isWritable() in SearchService Summary: Ref T12450. Minor cleanup: - setRoles() has no callers. - getRoles() has no callers (these two methods are leftovers from an earlier iteration of the change). - The `hasRole()` logic doesn't work since nothing calls `setRole()`. - `hasRole()` has only `isreadable/iswritable` as callers. - The `isReadable()/isWritable()` logic doesn't work since `hasRole()` doesn't work. Instead, just check if there are any readable/writable hosts. `Host` already inherits its config from `Service` so this gets the same answer without any fuss. Also add some read/write constants to make grepping this stuff a little easier. Test Plan: - Grepped for all removed symbols, saw only newer-generation calls in `Host`. - See next diff for use of `isWritable()`. Reviewers: chad, 20after4 Reviewed By: 20after4 Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17571 --- .../search/PhabricatorSearchService.php | 26 ++++--------------- 1 file changed, 5 insertions(+), 21 deletions(-) diff --git a/src/infrastructure/cluster/search/PhabricatorSearchService.php b/src/infrastructure/cluster/search/PhabricatorSearchService.php index 0a563207b1..b4f6b76aec 100644 --- a/src/infrastructure/cluster/search/PhabricatorSearchService.php +++ b/src/infrastructure/cluster/search/PhabricatorSearchService.php @@ -16,6 +16,9 @@ class PhabricatorSearchService const STATUS_OKAY = 'okay'; const STATUS_FAIL = 'fail'; + const ROLE_WRITE = 'write'; + const ROLE_READ = 'read'; + public function __construct(PhabricatorFulltextStorageEngine $engine) { $this->engine = $engine; $this->hostType = $engine->getHostType(); @@ -84,30 +87,11 @@ class PhabricatorSearchService } public function isWritable() { - return $this->hasRole('write'); + return (bool)$this->getAllHostsForRole(self::ROLE_WRITE); } public function isReadable() { - return $this->hasRole('read'); - } - - public function hasRole($role) { - return isset($this->roles[$role]) && $this->roles[$role] !== false; - } - - public function setRoles(array $roles) { - foreach ($roles as $role => $val) { - if ($val === false && isset($this->roles[$role])) { - unset($this->roles[$role]); - } else { - $this->roles[$role] = $val; - } - } - return $this; - } - - public function getRoles() { - return $this->roles; + return (bool)$this->getAllHostsForRole(self::ROLE_READ); } public function getPort() { From 8879118b696f51ea8797e52b1a686bfc8ee1d7f8 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 13:59:59 -0700 Subject: [PATCH 16/39] Fix a mid-air collision around SearchService roles My D17571 didn't interact nicely with D17564, which added callsites for one of the methods I removed. Auditors: 20after4 --- src/infrastructure/cluster/search/PhabricatorSearchService.php | 1 - 1 file changed, 1 deletion(-) diff --git a/src/infrastructure/cluster/search/PhabricatorSearchService.php b/src/infrastructure/cluster/search/PhabricatorSearchService.php index b4f6b76aec..2085f1d887 100644 --- a/src/infrastructure/cluster/search/PhabricatorSearchService.php +++ b/src/infrastructure/cluster/search/PhabricatorSearchService.php @@ -49,7 +49,6 @@ class PhabricatorSearchService public function setConfig($config) { $this->config = $config; - $this->setRoles(idx($config, 'roles', array())); if (!isset($config['hosts'])) { $config['hosts'] = array( From 5f939dcce0f850cbadeea25bd4ab4eae1f8e6417 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 13:09:27 -0700 Subject: [PATCH 17/39] Re-run config validation from `bin/search` Summary: Ref T12450. Normally, we validate config when: - You restart the webserver. - You edit it with `bin/config set ...`. - You edit it with the web UI. However, you can also change config by editing `local.json`, `some_env.conf.php`, a `SiteConfig` class, etc. In these cases, you may miss config warnings. Explicitly re-run search config checks from `bin/search`, similar to the additional database checks we run from `bin/storage`, to try to produce a better error message if the user has made a configuration error. Test Plan: ``` $ ./bin/search init Usage Exception: Setting "cluster.search" is misconfigured: Invalid search engine type: elastic. Valid types are: elasticsearch, mysql. ``` Reviewers: chad, 20after4 Reviewed By: 20after4 Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17574 --- ...abricatorSearchManagementIndexWorkflow.php | 2 ++ ...habricatorSearchManagementInitWorkflow.php | 1 + .../PhabricatorSearchManagementWorkflow.php | 24 ++++++++++++++++++- ...abricatorClusterSearchConfigOptionType.php | 7 +++++- 4 files changed, 32 insertions(+), 2 deletions(-) diff --git a/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php index 7838808f48..a324a20637 100644 --- a/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php @@ -45,6 +45,8 @@ final class PhabricatorSearchManagementIndexWorkflow } public function execute(PhutilArgumentParser $args) { + $this->validateClusterSearchConfig(); + $console = PhutilConsole::getConsole(); $is_all = $args->getArg('all'); diff --git a/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php index c984b2ddbf..8728b72ee5 100644 --- a/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementInitWorkflow.php @@ -11,6 +11,7 @@ final class PhabricatorSearchManagementInitWorkflow } public function execute(PhutilArgumentParser $args) { + $this->validateClusterSearchConfig(); $work_done = false; foreach (PhabricatorSearchService::getAllServices() as $service) { diff --git a/src/applications/search/management/PhabricatorSearchManagementWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementWorkflow.php index 23333665e3..86d8c104cb 100644 --- a/src/applications/search/management/PhabricatorSearchManagementWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementWorkflow.php @@ -1,4 +1,26 @@ getMessage())); + } + } + +} diff --git a/src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php b/src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php index 4a5f7ea6c5..3c099a0548 100644 --- a/src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php +++ b/src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php @@ -4,6 +4,10 @@ final class PhabricatorClusterSearchConfigOptionType extends PhabricatorConfigJSONOptionType { public function validateOption(PhabricatorConfigOption $option, $value) { + self::validateClusterSearchConfigValue($value); + } + + public static function validateValue($value) { if (!is_array($value)) { throw new Exception( pht( @@ -46,7 +50,8 @@ final class PhabricatorClusterSearchConfigOptionType if (!array_key_exists($spec['type'], $engines)) { throw new Exception( - pht('Invalid search engine type: %s. Valid types include: %s', + pht( + 'Invalid search engine type: %s. Valid types are: %s.', $spec['type'], implode(', ', array_keys($engines)))); } From 88798354e8c91554b15dd38b63732dfdda672a78 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 15:10:59 -0700 Subject: [PATCH 18/39] Soften a possible cluster search setup fatal Summary: Ref T12450. The way that config repair and setup issues interact is kind of complicated, and if `cluster.search` is invalid we may end up using `cluster.search` before we repair it. I poked at things for a bit but wasn't confident I could get it to consistently repair before we use it without doing a big messy change. The only thing that really matters is whether "type" is valid or not, so just put a slightly softer/more-tailored check in for that. Test Plan: - With `"type": "elastic"`, loaded setup issues. - Before patch: hard fatal. - After patch: softer fatal with more useful messaging. {F4321048} Reviewers: chad Reviewed By: chad Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17576 --- .../PhabricatorClusterSearchConfigOptionType.php | 2 +- .../cluster/search/PhabricatorSearchService.php | 12 ++++++++++++ 2 files changed, 13 insertions(+), 1 deletion(-) diff --git a/src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php b/src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php index 3c099a0548..90ead23e6d 100644 --- a/src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php +++ b/src/infrastructure/cluster/config/PhabricatorClusterSearchConfigOptionType.php @@ -4,7 +4,7 @@ final class PhabricatorClusterSearchConfigOptionType extends PhabricatorConfigJSONOptionType { public function validateOption(PhabricatorConfigOption $option, $value) { - self::validateClusterSearchConfigValue($value); + self::validateValue($value); } public static function validateValue($value) { diff --git a/src/infrastructure/cluster/search/PhabricatorSearchService.php b/src/infrastructure/cluster/search/PhabricatorSearchService.php index 2085f1d887..ed34f5cdf6 100644 --- a/src/infrastructure/cluster/search/PhabricatorSearchService.php +++ b/src/infrastructure/cluster/search/PhabricatorSearchService.php @@ -186,6 +186,18 @@ class PhabricatorSearchService $refs = array(); foreach ($services as $config) { + + // Normally, we've validated configuration before we get this far, but + // make sure we don't fatal if we end up here with a bogus configuration. + if (!isset($engines[$config['type']])) { + throw new Exception( + pht( + 'Configured search engine type "%s" is unknown. Valid engines '. + 'are: %s.', + $config['type'], + implode(', ', array_keys($engines)))); + } + $engine = $engines[$config['type']]; $cluster = new self($engine); $cluster->setConfig($config); From add103810989ede8424f081b529159284669b50a Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 28 Mar 2017 15:33:32 -0700 Subject: [PATCH 19/39] Don't summon the emoji autocompleter for ":3" Summary: Fixes T12460. Also ":)", ":(", ":/", and oldschool ":-)" variants. Not included are variants with actual letters (`:D`, `:O`, `:P`) and obscure variants (`:^)`, `:*)`). Test Plan: Typed `:3` (no emoji summoned). Typed `:dog3` (emoji summoned). Typed `@3` (user autocomplete summoned). Reviewers: chad Reviewed By: chad Maniphest Tasks: T12460 Differential Revision: https://secure.phabricator.com/D17577 --- resources/celerity/map.php | 16 ++++++++-------- .../form/control/PhabricatorRemarkupControl.php | 9 +++++++++ webroot/rsrc/js/phuix/PHUIXAutocomplete.js | 12 ++++++++++++ 3 files changed, 29 insertions(+), 8 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 8a4146b0a3..f83c2b0f09 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -528,7 +528,7 @@ return array( 'rsrc/js/phui/behavior-phui-tab-group.js' => '0a0b10e9', 'rsrc/js/phuix/PHUIXActionListView.js' => 'b5c256b8', 'rsrc/js/phuix/PHUIXActionView.js' => 'b3465b9b', - 'rsrc/js/phuix/PHUIXAutocomplete.js' => '7c492cd2', + 'rsrc/js/phuix/PHUIXAutocomplete.js' => '7910aacb', 'rsrc/js/phuix/PHUIXDropdownMenu.js' => '8018ee50', 'rsrc/js/phuix/PHUIXFormControl.js' => '83e03671', 'rsrc/js/phuix/PHUIXIconView.js' => 'bff6884b', @@ -885,7 +885,7 @@ return array( 'phui-workpanel-view-css' => 'a3a63478', 'phuix-action-list-view' => 'b5c256b8', 'phuix-action-view' => 'b3465b9b', - 'phuix-autocomplete' => '7c492cd2', + 'phuix-autocomplete' => '7910aacb', 'phuix-dropdown-menu' => '8018ee50', 'phuix-form-control-view' => '83e03671', 'phuix-icon-view' => 'bff6884b', @@ -1456,6 +1456,12 @@ return array( 'multirow-row-manager', 'javelin-json', ), + '7910aacb' => array( + 'javelin-install', + 'javelin-dom', + 'phuix-icon-view', + 'phabricator-prefab', + ), '7927a7d3' => array( 'javelin-behavior', 'javelin-quicksand', @@ -1464,12 +1470,6 @@ return array( 'owners-path-editor', 'javelin-behavior', ), - '7c492cd2' => array( - 'javelin-install', - 'javelin-dom', - 'phuix-icon-view', - 'phabricator-prefab', - ), '7cbe244b' => array( 'javelin-install', 'javelin-util', diff --git a/src/view/form/control/PhabricatorRemarkupControl.php b/src/view/form/control/PhabricatorRemarkupControl.php index 52fb44617d..75054b9575 100644 --- a/src/view/form/control/PhabricatorRemarkupControl.php +++ b/src/view/form/control/PhabricatorRemarkupControl.php @@ -97,6 +97,15 @@ final class PhabricatorRemarkupControl extends AphrontFormTextAreaControl { 'headerIcon' => 'fa-smile-o', 'headerText' => pht('Find Emoji:'), 'hintText' => $emoji_datasource->getPlaceholderText(), + + // Cancel on emoticons like ":3". + 'ignore' => array( + '3', + ')', + '(', + '-', + '/', + ), ), ), )); diff --git a/webroot/rsrc/js/phuix/PHUIXAutocomplete.js b/webroot/rsrc/js/phuix/PHUIXAutocomplete.js index b7116c557b..e99dcc34f2 100644 --- a/webroot/rsrc/js/phuix/PHUIXAutocomplete.js +++ b/webroot/rsrc/js/phuix/PHUIXAutocomplete.js @@ -343,6 +343,10 @@ JX.install('PHUIXAutocomplete', { return [' ', ':', ',', '.', '!', '?']; }, + _getIgnoreList: function() { + return this._map[this._active].ignore || []; + }, + _isTerminatedString: function(string) { var terminators = this._getTerminators(); for (var ii = 0; ii < terminators.length; ii++) { @@ -517,6 +521,14 @@ JX.install('PHUIXAutocomplete', { } } + var ignore = this._getIgnoreList(); + for (ii = 0; ii < ignore.length; ii++) { + if (trim.indexOf(ignore[ii]) === 0) { + this._deactivate(); + return; + } + } + // If the input is terminated by a space or another word-terminating // punctuation mark, we're going to deactivate if the results can not // be refined by addding more words. From 654f0f6043f810ca4c9deef10f97178dc5b811b9 Mon Sep 17 00:00:00 2001 From: Mukunda Modell Date: Tue, 28 Mar 2017 23:17:35 +0000 Subject: [PATCH 20/39] Make messages translatable and more sensible. Summary: These exception messages & comments didn't quite match reality. Fixed and added pht() around a couple of them. Test Plan: I didn't test this :P Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D17578 --- ...habricatorElasticFulltextStorageEngine.php | 3 +- src/docs/user/cluster/cluster.diviner | 28 +++++++++++++++++++ .../search/PhabricatorSearchService.php | 13 ++++----- 3 files changed, 36 insertions(+), 8 deletions(-) diff --git a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php index 6d2cd9adc5..aa966caddc 100644 --- a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php +++ b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php @@ -303,7 +303,8 @@ class PhabricatorElasticFulltextStorageEngine $exceptions[] = $e; } } - throw new PhutilAggregateException('All search hosts failed:', $exceptions); + throw new PhutilAggregateException(pht('All Fulltext Search hosts failed:'), + $exceptions); } public function indexExists(PhabricatorElasticSearchHost $host = null) { diff --git a/src/docs/user/cluster/cluster.diviner b/src/docs/user/cluster/cluster.diviner index 7704428e9c..15eed86eb4 100644 --- a/src/docs/user/cluster/cluster.diviner +++ b/src/docs/user/cluster/cluster.diviner @@ -47,6 +47,7 @@ will have on availability, resistance to data loss, and scalability. | **SSH Servers** | Minimal | Low | No Risk | Low | **Web Servers** | Minimal | **High** | No Risk | Moderate | **Notifications** | Minimal | Low | No Risk | Low +| **Fulltext Search** | Moderate | **High** | Minimal Risk | Moderate See below for a walkthrough of these services in greater detail. @@ -237,6 +238,33 @@ hosts is unlikely to have much impact on scalability. For details, see @{article:Cluster: Notifications}. +Cluster: Fulltext Search +======================== + +At a certain scale, you may begin to bump up against the limitations of MySQL's +built-in fulltext search capabilities. We have seen this with very large +installations with several million objects in the database and very many +simultaneous requests. At this point you may consider adding Elasticsearch +hosts to your cluster to reduce the load on your MySQL hosts. + +Elasticsearch has the ability to spread the load across multiple hosts and can +handle very large indexes by sharding. + +Search does not involve any risk of data lost because it's always possible to +rebuild the search index from the original database objects. This process can +be very time consuming, however, especially when the database grows very large. + +With multiple Elasticsearch hosts, you can survive the loss of a single host +with minimal disruption as Phabricator will detect the problem and direct +queries to one of the remaining hosts. + +Phabricator supports writing to multiple indexing servers. This Simplifies +Elasticsearch upgrades and makes it possible to recover more quickly from +problems with the search index. + +For details, see @{article:Cluster: Search}. + + Overlaying Services =================== diff --git a/src/infrastructure/cluster/search/PhabricatorSearchService.php b/src/infrastructure/cluster/search/PhabricatorSearchService.php index ed34f5cdf6..59ef7c408d 100644 --- a/src/infrastructure/cluster/search/PhabricatorSearchService.php +++ b/src/infrastructure/cluster/search/PhabricatorSearchService.php @@ -235,21 +235,20 @@ class PhabricatorSearchService * @throws PhutilAggregateException */ public static function executeSearch(PhabricatorSavedQuery $query) { - $services = self::getAllServices(); $exceptions = array(); - foreach ($services as $service) { - $engine = $service->getEngine(); - // try all hosts until one succeeds + // try all services until one succeeds + foreach (self::getAllServices() as $service) { try { + $engine = $service->getEngine(); $res = $engine->executeSearch($query); - // return immediately if we get results without an exception + // return immediately if we get results return $res; } catch (Exception $ex) { $exceptions[] = $ex; } } - throw new PhutilAggregateException('All search engines failed:', - $exceptions); + $msg = pht('All of the configured Fulltext Search services failed.'); + throw new PhutilAggregateException($msg, $exceptions); } } From 67a1c40476474ad741a26e067f437f1e31392ded Mon Sep 17 00:00:00 2001 From: Mukunda Modell Date: Thu, 30 Mar 2017 18:07:47 +0000 Subject: [PATCH 21/39] Set content-type to application/json Summary: Elasticsearch really wants a raw json body and it fails to accept the request as of es version 5.3 Test Plan: Tested with elasticsearch 5.2 and 5.3. Before this change 5.2 worked but 5.3 failed with `HTTP/406 "Content-Type header [application/x-www-form-urlencoded] is not supported"` [1] After this change, both worked. [1] https://phabricator.wikimedia.org/P5158 Reviewers: epriestley, #blessed_reviewers Reviewed By: epriestley, #blessed_reviewers Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D17580 --- .../fulltextstorage/PhabricatorElasticFulltextStorageEngine.php | 2 ++ 1 file changed, 2 insertions(+) diff --git a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php index aa966caddc..2e456ec408 100644 --- a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php +++ b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php @@ -551,6 +551,8 @@ class PhabricatorElasticFulltextStorageEngine $uri = $host->getURI($path); $data = phutil_json_encode($data); $future = new HTTPSFuture($uri, $data); + $future->addHeader('Content-Type', 'application/json'); + if ($method != 'GET') { $future->setMethod($method); } From cb1d90465447c8a7cc5bc42aa2566c12692fec25 Mon Sep 17 00:00:00 2001 From: Mukunda Modell Date: Thu, 30 Mar 2017 18:08:05 +0000 Subject: [PATCH 22/39] Make sure writes go to the right cluster Summary: Two little issues 1. there was an extra call to getHostForWrite, 2. The engine instance was shared between multiple service definitions so it was overwriting the list of writable hosts from one service with hosts from another. Test Plan: tested in wikimedia production with multiple services defined like this: ```language=json [ { "hosts": [ { "host": "search.svc.codfw.wmnet", "protocol": "https", "roles": { "read": true, "write": true }, "version": 5 } ], "path": "/phabricator", "port": 9243, "type": "elasticsearch" }, { "hosts": [ { "host": "search.svc.eqiad.wmnet", "protocol": "https", "roles": { "read": true, "write": true }, "version": 5 } ], "path": "/phabricator", "port": 9243, "type": "elasticsearch" } ] ``` Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D17581 --- .../fulltextstorage/PhabricatorElasticFulltextStorageEngine.php | 1 - src/infrastructure/cluster/search/PhabricatorSearchService.php | 2 +- 2 files changed, 1 insertion(+), 2 deletions(-) diff --git a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php index 2e456ec408..45011ba982 100644 --- a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php +++ b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php @@ -528,7 +528,6 @@ class PhabricatorElasticFulltextStorageEngine $host = $this->getHostForRead(); } $uri = '/_stats/'; - $host = $this->getHostForRead(); $res = $this->executeRequest($host, $uri, array()); $stats = $res['indices'][$this->index]; diff --git a/src/infrastructure/cluster/search/PhabricatorSearchService.php b/src/infrastructure/cluster/search/PhabricatorSearchService.php index 59ef7c408d..10cf78d94b 100644 --- a/src/infrastructure/cluster/search/PhabricatorSearchService.php +++ b/src/infrastructure/cluster/search/PhabricatorSearchService.php @@ -198,7 +198,7 @@ class PhabricatorSearchService implode(', ', array_keys($engines)))); } - $engine = $engines[$config['type']]; + $engine = clone($engines[$config['type']]); $cluster = new self($engine); $cluster->setConfig($config); $engine->setService($cluster); From 130ebd2c421a0bd0ae0f84ab60acc00ff0436850 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 30 Mar 2017 08:32:41 -0700 Subject: [PATCH 23/39] Immediately deactivate remarkup autocomplete if there's no query Summary: Fixes T12479. If you end a line with a character like ":" in a context which can trigger autocomplete (e.g., `.:`), then try to make a newline, we swallow the keystroke. Instead, allow the keystroke through if the user hasn't typed anything else yet. Test Plan: - Autocompleted emoji and users normally. - In an empty textarea, typed `.:`, got a newline instead of a swallowed keystroke. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12479 Differential Revision: https://secure.phabricator.com/D17583 --- resources/celerity/map.php | 16 ++++++------ webroot/rsrc/js/phuix/PHUIXAutocomplete.js | 29 ++++++++++++++-------- 2 files changed, 27 insertions(+), 18 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index f83c2b0f09..6bee132238 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -528,7 +528,7 @@ return array( 'rsrc/js/phui/behavior-phui-tab-group.js' => '0a0b10e9', 'rsrc/js/phuix/PHUIXActionListView.js' => 'b5c256b8', 'rsrc/js/phuix/PHUIXActionView.js' => 'b3465b9b', - 'rsrc/js/phuix/PHUIXAutocomplete.js' => '7910aacb', + 'rsrc/js/phuix/PHUIXAutocomplete.js' => 'd5b2abf3', 'rsrc/js/phuix/PHUIXDropdownMenu.js' => '8018ee50', 'rsrc/js/phuix/PHUIXFormControl.js' => '83e03671', 'rsrc/js/phuix/PHUIXIconView.js' => 'bff6884b', @@ -885,7 +885,7 @@ return array( 'phui-workpanel-view-css' => 'a3a63478', 'phuix-action-list-view' => 'b5c256b8', 'phuix-action-view' => 'b3465b9b', - 'phuix-autocomplete' => '7910aacb', + 'phuix-autocomplete' => 'd5b2abf3', 'phuix-dropdown-menu' => '8018ee50', 'phuix-form-control-view' => '83e03671', 'phuix-icon-view' => 'bff6884b', @@ -1456,12 +1456,6 @@ return array( 'multirow-row-manager', 'javelin-json', ), - '7910aacb' => array( - 'javelin-install', - 'javelin-dom', - 'phuix-icon-view', - 'phabricator-prefab', - ), '7927a7d3' => array( 'javelin-behavior', 'javelin-quicksand', @@ -2053,6 +2047,12 @@ return array( 'javelin-uri', 'phabricator-notification', ), + 'd5b2abf3' => array( + 'javelin-install', + 'javelin-dom', + 'phuix-icon-view', + 'phabricator-prefab', + ), 'd6a7e717' => array( 'multirow-row-manager', 'javelin-install', diff --git a/webroot/rsrc/js/phuix/PHUIXAutocomplete.js b/webroot/rsrc/js/phuix/PHUIXAutocomplete.js index e99dcc34f2..a03c2adf70 100644 --- a/webroot/rsrc/js/phuix/PHUIXAutocomplete.js +++ b/webroot/rsrc/js/phuix/PHUIXAutocomplete.js @@ -433,6 +433,16 @@ JX.install('PHUIXAutocomplete', { } } + // Deactivate if the user moves the cursor to the left of the assist + // range. For example, they might press the "left" arrow to move the + // cursor to the left, or click in the textarea prior to the active + // range. + var range = JX.TextAreaUtils.getSelectionRange(area); + if (range.start < this._cursorHead) { + this._deactivate(); + return; + } + if (special == 'tab' || special == 'return') { var r = e.getRawEvent(); if (r.shiftKey && special == 'tab') { @@ -443,6 +453,15 @@ JX.install('PHUIXAutocomplete', { return; } + // If the user hasn't typed any text yet after typing the character + // which can summon the autocomplete, deactivate and let the keystroke + // through. For example, We hit this when a line ends with an + // autocomplete character and the user is trying to type a newline. + if (range.start == this._cursorHead) { + this._deactivate(); + return; + } + // If we autocomplete, we're done. Otherwise, just eat the event. This // happens if you type too fast and try to tab complete before results // load. @@ -454,16 +473,6 @@ JX.install('PHUIXAutocomplete', { return; } - // Deactivate if the user moves the cursor to the left of the assist - // range. For example, they might press the "left" arrow to move the - // cursor to the left, or click in the textarea prior to the active - // range. - var range = JX.TextAreaUtils.getSelectionRange(area); - if (range.start < this._cursorHead) { - this._deactivate(); - return; - } - // Deactivate if the user moves the cursor to the right of the assist // range. For example, they might click later in the document. If the user // is pressing the "right" arrow key, they are not allowed to move the From 86673486c07aa1abdfe3557a89da67044b766157 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 30 Mar 2017 12:11:20 -0700 Subject: [PATCH 24/39] Move Phortune Contollers into folders Summary: Move individual controller files into cooresponding folders. Makes it easier to locate sections and expand without clutter. Also made "chargelist" part of account since it's tied to having an account specifically. Test Plan: Vist charges, merchants, subscription, accounts, and other pages. No errors from file move. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D17587 --- src/__phutil_library_map__.php | 58 +++++++++---------- .../PhabricatorPhortuneApplication.php | 2 +- .../PhortuneAccountChargeListController.php} | 2 +- .../PhortuneAccountEditController.php | 0 .../PhortuneAccountListController.php | 0 .../PhortuneAccountViewController.php | 0 .../PhortuneCartAcceptController.php | 0 .../PhortuneCartCancelController.php | 0 .../PhortuneCartCheckoutController.php | 0 .../{ => cart}/PhortuneCartController.php | 0 .../{ => cart}/PhortuneCartListController.php | 0 .../PhortuneCartUpdateController.php | 0 .../{ => cart}/PhortuneCartViewController.php | 0 .../PhortuneMerchantController.php | 0 .../PhortuneMerchantEditController.php | 0 ...hortuneMerchantInvoiceCreateController.php | 0 .../PhortuneMerchantListController.php | 0 .../PhortuneMerchantPictureController.php | 0 .../PhortuneMerchantViewController.php | 0 .../PhortunePaymentMethodCreateController.php | 0 ...PhortunePaymentMethodDisableController.php | 0 .../PhortunePaymentMethodEditController.php | 0 .../PhortuneProductListController.php | 0 .../PhortuneProductViewController.php | 0 .../PhortuneProviderActionController.php | 0 .../PhortuneProviderDisableController.php | 0 .../PhortuneProviderEditController.php | 0 .../PhortuneSubscriptionEditController.php | 0 .../PhortuneSubscriptionListController.php | 0 .../PhortuneSubscriptionViewController.php | 0 30 files changed, 31 insertions(+), 31 deletions(-) rename src/applications/phortune/controller/{PhortuneChargeListController.php => account/PhortuneAccountChargeListController.php} (97%) rename src/applications/phortune/controller/{ => account}/PhortuneAccountEditController.php (100%) rename src/applications/phortune/controller/{ => account}/PhortuneAccountListController.php (100%) rename src/applications/phortune/controller/{ => account}/PhortuneAccountViewController.php (100%) rename src/applications/phortune/controller/{ => cart}/PhortuneCartAcceptController.php (100%) rename src/applications/phortune/controller/{ => cart}/PhortuneCartCancelController.php (100%) rename src/applications/phortune/controller/{ => cart}/PhortuneCartCheckoutController.php (100%) rename src/applications/phortune/controller/{ => cart}/PhortuneCartController.php (100%) rename src/applications/phortune/controller/{ => cart}/PhortuneCartListController.php (100%) rename src/applications/phortune/controller/{ => cart}/PhortuneCartUpdateController.php (100%) rename src/applications/phortune/controller/{ => cart}/PhortuneCartViewController.php (100%) rename src/applications/phortune/controller/{ => merchant}/PhortuneMerchantController.php (100%) rename src/applications/phortune/controller/{ => merchant}/PhortuneMerchantEditController.php (100%) rename src/applications/phortune/controller/{ => merchant}/PhortuneMerchantInvoiceCreateController.php (100%) rename src/applications/phortune/controller/{ => merchant}/PhortuneMerchantListController.php (100%) rename src/applications/phortune/controller/{ => merchant}/PhortuneMerchantPictureController.php (100%) rename src/applications/phortune/controller/{ => merchant}/PhortuneMerchantViewController.php (100%) rename src/applications/phortune/controller/{ => payment}/PhortunePaymentMethodCreateController.php (100%) rename src/applications/phortune/controller/{ => payment}/PhortunePaymentMethodDisableController.php (100%) rename src/applications/phortune/controller/{ => payment}/PhortunePaymentMethodEditController.php (100%) rename src/applications/phortune/controller/{ => product}/PhortuneProductListController.php (100%) rename src/applications/phortune/controller/{ => product}/PhortuneProductViewController.php (100%) rename src/applications/phortune/controller/{ => provider}/PhortuneProviderActionController.php (100%) rename src/applications/phortune/controller/{ => provider}/PhortuneProviderDisableController.php (100%) rename src/applications/phortune/controller/{ => provider}/PhortuneProviderEditController.php (100%) rename src/applications/phortune/controller/{ => subscription}/PhortuneSubscriptionEditController.php (100%) rename src/applications/phortune/controller/{ => subscription}/PhortuneSubscriptionListController.php (100%) rename src/applications/phortune/controller/{ => subscription}/PhortuneSubscriptionViewController.php (100%) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f9d013f497..563b15e389 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -4311,35 +4311,35 @@ phutil_register_library_map(array( 'PholioTransactionView' => 'applications/pholio/view/PholioTransactionView.php', 'PholioUploadedImageView' => 'applications/pholio/view/PholioUploadedImageView.php', 'PhortuneAccount' => 'applications/phortune/storage/PhortuneAccount.php', - 'PhortuneAccountEditController' => 'applications/phortune/controller/PhortuneAccountEditController.php', + 'PhortuneAccountChargeListController' => 'applications/phortune/controller/account/PhortuneAccountChargeListController.php', + 'PhortuneAccountEditController' => 'applications/phortune/controller/account/PhortuneAccountEditController.php', 'PhortuneAccountEditor' => 'applications/phortune/editor/PhortuneAccountEditor.php', 'PhortuneAccountHasMemberEdgeType' => 'applications/phortune/edge/PhortuneAccountHasMemberEdgeType.php', - 'PhortuneAccountListController' => 'applications/phortune/controller/PhortuneAccountListController.php', + 'PhortuneAccountListController' => 'applications/phortune/controller/account/PhortuneAccountListController.php', 'PhortuneAccountPHIDType' => 'applications/phortune/phid/PhortuneAccountPHIDType.php', 'PhortuneAccountQuery' => 'applications/phortune/query/PhortuneAccountQuery.php', 'PhortuneAccountTransaction' => 'applications/phortune/storage/PhortuneAccountTransaction.php', 'PhortuneAccountTransactionQuery' => 'applications/phortune/query/PhortuneAccountTransactionQuery.php', - 'PhortuneAccountViewController' => 'applications/phortune/controller/PhortuneAccountViewController.php', + 'PhortuneAccountViewController' => 'applications/phortune/controller/account/PhortuneAccountViewController.php', 'PhortuneAdHocCart' => 'applications/phortune/cart/PhortuneAdHocCart.php', 'PhortuneAdHocProduct' => 'applications/phortune/product/PhortuneAdHocProduct.php', 'PhortuneCart' => 'applications/phortune/storage/PhortuneCart.php', - 'PhortuneCartAcceptController' => 'applications/phortune/controller/PhortuneCartAcceptController.php', - 'PhortuneCartCancelController' => 'applications/phortune/controller/PhortuneCartCancelController.php', - 'PhortuneCartCheckoutController' => 'applications/phortune/controller/PhortuneCartCheckoutController.php', - 'PhortuneCartController' => 'applications/phortune/controller/PhortuneCartController.php', + 'PhortuneCartAcceptController' => 'applications/phortune/controller/cart/PhortuneCartAcceptController.php', + 'PhortuneCartCancelController' => 'applications/phortune/controller/cart/PhortuneCartCancelController.php', + 'PhortuneCartCheckoutController' => 'applications/phortune/controller/cart/PhortuneCartCheckoutController.php', + 'PhortuneCartController' => 'applications/phortune/controller/cart/PhortuneCartController.php', 'PhortuneCartEditor' => 'applications/phortune/editor/PhortuneCartEditor.php', 'PhortuneCartImplementation' => 'applications/phortune/cart/PhortuneCartImplementation.php', - 'PhortuneCartListController' => 'applications/phortune/controller/PhortuneCartListController.php', + 'PhortuneCartListController' => 'applications/phortune/controller/cart/PhortuneCartListController.php', 'PhortuneCartPHIDType' => 'applications/phortune/phid/PhortuneCartPHIDType.php', 'PhortuneCartQuery' => 'applications/phortune/query/PhortuneCartQuery.php', 'PhortuneCartReplyHandler' => 'applications/phortune/mail/PhortuneCartReplyHandler.php', 'PhortuneCartSearchEngine' => 'applications/phortune/query/PhortuneCartSearchEngine.php', 'PhortuneCartTransaction' => 'applications/phortune/storage/PhortuneCartTransaction.php', 'PhortuneCartTransactionQuery' => 'applications/phortune/query/PhortuneCartTransactionQuery.php', - 'PhortuneCartUpdateController' => 'applications/phortune/controller/PhortuneCartUpdateController.php', - 'PhortuneCartViewController' => 'applications/phortune/controller/PhortuneCartViewController.php', + 'PhortuneCartUpdateController' => 'applications/phortune/controller/cart/PhortuneCartUpdateController.php', + 'PhortuneCartViewController' => 'applications/phortune/controller/cart/PhortuneCartViewController.php', 'PhortuneCharge' => 'applications/phortune/storage/PhortuneCharge.php', - 'PhortuneChargeListController' => 'applications/phortune/controller/PhortuneChargeListController.php', 'PhortuneChargePHIDType' => 'applications/phortune/phid/PhortuneChargePHIDType.php', 'PhortuneChargeQuery' => 'applications/phortune/query/PhortuneChargeQuery.php', 'PhortuneChargeSearchEngine' => 'applications/phortune/query/PhortuneChargeSearchEngine.php', @@ -4358,27 +4358,27 @@ phutil_register_library_map(array( 'PhortuneMemberHasMerchantEdgeType' => 'applications/phortune/edge/PhortuneMemberHasMerchantEdgeType.php', 'PhortuneMerchant' => 'applications/phortune/storage/PhortuneMerchant.php', 'PhortuneMerchantCapability' => 'applications/phortune/capability/PhortuneMerchantCapability.php', - 'PhortuneMerchantController' => 'applications/phortune/controller/PhortuneMerchantController.php', - 'PhortuneMerchantEditController' => 'applications/phortune/controller/PhortuneMerchantEditController.php', + 'PhortuneMerchantController' => 'applications/phortune/controller/merchant/PhortuneMerchantController.php', + 'PhortuneMerchantEditController' => 'applications/phortune/controller/merchant/PhortuneMerchantEditController.php', 'PhortuneMerchantEditEngine' => 'applications/phortune/editor/PhortuneMerchantEditEngine.php', 'PhortuneMerchantEditor' => 'applications/phortune/editor/PhortuneMerchantEditor.php', 'PhortuneMerchantHasMemberEdgeType' => 'applications/phortune/edge/PhortuneMerchantHasMemberEdgeType.php', - 'PhortuneMerchantInvoiceCreateController' => 'applications/phortune/controller/PhortuneMerchantInvoiceCreateController.php', - 'PhortuneMerchantListController' => 'applications/phortune/controller/PhortuneMerchantListController.php', + 'PhortuneMerchantInvoiceCreateController' => 'applications/phortune/controller/merchant/PhortuneMerchantInvoiceCreateController.php', + 'PhortuneMerchantListController' => 'applications/phortune/controller/merchant/PhortuneMerchantListController.php', 'PhortuneMerchantPHIDType' => 'applications/phortune/phid/PhortuneMerchantPHIDType.php', - 'PhortuneMerchantPictureController' => 'applications/phortune/controller/PhortuneMerchantPictureController.php', + 'PhortuneMerchantPictureController' => 'applications/phortune/controller/merchant/PhortuneMerchantPictureController.php', 'PhortuneMerchantQuery' => 'applications/phortune/query/PhortuneMerchantQuery.php', 'PhortuneMerchantSearchEngine' => 'applications/phortune/query/PhortuneMerchantSearchEngine.php', 'PhortuneMerchantTransaction' => 'applications/phortune/storage/PhortuneMerchantTransaction.php', 'PhortuneMerchantTransactionQuery' => 'applications/phortune/query/PhortuneMerchantTransactionQuery.php', - 'PhortuneMerchantViewController' => 'applications/phortune/controller/PhortuneMerchantViewController.php', + 'PhortuneMerchantViewController' => 'applications/phortune/controller/merchant/PhortuneMerchantViewController.php', 'PhortuneMonthYearExpiryControl' => 'applications/phortune/control/PhortuneMonthYearExpiryControl.php', 'PhortuneOrderTableView' => 'applications/phortune/view/PhortuneOrderTableView.php', 'PhortunePayPalPaymentProvider' => 'applications/phortune/provider/PhortunePayPalPaymentProvider.php', 'PhortunePaymentMethod' => 'applications/phortune/storage/PhortunePaymentMethod.php', - 'PhortunePaymentMethodCreateController' => 'applications/phortune/controller/PhortunePaymentMethodCreateController.php', - 'PhortunePaymentMethodDisableController' => 'applications/phortune/controller/PhortunePaymentMethodDisableController.php', - 'PhortunePaymentMethodEditController' => 'applications/phortune/controller/PhortunePaymentMethodEditController.php', + 'PhortunePaymentMethodCreateController' => 'applications/phortune/controller/payment/PhortunePaymentMethodCreateController.php', + 'PhortunePaymentMethodDisableController' => 'applications/phortune/controller/payment/PhortunePaymentMethodDisableController.php', + 'PhortunePaymentMethodEditController' => 'applications/phortune/controller/payment/PhortunePaymentMethodEditController.php', 'PhortunePaymentMethodPHIDType' => 'applications/phortune/phid/PhortunePaymentMethodPHIDType.php', 'PhortunePaymentMethodQuery' => 'applications/phortune/query/PhortunePaymentMethodQuery.php', 'PhortunePaymentProvider' => 'applications/phortune/provider/PhortunePaymentProvider.php', @@ -4391,13 +4391,13 @@ phutil_register_library_map(array( 'PhortunePaymentProviderTestCase' => 'applications/phortune/provider/__tests__/PhortunePaymentProviderTestCase.php', 'PhortuneProduct' => 'applications/phortune/storage/PhortuneProduct.php', 'PhortuneProductImplementation' => 'applications/phortune/product/PhortuneProductImplementation.php', - 'PhortuneProductListController' => 'applications/phortune/controller/PhortuneProductListController.php', + 'PhortuneProductListController' => 'applications/phortune/controller/product/PhortuneProductListController.php', 'PhortuneProductPHIDType' => 'applications/phortune/phid/PhortuneProductPHIDType.php', 'PhortuneProductQuery' => 'applications/phortune/query/PhortuneProductQuery.php', - 'PhortuneProductViewController' => 'applications/phortune/controller/PhortuneProductViewController.php', - 'PhortuneProviderActionController' => 'applications/phortune/controller/PhortuneProviderActionController.php', - 'PhortuneProviderDisableController' => 'applications/phortune/controller/PhortuneProviderDisableController.php', - 'PhortuneProviderEditController' => 'applications/phortune/controller/PhortuneProviderEditController.php', + 'PhortuneProductViewController' => 'applications/phortune/controller/product/PhortuneProductViewController.php', + 'PhortuneProviderActionController' => 'applications/phortune/controller/provider/PhortuneProviderActionController.php', + 'PhortuneProviderDisableController' => 'applications/phortune/controller/provider/PhortuneProviderDisableController.php', + 'PhortuneProviderEditController' => 'applications/phortune/controller/provider/PhortuneProviderEditController.php', 'PhortunePurchase' => 'applications/phortune/storage/PhortunePurchase.php', 'PhortunePurchasePHIDType' => 'applications/phortune/phid/PhortunePurchasePHIDType.php', 'PhortunePurchaseQuery' => 'applications/phortune/query/PhortunePurchaseQuery.php', @@ -4405,15 +4405,15 @@ phutil_register_library_map(array( 'PhortuneStripePaymentProvider' => 'applications/phortune/provider/PhortuneStripePaymentProvider.php', 'PhortuneSubscription' => 'applications/phortune/storage/PhortuneSubscription.php', 'PhortuneSubscriptionCart' => 'applications/phortune/cart/PhortuneSubscriptionCart.php', - 'PhortuneSubscriptionEditController' => 'applications/phortune/controller/PhortuneSubscriptionEditController.php', + 'PhortuneSubscriptionEditController' => 'applications/phortune/controller/subscription/PhortuneSubscriptionEditController.php', 'PhortuneSubscriptionImplementation' => 'applications/phortune/subscription/PhortuneSubscriptionImplementation.php', - 'PhortuneSubscriptionListController' => 'applications/phortune/controller/PhortuneSubscriptionListController.php', + 'PhortuneSubscriptionListController' => 'applications/phortune/controller/subscription/PhortuneSubscriptionListController.php', 'PhortuneSubscriptionPHIDType' => 'applications/phortune/phid/PhortuneSubscriptionPHIDType.php', 'PhortuneSubscriptionProduct' => 'applications/phortune/product/PhortuneSubscriptionProduct.php', 'PhortuneSubscriptionQuery' => 'applications/phortune/query/PhortuneSubscriptionQuery.php', 'PhortuneSubscriptionSearchEngine' => 'applications/phortune/query/PhortuneSubscriptionSearchEngine.php', 'PhortuneSubscriptionTableView' => 'applications/phortune/view/PhortuneSubscriptionTableView.php', - 'PhortuneSubscriptionViewController' => 'applications/phortune/controller/PhortuneSubscriptionViewController.php', + 'PhortuneSubscriptionViewController' => 'applications/phortune/controller/subscription/PhortuneSubscriptionViewController.php', 'PhortuneSubscriptionWorker' => 'applications/phortune/worker/PhortuneSubscriptionWorker.php', 'PhortuneTestPaymentProvider' => 'applications/phortune/provider/PhortuneTestPaymentProvider.php', 'PhortuneWePayPaymentProvider' => 'applications/phortune/provider/PhortuneWePayPaymentProvider.php', @@ -9742,6 +9742,7 @@ phutil_register_library_map(array( 'PhabricatorApplicationTransactionInterface', 'PhabricatorPolicyInterface', ), + 'PhortuneAccountChargeListController' => 'PhortuneController', 'PhortuneAccountEditController' => 'PhortuneController', 'PhortuneAccountEditor' => 'PhabricatorApplicationTransactionEditor', 'PhortuneAccountHasMemberEdgeType' => 'PhabricatorEdgeType', @@ -9777,7 +9778,6 @@ phutil_register_library_map(array( 'PhortuneDAO', 'PhabricatorPolicyInterface', ), - 'PhortuneChargeListController' => 'PhortuneController', 'PhortuneChargePHIDType' => 'PhabricatorPHIDType', 'PhortuneChargeQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhortuneChargeSearchEngine' => 'PhabricatorApplicationSearchEngine', diff --git a/src/applications/phortune/application/PhabricatorPhortuneApplication.php b/src/applications/phortune/application/PhabricatorPhortuneApplication.php index c87914b79f..15c312773b 100644 --- a/src/applications/phortune/application/PhabricatorPhortuneApplication.php +++ b/src/applications/phortune/application/PhabricatorPhortuneApplication.php @@ -52,7 +52,7 @@ final class PhabricatorPhortuneApplication extends PhabricatorApplication { => 'PhortuneCartListController', ), 'charge/(?:query/(?P[^/]+)/)?' - => 'PhortuneChargeListController', + => 'PhortuneAccountChargeListController', ), 'card/(?P\d+)/' => array( 'edit/' => 'PhortunePaymentMethodEditController', diff --git a/src/applications/phortune/controller/PhortuneChargeListController.php b/src/applications/phortune/controller/account/PhortuneAccountChargeListController.php similarity index 97% rename from src/applications/phortune/controller/PhortuneChargeListController.php rename to src/applications/phortune/controller/account/PhortuneAccountChargeListController.php index b8edb92507..ed3f901675 100644 --- a/src/applications/phortune/controller/PhortuneChargeListController.php +++ b/src/applications/phortune/controller/account/PhortuneAccountChargeListController.php @@ -1,6 +1,6 @@ Date: Thu, 30 Mar 2017 11:17:28 -0700 Subject: [PATCH 25/39] Update PhortuneLanding page UI Summary: Minor, uses 'user-circle' for account, and merchant logo for merchants in lists. Test Plan: View the landing page, see updated logos and icons. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Differential Revision: https://secure.phabricator.com/D17586 --- .../controller/account/PhortuneAccountListController.php | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/src/applications/phortune/controller/account/PhortuneAccountListController.php b/src/applications/phortune/controller/account/PhortuneAccountListController.php index d95fd709d5..177f84d75d 100644 --- a/src/applications/phortune/controller/account/PhortuneAccountListController.php +++ b/src/applications/phortune/controller/account/PhortuneAccountListController.php @@ -18,6 +18,7 @@ final class PhortuneAccountListController extends PhortuneController { $merchants = id(new PhortuneMerchantQuery()) ->setViewer($viewer) ->withMemberPHIDs(array($viewer->getPHID())) + ->needProfileImage(true) ->execute(); $title = pht('Accounts'); @@ -39,7 +40,7 @@ final class PhortuneAccountListController extends PhortuneController { ->setHeader($account->getName()) ->setHref($this->getApplicationURI($account->getID().'/')) ->setObject($account) - ->setImageIcon('fa-credit-card'); + ->setImageIcon('fa-user-circle'); $payment_list->addItem($item); } @@ -71,7 +72,7 @@ final class PhortuneAccountListController extends PhortuneController { ->setHeader($merchant->getName()) ->setHref($this->getApplicationURI('/merchant/'.$merchant->getID().'/')) ->setObject($merchant) - ->setImageIcon('fa-bank'); + ->setImageURI($merchant->getProfileImageURI()); $merchant_list->addItem($item); } From 7ab4e7dbced90f74dfa5a18b04def800a01bf986 Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 30 Mar 2017 14:59:40 -0700 Subject: [PATCH 26/39] Allow Owner Packages to be in a Dashboard Panel Summary: Ref T12324. Add back Owners. Test Plan: read carefully Reviewers: epriestley, eadler Reviewed By: eadler Subscribers: Korvin Maniphest Tasks: T12324 Differential Revision: https://secure.phabricator.com/D17588 --- .../owners/query/PhabricatorOwnersPackageSearchEngine.php | 4 ---- 1 file changed, 4 deletions(-) diff --git a/src/applications/owners/query/PhabricatorOwnersPackageSearchEngine.php b/src/applications/owners/query/PhabricatorOwnersPackageSearchEngine.php index d6001419b4..728c3f42a8 100644 --- a/src/applications/owners/query/PhabricatorOwnersPackageSearchEngine.php +++ b/src/applications/owners/query/PhabricatorOwnersPackageSearchEngine.php @@ -15,10 +15,6 @@ final class PhabricatorOwnersPackageSearchEngine return new PhabricatorOwnersPackageQuery(); } - public function canUseInPanelContext() { - return false; - } - protected function buildCustomSearchFields() { return array( id(new PhabricatorSearchDatasourceField()) From 4d29d8e2b7712f1ca462913c794edf789a605d6a Mon Sep 17 00:00:00 2001 From: Chad Little Date: Thu, 30 Mar 2017 22:15:45 -0700 Subject: [PATCH 27/39] Fix filetree drag nav CSS Summary: Fixes T11630. Not sure what the max-width fixes, but I don't see anything off on various mobile, desktop. Test Plan: Enable filetree in differential, drag navigation all over, see normal width calculations. Reviewers: epriestley Reviewed By: epriestley Subscribers: Korvin Maniphest Tasks: T11630 Differential Revision: https://secure.phabricator.com/D17591 --- resources/celerity/map.php | 6 +++--- webroot/rsrc/css/aphront/phabricator-nav-view.css | 5 +++++ 2 files changed, 8 insertions(+), 3 deletions(-) diff --git a/resources/celerity/map.php b/resources/celerity/map.php index 6bee132238..f45c16ec69 100644 --- a/resources/celerity/map.php +++ b/resources/celerity/map.php @@ -9,7 +9,7 @@ return array( 'names' => array( 'conpherence.pkg.css' => '82aca405', 'conpherence.pkg.js' => '6249a1cf', - 'core.pkg.css' => '87c434ee', + 'core.pkg.css' => '1bf8fa70', 'core.pkg.js' => '021685f1', 'darkconsole.pkg.js' => 'e7393ebb', 'differential.pkg.css' => '90b30783', @@ -26,7 +26,7 @@ return array( 'rsrc/css/aphront/multi-column.css' => '84cc6640', 'rsrc/css/aphront/notification.css' => '3f6c89c9', 'rsrc/css/aphront/panel-view.css' => '8427b78d', - 'rsrc/css/aphront/phabricator-nav-view.css' => 'e58a4a30', + 'rsrc/css/aphront/phabricator-nav-view.css' => 'faf6a6fc', 'rsrc/css/aphront/table-view.css' => '6ca8e057', 'rsrc/css/aphront/tokenizer.css' => '9a8cb501', 'rsrc/css/aphront/tooltip.css' => '173b9431', @@ -786,7 +786,7 @@ return array( 'phabricator-keyboard-shortcut' => '1ae869f2', 'phabricator-keyboard-shortcut-manager' => '4a021c10', 'phabricator-main-menu-view' => '5294060f', - 'phabricator-nav-view-css' => 'e58a4a30', + 'phabricator-nav-view-css' => 'faf6a6fc', 'phabricator-notification' => 'ccf1cbf8', 'phabricator-notification-css' => '3f6c89c9', 'phabricator-notification-menu-css' => '6a697e43', diff --git a/webroot/rsrc/css/aphront/phabricator-nav-view.css b/webroot/rsrc/css/aphront/phabricator-nav-view.css index a9c32f2e00..e8081a55e6 100644 --- a/webroot/rsrc/css/aphront/phabricator-nav-view.css +++ b/webroot/rsrc/css/aphront/phabricator-nav-view.css @@ -69,6 +69,11 @@ margin-left: 212px; } +.device-desktop .phabricator-standard-page-body .has-drag-nav + .phabricator-nav-local { + max-width: none; +} + .has-drag-nav ul.phui-list-view { height: 100%; overflow-y: auto; From 1c5503cb292f1b25ed96956fd98ad774be4d466e Mon Sep 17 00:00:00 2001 From: Daniel Stone Date: Sun, 2 Apr 2017 15:26:26 +0000 Subject: [PATCH 28/39] Custom fields: Render 'required' for tokenizer fields Summary: When building a tokenizer-based edit control for a custom field (e.g. a datasource type), preserve a field validation error whilst building edit controls. Test Plan: - Create custom datasource field, set it to required - Observe that 'Required' does not appear next to control - Apply patch - Observe 'Required' appears next to control Reviewers: #blessed_reviewers, epriestley Reviewed By: #blessed_reviewers, epriestley Subscribers: epriestley Differential Revision: https://secure.phabricator.com/D17592 --- .../standard/PhabricatorStandardCustomFieldTokenizer.php | 1 + 1 file changed, 1 insertion(+) diff --git a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldTokenizer.php b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldTokenizer.php index d0e4e8d6ee..d2b063ffde 100644 --- a/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldTokenizer.php +++ b/src/infrastructure/customfield/standard/PhabricatorStandardCustomFieldTokenizer.php @@ -14,6 +14,7 @@ abstract class PhabricatorStandardCustomFieldTokenizer ->setName($this->getFieldKey()) ->setDatasource($this->getDatasource()) ->setCaption($this->getCaption()) + ->setError($this->getFieldError()) ->setValue(nonempty($value, array())); $limit = $this->getFieldConfigValue('limit'); From 515cb98819ae0cafeaf664c25e9d150007d5c423 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 2 Apr 2017 08:46:37 -0700 Subject: [PATCH 29/39] When running unit tests, ignore any custom task fields Summary: If you have `maniphest.custom-field-definitions` set to include "required" fields, a bunch of tests which create tasks can fail. To avoid this, reset this config while running tests. This mechanism should probably be more general (e.g., reset all config by default, only whitelist some config) but just fix this for now since it's a one-liner and doesn't make eventual cleanup any harder. Test Plan: Ran `arc unit`, hitting tests that create tasks. Reviewers: chad, 20after4 Reviewed By: chad Differential Revision: https://secure.phabricator.com/D17595 --- src/infrastructure/testing/PhabricatorTestCase.php | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/infrastructure/testing/PhabricatorTestCase.php b/src/infrastructure/testing/PhabricatorTestCase.php index c9790cd1e4..c4881ab846 100644 --- a/src/infrastructure/testing/PhabricatorTestCase.php +++ b/src/infrastructure/testing/PhabricatorTestCase.php @@ -128,6 +128,10 @@ abstract class PhabricatorTestCase extends PhutilTestCase { $this->env->overrideEnvConfig('phabricator.silent', false); $this->env->overrideEnvConfig('cluster.read-only', false); + + $this->env->overrideEnvConfig( + 'maniphest.custom-field-definitions', + array()); } protected function didRunTests() { From 64234535e3c96e7c60ce86815b25b592e7ab8a41 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 2 Apr 2017 08:43:02 -0700 Subject: [PATCH 30/39] Remove FIELD_KEYWORDS, index project slugs as body content Summary: D17384 added a "keywords" field but only partially implemented it. - Remove this field. - Index project slugs as part of the document body instead. Test Plan: - Ran `bin/search index PHID-PROJ-... --force`. - Found project by searching for a unique slug. Reviewers: chad, 20after4 Reviewed By: chad Differential Revision: https://secure.phabricator.com/D17596 --- .../PhabricatorProjectFulltextEngine.php | 25 +++++++++++++------ .../PhabricatorSearchDocumentFieldType.php | 1 - 2 files changed, 17 insertions(+), 9 deletions(-) diff --git a/src/applications/project/search/PhabricatorProjectFulltextEngine.php b/src/applications/project/search/PhabricatorProjectFulltextEngine.php index 14314c3436..ecec952990 100644 --- a/src/applications/project/search/PhabricatorProjectFulltextEngine.php +++ b/src/applications/project/search/PhabricatorProjectFulltextEngine.php @@ -8,17 +8,26 @@ final class PhabricatorProjectFulltextEngine $object) { $project = $object; + $viewer = $this->getViewer(); + + // Reload the project to get slugs. + $project = id(new PhabricatorProjectQuery()) + ->withIDs(array($project->getID())) + ->setViewer($viewer) + ->needSlugs(true) + ->executeOne(); + $project->updateDatasourceTokens(); - $document->setDocumentTitle($project->getDisplayName()); - $document->addField(PhabricatorSearchDocumentFieldType::FIELD_KEYWORDS, - $project->getPrimarySlug()); - try { - $slugs = $project->getSlugs(); - foreach ($slugs as $slug) {} - } catch (PhabricatorDataNotAttachedException $e) { - // ignore + $slugs = array(); + foreach ($project->getSlugs() as $slug) { + $slugs[] = $slug->getSlug(); } + $body = implode("\n", $slugs); + + $document + ->setDocumentTitle($project->getDisplayName()) + ->addField(PhabricatorSearchDocumentFieldType::FIELD_BODY, $body); $document->addRelationship( $project->isArchived() diff --git a/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php b/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php index 12c90f8469..10dbf0ca65 100644 --- a/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php +++ b/src/applications/search/constants/PhabricatorSearchDocumentFieldType.php @@ -5,6 +5,5 @@ final class PhabricatorSearchDocumentFieldType extends Phobject { const FIELD_TITLE = 'titl'; const FIELD_BODY = 'body'; const FIELD_COMMENT = 'cmnt'; - const FIELD_KEYWORDS = 'kwrd'; } From 287e708c4d3ecdec3af77f5c409d0aa9f118ef94 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 2 Apr 2017 12:55:38 -0700 Subject: [PATCH 31/39] Adjust and wordsmith Search documentation Summary: Ref T12450. General adjustments: - Try to make "Cluster: Search" more about "stuff in common + types" instead of pretty much all being Elastic-specific, so we can add Solr or whatever later. - Provide guidance about rebuilding indexes after making a change. - Simplify the basic examples, then provide a more advanced example at the ed. - Really try to avoid suggesting anyone configure Elasticsearch ever for any reason. Test Plan: Read documents, previewed in remarkup. Reviewers: chad, 20after4 Reviewed By: 20after4 Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17602 --- src/docs/user/cluster/cluster.diviner | 26 +-- src/docs/user/cluster/cluster_search.diviner | 234 +++++++++++++++---- 2 files changed, 191 insertions(+), 69 deletions(-) diff --git a/src/docs/user/cluster/cluster.diviner b/src/docs/user/cluster/cluster.diviner index 15eed86eb4..30ad53efb8 100644 --- a/src/docs/user/cluster/cluster.diviner +++ b/src/docs/user/cluster/cluster.diviner @@ -47,7 +47,7 @@ will have on availability, resistance to data loss, and scalability. | **SSH Servers** | Minimal | Low | No Risk | Low | **Web Servers** | Minimal | **High** | No Risk | Moderate | **Notifications** | Minimal | Low | No Risk | Low -| **Fulltext Search** | Moderate | **High** | Minimal Risk | Moderate +| **Fulltext Search** | Minimal | Low | No Risk | Low See below for a walkthrough of these services in greater detail. @@ -241,26 +241,14 @@ For details, see @{article:Cluster: Notifications}. Cluster: Fulltext Search ======================== -At a certain scale, you may begin to bump up against the limitations of MySQL's -built-in fulltext search capabilities. We have seen this with very large -installations with several million objects in the database and very many -simultaneous requests. At this point you may consider adding Elasticsearch -hosts to your cluster to reduce the load on your MySQL hosts. +Configuring search services is relatively simple and has no pre-requisites. -Elasticsearch has the ability to spread the load across multiple hosts and can -handle very large indexes by sharding. +By default, Phabricator uses MySQL as a fulltext search engine, so deploying +multiple database hosts will effectively also deploy multiple fulltext search +hosts. -Search does not involve any risk of data lost because it's always possible to -rebuild the search index from the original database objects. This process can -be very time consuming, however, especially when the database grows very large. - -With multiple Elasticsearch hosts, you can survive the loss of a single host -with minimal disruption as Phabricator will detect the problem and direct -queries to one of the remaining hosts. - -Phabricator supports writing to multiple indexing servers. This Simplifies -Elasticsearch upgrades and makes it possible to recover more quickly from -problems with the search index. +Search indexes can be completely rebuilt from the database, so there is no +risk of data loss no matter how fulltext search is configured. For details, see @{article:Cluster: Search}. diff --git a/src/docs/user/cluster/cluster_search.diviner b/src/docs/user/cluster/cluster_search.diviner index 662abecbc3..c658f50db4 100644 --- a/src/docs/user/cluster/cluster_search.diviner +++ b/src/docs/user/cluster/cluster_search.diviner @@ -4,73 +4,207 @@ Overview ======== -You can configure phabricator to connect to one or more fulltext search clusters -running either Elasticsearch or MySQL. By default and without further -configuration, Phabricator will use MySQL for fulltext search. This will be -adequate for the vast majority of users. Installs with a very large number of -objects or specialized search needs can consider enabling Elasticsearch for -better scalability and potentially better search results. +You can configure Phabricator to connect to one or more fulltext search +services. + +By default, Phabricator will use MySQL for fulltext search. This is suitable +for most installs. However, alternate engines are supported. + Configuring Search Services =========================== -To configure an Elasticsearch service, use the `cluster.search` configuration -option. A typical Elasticsearch configuration will probably look similar to -the following example: +To configure search services, adjust the `cluster.search` configuration +option. This option contains a list of one or more fulltext search services, +like this: + +```lang=json +[ + { + "type": "...", + "hosts": [ + ... + ], + "roles": { + "read": true, + "write": true + } + } +] +``` + +When a user makes a change to a document, Phabricator writes the updated +document into every configured, writable fulltext service. + +When a user issues a query, Phabricator tries configured, readable services +in order until it is able to execute the query successfully. + +These options are supported by all service types: + +| Key | Description | +|---|---| +| `type` | Constant identifying the service type, like `mysql`. +| `roles` | Dictionary of role settings, for enabling reads and writes. +| `hosts` | List of hosts for this service. + +Some service types support additional options. + +Available Service Types +======================= + +These service types are supported: + +| Service | Key | Description | +|---|---|---| +| MySQL | `mysql` | Default MySQL fulltext index. +| Elasticsearch | `elasticsearch` | Use an external Elasticsearch service + + +Fulltext Service Roles +====================== + +These roles are supported: + +| Role | Key | Description +|---|---|---| +| Read | `read` | Allows the service to be queried when users search. +| Write | `write` | Allows documents to be published to the service. + + +Specifying Hosts +================ + +The `hosts` key should contain a list of dictionaries, each specifying the +details of a host. A service should normally have one or more hosts. + +When an option is set at the service level, it serves as a default for all +hosts. It may be overridden by changing the value for a particular host. + + +Service Type: MySQL +============== + +The `mysql` service type does not require any configuration, and does not +need to have hosts specified. This service uses the builtin database to +index and search documents. + +A typical `mysql` service configuration looks like this: ```lang=json { - "cluster.search": [ - { - "type": "elasticsearch", - "hosts": [ - { - "host": "127.0.0.1", - "roles": { "write": true, "read": true } - } - ], - "port": 9200, - "protocol": "http", - "path": "/phabricator", - "version": 5 - }, - ], + "type": "mysql" } ``` -Supported Options ------------------ -| Key | Type |Comments| -|`type` | String |Engine type. Currently, 'elasticsearch' or 'mysql'| -|`protocol`| String |Either 'http' or 'https'| -|`port`| Int |The TCP port that Elasticsearch is bound to| -|`path`| String |The path portion of the url for phabricator's index.| -|`version`| Int |The version of Elasticsearch server. Supports either 2 or 5.| -|`hosts`| List |A list of one or more Elasticsearch host names / addresses.| -Host Configuration ------------------- -Each search service must have one or more hosts associated with it. Each host -entry consists of a `host` key, a dictionary of roles and can optionally -override any of the options that are valid at the service level (see above). +Service Type: Elasticsearch +====================== -Currently supported roles are `read` and `write`. These can be individually -enabled or disabled on a per-host basis. A typical setup might include two -elasticsearch clusters in two separate datacenters. You can configure one -cluster for reads and both for writes. When one cluster is down for maintenance -you can simply swap the read role over to the backup cluster and then proceed -with maintenance without any service interruption. +The `elasticsearch` sevice type supports these options: + +| Key | Description | +|---|---| +| `protocol` | Either `"http"` (default) or `"https"`. +| `port` | Elasticsearch TCP port. +| `version` | Elasticsearch version, either `2` or `5` (default). +| `path` | Path for the index. Defaults to `/phabriator`. Advanced. + +A typical `elasticsearch` service configuration looks like this: + +```lang=json +{ + "type": "elasticsearch", + "hosts": [ + { + "protocol": "http", + "host": "127.0.0.1", + "port": 9200 + } + ] +} +``` Monitoring Search Services ========================== -You can monitor fulltext search in {nav Config > Search Servers}. This interface -shows you a quick overview of services and their health. +You can monitor fulltext search in {nav Config > Search Servers}. This +interface shows you a quick overview of services and their health. The table on this page shows some basic stats for each configured service, followed by the configuration and current status of each host. -NOTE: This page runs its diagnostics //from the web server that is serving the -request//. If you are recovering from a disaster, the view this page shows -may be partial or misleading, and two requests served by different servers may -see different views of the cluster. + +Rebuilding Indexes +================== + +After adding new search services, you will need to rebuild document indexes +on them. To do this, first initialize the services: + +``` +phabricator/ $ ./bin/search init +``` + +This will perform index setup steps and other one-time configuration. + +To populate documents in all indexes, run this command: + +``` +phabricator/ $ ./bin/search index --force --background --type all +``` + +This initiates an exhaustive rebuild of the document indexes. To get a more +detailed list of indexing options available, run: + +``` +phabricator/ $ ./bin/search help index +``` + + +Advanced Example +================ + +This is a more advanced example which shows a configuration with multiple +different services in different roles. In this example: + + - Phabricator is using an Elasticsearch 2 service as its primary fulltext + service. + - An Elasticsearch 5 service is online, but only receiving writes. + - The MySQL service is serving as a backup if Elasticsearch fails. + +This particular configuration may not be very useful. It is primarily +intended to show how to configure many different options. + + +```lang=json +[ + { + "type": "elasticsearch", + "version": 2, + "hosts": [ + { + "host": "elastic2.mycompany.com", + "port": 9200, + "protocol": "http" + } + ] + }, + { + "type": "elasticsearch", + "version": 5, + "hosts": [ + { + "host": "elastic5.mycompany.com", + "port": 9789, + "protocol": "https" + "roles": { + "read": false, + "write": true + } + } + ] + }, + { + "type": "mysql" + } +] +``` From 6d81675032661359fa340ec6a74bee0e02eed652 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 2 Apr 2017 09:59:49 -0700 Subject: [PATCH 32/39] Remove "url" from Elasticsearch index Summary: Ref T12450. This was added a very very long time ago (D2298). I don't want to put this in the upstream index anymore because I don't want to encourage third parties to develop software which reads the index directly. Reading the index directly is a big skeleton key which bypasses policy checks. This was added before much of the policy model existed, when that wasn't as much of a concern. On a tecnhnical note, this also doesn't update when `phabricator.base-uri` changes. This can be written as a search index extension if an install relies on it for some bizarre reason, although none should and I'm unaware of any actual use cases in the wild for it, even at Facebook. Test Plan: Indexed some random stuff into ElasticSearch. Reviewers: chad, 20after4 Reviewed By: chad Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17600 --- .../fulltextstorage/PhabricatorElasticFulltextStorageEngine.php | 2 -- 1 file changed, 2 deletions(-) diff --git a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php index 45011ba982..51e849f91a 100644 --- a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php +++ b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php @@ -78,10 +78,8 @@ class PhabricatorElasticFulltextStorageEngine $timestamp_key = $this->getTimestampField(); - // URL is not used internally but it can be useful externally. $spec = array( 'title' => $doc->getDocumentTitle(), - 'url' => PhabricatorEnv::getProductionURI($handle->getURI()), 'dateCreated' => $doc->getDocumentCreated(), $timestamp_key => $doc->getDocumentModified(), ); From bd939782001ed1b4cb6ed2b812bad58c75309a13 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 2 Apr 2017 09:17:29 -0700 Subject: [PATCH 33/39] Count and report skipped documents from "bin/search index" Summary: Ref T12450. There's currently a bad behavior where inserting a document into one search service marks it as up to date everywhere. This isn't nearly as obvious as it should be because `bin/search index` doesn't make it terribly clear when a document was skipped because the index version was already up to date. When running `bin/seach index` without `--force` or `--background`, keep track of updated vs not-updated documents and print out some guidance. In other configurations, try to provide more help too. Test Plan: {F4452134} Reviewers: chad, 20after4 Reviewed By: 20after4 Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17597 --- ...abricatorSearchManagementIndexWorkflow.php | 78 ++++++++++++++++++- 1 file changed, 76 insertions(+), 2 deletions(-) diff --git a/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php b/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php index a324a20637..d7667b86b6 100644 --- a/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php +++ b/src/applications/search/management/PhabricatorSearchManagementIndexWorkflow.php @@ -87,8 +87,9 @@ final class PhabricatorSearchManagementIndexWorkflow } if (!$is_background) { - $console->writeOut( - "%s\n", + echo tsprintf( + "** %s ** %s\n", + pht('NOTE'), pht( 'Run this workflow with "%s" to queue tasks for the daemon workers.', '--background')); @@ -109,9 +110,32 @@ final class PhabricatorSearchManagementIndexWorkflow ); $any_success = false; + + // If we aren't using "--background" or "--force", track how many objects + // we're skipping so we can print this information for the user and give + // them a hint that they might want to use "--force". + $track_skips = (!$is_background && !$is_force); + + $count_updated = 0; + $count_skipped = 0; + foreach ($phids as $phid) { try { + if ($track_skips) { + $old_versions = $this->loadIndexVersions($phid); + } + PhabricatorSearchWorker::queueDocumentForIndexing($phid, $parameters); + + if ($track_skips) { + $new_versions = $this->loadIndexVersions($phid); + if ($old_versions !== $new_versions) { + $count_updated++; + } else { + $count_skipped++; + } + } + $any_success = true; } catch (Exception $ex) { phlog($ex); @@ -127,6 +151,45 @@ final class PhabricatorSearchManagementIndexWorkflow pht('Failed to rebuild search index for any documents.')); } + if ($track_skips) { + if ($count_updated) { + echo tsprintf( + "** %s ** %s\n", + pht('DONE'), + pht( + 'Updated search indexes for %s document(s).', + new PhutilNumber($count_updated))); + } + + if ($count_skipped) { + echo tsprintf( + "** %s ** %s\n", + pht('SKIP'), + pht( + 'Skipped %s documents(s) which have not updated since they were '. + 'last indexed.', + new PhutilNumber($count_skipped))); + echo tsprintf( + "** %s ** %s\n", + pht('NOTE'), + pht( + 'Use "--force" to force the index to update these documents.')); + } + } else if ($is_background) { + echo tsprintf( + "** %s ** %s\n", + pht('DONE'), + pht( + 'Queued %s document(s) for background indexing.', + new PhutilNumber(count($phids)))); + } else { + echo tsprintf( + "** %s ** %s\n", + pht('DONE'), + pht( + 'Forced search index updates for %s document(s).', + new PhutilNumber(count($phids)))); + } } private function loadPHIDsByNames(array $names) { @@ -206,5 +269,16 @@ final class PhabricatorSearchManagementIndexWorkflow return $phids; } + private function loadIndexVersions($phid) { + $table = new PhabricatorSearchIndexVersion(); + $conn = $table->establishConnection('r'); + + return queryfx_all( + $conn, + 'SELECT extensionKey, version FROM %T WHERE objectPHID = %s + ORDER BY extensionKey, version', + $table->getTableName(), + $phid); + } } From 0f144d29e92059b787cfb3816a0d3988d4b1feb3 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 2 Apr 2017 09:27:19 -0700 Subject: [PATCH 34/39] When "cluster.search" changes, don't trust the old index versions Summary: Ref T12450. We track a "document version" for updating search indexes, so that if a document is rapidly updated many times in a row we can skip most of the work. However, this version doesn't consider "cluster.search" configuration, so if you add a new service (like a new ElasticSearch host) we still think that every document is up-to-date. When you run `bin/search index` to populate the index (without `--force`), we just do nothing. This isn't necessarily very obvious. D17597 makes it more clear, by printing "everything was skipped and nothing happened" at the end. Here, fix the issue by considering the content of "cluster.search" when computing fulltext document versions: if you change `cluster.search`, we throw away the version index and reindex everything. This is slightly more work than we need to do, but changes to "cluster.search" are rare and this is much easier than trying to individually track which versions of which documents are in which services, which probably isn't very useful anyway. Test Plan: - Ran `bin/search index --type project`, saw everything get skipped. - Changed `cluster.search`. - Ran `search index` again, saw everything get updated. - Ran a third time without changing `cluster.search`, everything was properly skipped. Reviewers: chad, 20after4 Reviewed By: 20after4 Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17598 --- ...habricatorFulltextIndexEngineExtension.php | 24 +++++++++++++++++++ 1 file changed, 24 insertions(+) diff --git a/src/applications/search/engineextension/PhabricatorFulltextIndexEngineExtension.php b/src/applications/search/engineextension/PhabricatorFulltextIndexEngineExtension.php index 0767849abe..ab4da88420 100644 --- a/src/applications/search/engineextension/PhabricatorFulltextIndexEngineExtension.php +++ b/src/applications/search/engineextension/PhabricatorFulltextIndexEngineExtension.php @@ -5,6 +5,8 @@ final class PhabricatorFulltextIndexEngineExtension const EXTENSIONKEY = 'fulltext'; + private $configurationVersion; + public function getExtensionName() { return pht('Fulltext Engine'); } @@ -12,6 +14,11 @@ final class PhabricatorFulltextIndexEngineExtension public function getIndexVersion($object) { $version = array(); + // When "cluster.search" is reconfigured, new indexes which don't have any + // data yet may have been added. We err on the side of caution and assume + // that every document may need to be reindexed. + $version[] = $this->getConfigurationVersion(); + if ($object instanceof PhabricatorApplicationTransactionInterface) { // If this is a normal object with transactions, we only need to // reindex it if there are new transactions (or comment edits). @@ -88,5 +95,22 @@ final class PhabricatorFulltextIndexEngineExtension return $comment_row['id']; } + private function getConfigurationVersion() { + if ($this->configurationVersion === null) { + $this->configurationVersion = $this->newConfigurationVersion(); + } + return $this->configurationVersion; + } + + private function newConfigurationVersion() { + $raw = array( + 'services' => PhabricatorEnv::getEnvConfig('cluster.search'), + ); + + $json = phutil_json_encode($raw); + + return PhabricatorHash::digestForIndex($json); + } + } From 304d19f92a7bea08573045d6951cefa4b14e7086 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 2 Apr 2017 09:53:26 -0700 Subject: [PATCH 35/39] After a fulltext write to a particular service fails, keep trying writes to other services Summary: Ref T12450. Currently, if a write fails, we stop and don't try to write to other index services. There's no technical reason not to keep trying writes, it makes some testing easier, and it would improve behavior in a scenario where engines are configured as "primary" and "backup" and the primary service is having some issues. Also, make "no writable services are configured" acceptable, rather than an error. This state is probably goofy but if we want to detect it I think it should probably be a config-validation issue, not a write-time check. I also think it's not totally unreasonable to want to just turn off all writes for a while (maybe to reduce load while you're doing a background update). Test Plan: - Configured a bad ElasticSearch engine and a good MySQL engine. - Ran `bin/search index ... --force`. - Saw MySQL get updated even though ElasticSearch failed. Reviewers: chad, 20after4 Reviewed By: 20after4 Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17599 --- .../search/PhabricatorSearchService.php | 28 +++++++++++++------ 1 file changed, 19 insertions(+), 9 deletions(-) diff --git a/src/infrastructure/cluster/search/PhabricatorSearchService.php b/src/infrastructure/cluster/search/PhabricatorSearchService.php index 10cf78d94b..a9ceb0e7e5 100644 --- a/src/infrastructure/cluster/search/PhabricatorSearchService.php +++ b/src/infrastructure/cluster/search/PhabricatorSearchService.php @@ -212,20 +212,30 @@ class PhabricatorSearchService /** * (re)index the document: attempt to pass the document to all writable * fulltext search hosts - * @throws PhabricatorClusterNoHostForRoleException */ public static function reindexAbstractDocument( - PhabricatorSearchAbstractDocument $doc) { - $indexed = 0; + PhabricatorSearchAbstractDocument $document) { + + $exceptions = array(); foreach (self::getAllServices() as $service) { - $hosts = $service->getAllHostsForRole('write'); - if (count($hosts)) { - $service->getEngine()->reindexAbstractDocument($doc); - $indexed++; + if (!$service->isWritable()) { + continue; + } + + $engine = $service->getEngine(); + try { + $engine->reindexAbstractDocument($document); + } catch (Exception $ex) { + $exceptions[] = $ex; } } - if ($indexed == 0) { - throw new PhabricatorClusterNoHostForRoleException('write'); + + if ($exceptions) { + throw new PhutilAggregateException( + pht( + 'Writes to search services failed while reindexing document "%s".', + $document->getPHID()), + $exceptions); } } From a9e2732a5cb365b7ec30fe7fbe9e9305fbbad2d7 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 2 Apr 2017 12:03:28 -0700 Subject: [PATCH 36/39] Spell "Elasticsearch" correctly, not "ElasticSearch" Summary: Ref T12450. These are like 95% my fault, but Elastic appears to spell the name "Elasticsearch" consistently in their branding. Test Plan: `grep ElasticSearch` Reviewers: chad, 20after4 Maniphest Tasks: T12450 Differential Revision: https://secure.phabricator.com/D17601 --- src/__phutil_library_map__.php | 10 +++++----- ...atorElasticsearchSetupCheck.php.lowercase} | 4 ++-- .../check/PhabricatorMySQLSetupCheck.php | 6 +++--- .../maniphest/query/ManiphestTaskQuery.php | 2 +- ...habricatorElasticFulltextStorageEngine.php | 20 +++++++------------ ...orElasticsearchQueryBuilder.php.lowercase} | 2 +- ...habricatorElasticsearchHost.php.lowercase} | 4 ++-- 7 files changed, 21 insertions(+), 27 deletions(-) rename src/applications/config/check/{PhabricatorElasticSearchSetupCheck.php => PhabricatorElasticsearchSetupCheck.php.lowercase} (95%) rename src/applications/search/fulltextstorage/{PhabricatorElasticSearchQueryBuilder.php => PhabricatorElasticsearchQueryBuilder.php.lowercase} (97%) rename src/infrastructure/cluster/search/{PhabricatorElasticSearchHost.php => PhabricatorElasticsearchHost.php.lowercase} (96%) diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 563b15e389..9ccc495e7a 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2655,9 +2655,9 @@ phutil_register_library_map(array( 'PhabricatorEditorMultipleSetting' => 'applications/settings/setting/PhabricatorEditorMultipleSetting.php', 'PhabricatorEditorSetting' => 'applications/settings/setting/PhabricatorEditorSetting.php', 'PhabricatorElasticFulltextStorageEngine' => 'applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php', - 'PhabricatorElasticSearchHost' => 'infrastructure/cluster/search/PhabricatorElasticSearchHost.php', - 'PhabricatorElasticSearchQueryBuilder' => 'applications/search/fulltextstorage/PhabricatorElasticSearchQueryBuilder.php', - 'PhabricatorElasticSearchSetupCheck' => 'applications/config/check/PhabricatorElasticSearchSetupCheck.php', + 'PhabricatorElasticsearchHost' => 'infrastructure/cluster/search/PhabricatorElasticsearchHost.php', + 'PhabricatorElasticsearchQueryBuilder' => 'applications/search/fulltextstorage/PhabricatorElasticsearchQueryBuilder.php', + 'PhabricatorElasticsearchSetupCheck' => 'applications/config/check/PhabricatorElasticsearchSetupCheck.php', 'PhabricatorEmailAddressesSettingsPanel' => 'applications/settings/panel/PhabricatorEmailAddressesSettingsPanel.php', 'PhabricatorEmailContentSource' => 'applications/metamta/contentsource/PhabricatorEmailContentSource.php', 'PhabricatorEmailDeliverySettingsPanel' => 'applications/settings/panel/PhabricatorEmailDeliverySettingsPanel.php', @@ -7750,8 +7750,8 @@ phutil_register_library_map(array( 'PhabricatorEditorMultipleSetting' => 'PhabricatorSelectSetting', 'PhabricatorEditorSetting' => 'PhabricatorStringSetting', 'PhabricatorElasticFulltextStorageEngine' => 'PhabricatorFulltextStorageEngine', - 'PhabricatorElasticSearchHost' => 'PhabricatorSearchHost', - 'PhabricatorElasticSearchSetupCheck' => 'PhabricatorSetupCheck', + 'PhabricatorElasticsearchHost' => 'PhabricatorSearchHost', + 'PhabricatorElasticsearchSetupCheck' => 'PhabricatorSetupCheck', 'PhabricatorEmailAddressesSettingsPanel' => 'PhabricatorSettingsPanel', 'PhabricatorEmailContentSource' => 'PhabricatorContentSource', 'PhabricatorEmailDeliverySettingsPanel' => 'PhabricatorEditEngineSettingsPanel', diff --git a/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php b/src/applications/config/check/PhabricatorElasticsearchSetupCheck.php.lowercase similarity index 95% rename from src/applications/config/check/PhabricatorElasticSearchSetupCheck.php rename to src/applications/config/check/PhabricatorElasticsearchSetupCheck.php.lowercase index dab886b92a..157db5e141 100644 --- a/src/applications/config/check/PhabricatorElasticSearchSetupCheck.php +++ b/src/applications/config/check/PhabricatorElasticsearchSetupCheck.php.lowercase @@ -1,6 +1,6 @@ setParameter('query', $this->fullTextSearch); // NOTE: Setting this to something larger than 10,000 will raise errors in - // ElasticSearch, and billions of results won't fit in memory anyway. + // Elasticsearch, and billions of results won't fit in memory anyway. $fulltext_query->setParameter('limit', 10000); $fulltext_query->setParameter('types', array(ManiphestTaskPHIDType::TYPECONST)); diff --git a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php index 51e849f91a..8c75c17e36 100644 --- a/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php +++ b/src/applications/search/fulltextstorage/PhabricatorElasticFulltextStorageEngine.php @@ -32,19 +32,13 @@ class PhabricatorElasticFulltextStorageEngine } public function getHostType() { - return new PhabricatorElasticSearchHost($this); + return new PhabricatorElasticsearchHost($this); } - /** - * @return PhabricatorElasticSearchHost - */ public function getHostForRead() { return $this->getService()->getAnyHostForRole('read'); } - /** - * @return PhabricatorElasticSearchHost - */ public function getHostForWrite() { return $this->getService()->getAnyHostForRole('write'); } @@ -148,7 +142,7 @@ class PhabricatorElasticFulltextStorageEngine } private function buildSpec(PhabricatorSavedQuery $query) { - $q = new PhabricatorElasticSearchQueryBuilder('bool'); + $q = new PhabricatorElasticsearchQueryBuilder('bool'); $query_string = $query->getParameter('query'); if (strlen($query_string)) { $fields = $this->getTypeConstants('PhabricatorSearchDocumentFieldType'); @@ -305,7 +299,7 @@ class PhabricatorElasticFulltextStorageEngine $exceptions); } - public function indexExists(PhabricatorElasticSearchHost $host = null) { + public function indexExists(PhabricatorElasticsearchHost $host = null) { if (!$host) { $host = $this->getHostForRead(); } @@ -439,7 +433,7 @@ class PhabricatorElasticFulltextStorageEngine return $data; } - public function indexIsSane(PhabricatorElasticSearchHost $host = null) { + public function indexIsSane(PhabricatorElasticsearchHost $host = null) { if (!$host) { $host = $this->getHostForRead(); } @@ -518,7 +512,7 @@ class PhabricatorElasticFulltextStorageEngine $this->executeRequest($host, '/', $data, 'PUT'); } - public function getIndexStats(PhabricatorElasticSearchHost $host = null) { + public function getIndexStats(PhabricatorElasticsearchHost $host = null) { if ($this->version < 2) { return false; } @@ -542,7 +536,7 @@ class PhabricatorElasticFulltextStorageEngine ); } - private function executeRequest(PhabricatorElasticSearchHost $host, $path, + private function executeRequest(PhabricatorElasticsearchHost $host, $path, array $data, $method = 'GET') { $uri = $host->getURI($path); @@ -576,7 +570,7 @@ class PhabricatorElasticFulltextStorageEngine } catch (PhutilJSONParserException $ex) { $host->didHealthCheck(false); throw new PhutilProxyException( - pht('ElasticSearch server returned invalid JSON!'), + pht('Elasticsearch server returned invalid JSON!'), $ex); } diff --git a/src/applications/search/fulltextstorage/PhabricatorElasticSearchQueryBuilder.php b/src/applications/search/fulltextstorage/PhabricatorElasticsearchQueryBuilder.php.lowercase similarity index 97% rename from src/applications/search/fulltextstorage/PhabricatorElasticSearchQueryBuilder.php rename to src/applications/search/fulltextstorage/PhabricatorElasticsearchQueryBuilder.php.lowercase index 659660d813..0f835eb726 100644 --- a/src/applications/search/fulltextstorage/PhabricatorElasticSearchQueryBuilder.php +++ b/src/applications/search/fulltextstorage/PhabricatorElasticsearchQueryBuilder.php.lowercase @@ -1,6 +1,6 @@ Date: Sun, 2 Apr 2017 14:59:36 -0700 Subject: [PATCH 37/39] Rename "ElasticSearch" filenames to "Elasticsearch" (2/2) Sometimes git does some odd magic on case-insensitive filesystems, try to trick it. Auditors: chad --- ...Check.php.lowercase => PhabricatorElasticsearchSetupCheck.php} | 0 ...der.php.lowercase => PhabricatorElasticsearchQueryBuilder.php} | 0 ...csearchHost.php.lowercase => PhabricatorElasticsearchHost.php} | 0 3 files changed, 0 insertions(+), 0 deletions(-) rename src/applications/config/check/{PhabricatorElasticsearchSetupCheck.php.lowercase => PhabricatorElasticsearchSetupCheck.php} (100%) rename src/applications/search/fulltextstorage/{PhabricatorElasticsearchQueryBuilder.php.lowercase => PhabricatorElasticsearchQueryBuilder.php} (100%) rename src/infrastructure/cluster/search/{PhabricatorElasticsearchHost.php.lowercase => PhabricatorElasticsearchHost.php} (100%) diff --git a/src/applications/config/check/PhabricatorElasticsearchSetupCheck.php.lowercase b/src/applications/config/check/PhabricatorElasticsearchSetupCheck.php similarity index 100% rename from src/applications/config/check/PhabricatorElasticsearchSetupCheck.php.lowercase rename to src/applications/config/check/PhabricatorElasticsearchSetupCheck.php diff --git a/src/applications/search/fulltextstorage/PhabricatorElasticsearchQueryBuilder.php.lowercase b/src/applications/search/fulltextstorage/PhabricatorElasticsearchQueryBuilder.php similarity index 100% rename from src/applications/search/fulltextstorage/PhabricatorElasticsearchQueryBuilder.php.lowercase rename to src/applications/search/fulltextstorage/PhabricatorElasticsearchQueryBuilder.php diff --git a/src/infrastructure/cluster/search/PhabricatorElasticsearchHost.php.lowercase b/src/infrastructure/cluster/search/PhabricatorElasticsearchHost.php similarity index 100% rename from src/infrastructure/cluster/search/PhabricatorElasticsearchHost.php.lowercase rename to src/infrastructure/cluster/search/PhabricatorElasticsearchHost.php From 009aff1a23d876689e4b85c1acfe87da1594eb8a Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 2 Apr 2017 17:12:18 -0700 Subject: [PATCH 38/39] Return task descriptions from "maniphest.search" Summary: Fixes T12461. This returns the field as a dictionary with a `"raw"` value, so we could eventually do this if we want without breaking the API: ``` { "type": "remarkup", "raw": "**raw**", "html": "raw", "text": "raw" } ``` Test Plan: Called `maniphest.search`, reviewed output. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12461 Differential Revision: https://secure.phabricator.com/D17603 --- src/applications/maniphest/storage/ManiphestTask.php | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/src/applications/maniphest/storage/ManiphestTask.php b/src/applications/maniphest/storage/ManiphestTask.php index f03bc277ac..ebcdef691d 100644 --- a/src/applications/maniphest/storage/ManiphestTask.php +++ b/src/applications/maniphest/storage/ManiphestTask.php @@ -473,6 +473,10 @@ final class ManiphestTask extends ManiphestDAO ->setKey('title') ->setType('string') ->setDescription(pht('The title of the task.')), + id(new PhabricatorConduitSearchFieldSpecification()) + ->setKey('description') + ->setType('remarkup') + ->setDescription(pht('The task description.')), id(new PhabricatorConduitSearchFieldSpecification()) ->setKey('authorPHID') ->setType('phid') @@ -501,7 +505,6 @@ final class ManiphestTask extends ManiphestDAO } public function getFieldValuesForConduit() { - $status_value = $this->getStatus(); $status_info = array( 'value' => $status_value, @@ -519,6 +522,9 @@ final class ManiphestTask extends ManiphestDAO return array( 'name' => $this->getTitle(), + 'description' => array( + 'raw' => $this->getDescription(), + ), 'authorPHID' => $this->getAuthorPHID(), 'ownerPHID' => $this->getOwnerPHID(), 'status' => $status_info, From 163e1ec4426eb40e15ad46f831a731f13b275918 Mon Sep 17 00:00:00 2001 From: epriestley Date: Sun, 2 Apr 2017 17:27:20 -0700 Subject: [PATCH 39/39] Expose the commit/task/revision relationship edges to "edge.search" Summary: Fixes T12480. Test Plan: {F4465908} Reviewers: chad Reviewed By: chad Maniphest Tasks: T12480 Differential Revision: https://secure.phabricator.com/D17604 --- .../edge/DifferentialRevisionHasCommitEdgeType.php | 13 +++++++++++++ .../edge/DifferentialRevisionHasTaskEdgeType.php | 12 ++++++++++++ .../edge/DiffusionCommitHasRevisionEdgeType.php | 13 +++++++++++++ .../edge/DiffusionCommitHasTaskEdgeType.php | 12 ++++++++++++ .../edge/ManiphestTaskHasCommitEdgeType.php | 12 ++++++++++++ .../edge/ManiphestTaskHasRevisionEdgeType.php | 12 ++++++++++++ 6 files changed, 74 insertions(+) diff --git a/src/applications/differential/edge/DifferentialRevisionHasCommitEdgeType.php b/src/applications/differential/edge/DifferentialRevisionHasCommitEdgeType.php index 8fe95bb844..819794cbd6 100644 --- a/src/applications/differential/edge/DifferentialRevisionHasCommitEdgeType.php +++ b/src/applications/differential/edge/DifferentialRevisionHasCommitEdgeType.php @@ -12,6 +12,19 @@ final class DifferentialRevisionHasCommitEdgeType extends PhabricatorEdgeType { return true; } + public function getConduitKey() { + return 'revision.commit'; + } + + public function getConduitName() { + return pht('Revision Has Commit'); + } + + public function getConduitDescription() { + return pht( + 'The source revision is associated with the destination commit.'); + } + public function getTransactionAddString( $actor, $add_count, diff --git a/src/applications/differential/edge/DifferentialRevisionHasTaskEdgeType.php b/src/applications/differential/edge/DifferentialRevisionHasTaskEdgeType.php index de9a9cad57..c4cf84c5fe 100644 --- a/src/applications/differential/edge/DifferentialRevisionHasTaskEdgeType.php +++ b/src/applications/differential/edge/DifferentialRevisionHasTaskEdgeType.php @@ -12,6 +12,18 @@ final class DifferentialRevisionHasTaskEdgeType extends PhabricatorEdgeType { return true; } + public function getConduitKey() { + return 'revision.task'; + } + + public function getConduitName() { + return pht('Revision Has Task'); + } + + public function getConduitDescription() { + return pht('The source revision is associated with the destination task.'); + } + public function getTransactionAddString( $actor, $add_count, diff --git a/src/applications/diffusion/edge/DiffusionCommitHasRevisionEdgeType.php b/src/applications/diffusion/edge/DiffusionCommitHasRevisionEdgeType.php index c0b51dd086..ce7a899bda 100644 --- a/src/applications/diffusion/edge/DiffusionCommitHasRevisionEdgeType.php +++ b/src/applications/diffusion/edge/DiffusionCommitHasRevisionEdgeType.php @@ -12,4 +12,17 @@ final class DiffusionCommitHasRevisionEdgeType extends PhabricatorEdgeType { return true; } + public function getConduitKey() { + return 'commit.revision'; + } + + public function getConduitName() { + return pht('Commit Has Revision'); + } + + public function getConduitDescription() { + return pht( + 'The source commit is associated with the destination revision.'); + } + } diff --git a/src/applications/diffusion/edge/DiffusionCommitHasTaskEdgeType.php b/src/applications/diffusion/edge/DiffusionCommitHasTaskEdgeType.php index ad769eff72..497b242650 100644 --- a/src/applications/diffusion/edge/DiffusionCommitHasTaskEdgeType.php +++ b/src/applications/diffusion/edge/DiffusionCommitHasTaskEdgeType.php @@ -12,6 +12,18 @@ final class DiffusionCommitHasTaskEdgeType extends PhabricatorEdgeType { return ManiphestTaskHasCommitEdgeType::EDGECONST; } + public function getConduitKey() { + return 'commit.task'; + } + + public function getConduitName() { + return pht('Commit Has Task'); + } + + public function getConduitDescription() { + return pht('The source commit is associated with the destination task.'); + } + public function getTransactionAddString( $actor, $add_count, diff --git a/src/applications/maniphest/edge/ManiphestTaskHasCommitEdgeType.php b/src/applications/maniphest/edge/ManiphestTaskHasCommitEdgeType.php index 4e3505163d..55515a3464 100644 --- a/src/applications/maniphest/edge/ManiphestTaskHasCommitEdgeType.php +++ b/src/applications/maniphest/edge/ManiphestTaskHasCommitEdgeType.php @@ -12,6 +12,18 @@ final class ManiphestTaskHasCommitEdgeType extends PhabricatorEdgeType { return DiffusionCommitHasTaskEdgeType::EDGECONST; } + public function getConduitKey() { + return 'task.commit'; + } + + public function getConduitName() { + return pht('Task Has Commit'); + } + + public function getConduitDescription() { + return pht('The source task is associated with the destination commit.'); + } + public function getTransactionAddString( $actor, $add_count, diff --git a/src/applications/maniphest/edge/ManiphestTaskHasRevisionEdgeType.php b/src/applications/maniphest/edge/ManiphestTaskHasRevisionEdgeType.php index 6bd1e6c94e..7e35f01627 100644 --- a/src/applications/maniphest/edge/ManiphestTaskHasRevisionEdgeType.php +++ b/src/applications/maniphest/edge/ManiphestTaskHasRevisionEdgeType.php @@ -12,6 +12,18 @@ final class ManiphestTaskHasRevisionEdgeType extends PhabricatorEdgeType { return true; } + public function getConduitKey() { + return 'task.revision'; + } + + public function getConduitName() { + return pht('Task Has Revision'); + } + + public function getConduitDescription() { + return pht('The source task is associated with the destination revision.'); + } + public function getTransactionAddString( $actor, $add_count,