From 1eab16c395e38598578683a73f5e4bce0d41a091 Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 12 Jan 2016 06:45:17 -0800 Subject: [PATCH] Move repository URIs to a dedicated index Summary: Ref T4705 (there are also some other adjacent related tasks dealing with URIs). Currently, we issue a "get repositories matching URIs: ..." query by loading every possible repository and then checking their URIs in PHP. Instead, put URIs in a separate table. I plan for each repository to potentially have multiple URIs soon, so this prepares for that. Test Plan: - Ran migrations. - Looked at index table, made sure it appeared sensible. - Ran some queries by `uri` to find repositories, found the repositories I expected. - Updated the remote URI of a repository, saw queries / index update appropriately. Reviewers: chad Reviewed By: chad Maniphest Tasks: T4705 Differential Revision: https://secure.phabricator.com/D15005 --- .../sql/autopatches/20160112.repo.01.uri.sql | 7 ++ .../20160112.repo.02.uri.index.php | 7 ++ src/__phutil_library_map__.php | 4 ++ .../diffusion/DiffusionLintSaveRunner.php | 2 +- ...sionRepositoryURIsIndexEngineExtension.php | 22 ++++++ .../RepositoryQueryConduitAPIMethod.php | 2 +- .../editor/PhabricatorRepositoryEditor.php | 4 ++ .../query/PhabricatorRepositoryQuery.php | 52 +++++++++----- .../storage/PhabricatorRepository.php | 38 +++++++---- .../storage/PhabricatorRepositoryURIIndex.php | 67 +++++++++++++++++++ 10 files changed, 175 insertions(+), 30 deletions(-) create mode 100644 resources/sql/autopatches/20160112.repo.01.uri.sql create mode 100644 resources/sql/autopatches/20160112.repo.02.uri.index.php create mode 100644 src/applications/diffusion/engineextension/DiffusionRepositoryURIsIndexEngineExtension.php create mode 100644 src/applications/repository/storage/PhabricatorRepositoryURIIndex.php diff --git a/resources/sql/autopatches/20160112.repo.01.uri.sql b/resources/sql/autopatches/20160112.repo.01.uri.sql new file mode 100644 index 0000000000..0dc925b10a --- /dev/null +++ b/resources/sql/autopatches/20160112.repo.01.uri.sql @@ -0,0 +1,7 @@ +CREATE TABLE {$NAMESPACE}_repository.repository_uriindex ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + repositoryPHID VARBINARY(64) NOT NULL, + repositoryURI LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}, + KEY `key_repository` (repositoryPHID), + KEY `key_uri` (repositoryURI(128)) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/resources/sql/autopatches/20160112.repo.02.uri.index.php b/resources/sql/autopatches/20160112.repo.02.uri.index.php new file mode 100644 index 0000000000..7a11be3179 --- /dev/null +++ b/resources/sql/autopatches/20160112.repo.02.uri.index.php @@ -0,0 +1,7 @@ +updateURIIndex(); +} diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 8a59bcf08e..d3bdb03232 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -734,6 +734,7 @@ phutil_register_library_map(array( 'DiffusionRepositorySymbolsController' => 'applications/diffusion/controller/DiffusionRepositorySymbolsController.php', 'DiffusionRepositoryTag' => 'applications/diffusion/data/DiffusionRepositoryTag.php', 'DiffusionRepositoryTestAutomationController' => 'applications/diffusion/controller/DiffusionRepositoryTestAutomationController.php', + 'DiffusionRepositoryURIsIndexEngineExtension' => 'applications/diffusion/engineextension/DiffusionRepositoryURIsIndexEngineExtension.php', 'DiffusionRequest' => 'applications/diffusion/request/DiffusionRequest.php', 'DiffusionResolveRefsConduitAPIMethod' => 'applications/diffusion/conduit/DiffusionResolveRefsConduitAPIMethod.php', 'DiffusionResolveUserQuery' => 'applications/diffusion/query/DiffusionResolveUserQuery.php', @@ -3009,6 +3010,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryTransaction' => 'applications/repository/storage/PhabricatorRepositoryTransaction.php', 'PhabricatorRepositoryTransactionQuery' => 'applications/repository/query/PhabricatorRepositoryTransactionQuery.php', 'PhabricatorRepositoryType' => 'applications/repository/constants/PhabricatorRepositoryType.php', + 'PhabricatorRepositoryURIIndex' => 'applications/repository/storage/PhabricatorRepositoryURIIndex.php', 'PhabricatorRepositoryURINormalizer' => 'applications/repository/data/PhabricatorRepositoryURINormalizer.php', 'PhabricatorRepositoryURINormalizerTestCase' => 'applications/repository/data/__tests__/PhabricatorRepositoryURINormalizerTestCase.php', 'PhabricatorRepositoryURITestCase' => 'applications/repository/storage/__tests__/PhabricatorRepositoryURITestCase.php', @@ -4707,6 +4709,7 @@ phutil_register_library_map(array( 'DiffusionRepositorySymbolsController' => 'DiffusionRepositoryEditController', 'DiffusionRepositoryTag' => 'Phobject', 'DiffusionRepositoryTestAutomationController' => 'DiffusionRepositoryEditController', + 'DiffusionRepositoryURIsIndexEngineExtension' => 'PhabricatorIndexEngineExtension', 'DiffusionRequest' => 'Phobject', 'DiffusionResolveRefsConduitAPIMethod' => 'DiffusionQueryConduitAPIMethod', 'DiffusionResolveUserQuery' => 'Phobject', @@ -7415,6 +7418,7 @@ phutil_register_library_map(array( 'PhabricatorRepositoryTransaction' => 'PhabricatorApplicationTransaction', 'PhabricatorRepositoryTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorRepositoryType' => 'Phobject', + 'PhabricatorRepositoryURIIndex' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryURINormalizer' => 'Phobject', 'PhabricatorRepositoryURINormalizerTestCase' => 'PhabricatorTestCase', 'PhabricatorRepositoryURITestCase' => 'PhabricatorTestCase', diff --git a/src/applications/diffusion/DiffusionLintSaveRunner.php b/src/applications/diffusion/DiffusionLintSaveRunner.php index e32392be40..bcfeefd510 100644 --- a/src/applications/diffusion/DiffusionLintSaveRunner.php +++ b/src/applications/diffusion/DiffusionLintSaveRunner.php @@ -71,7 +71,7 @@ final class DiffusionLintSaveRunner extends Phobject { } else if ($uuid) { $repository_query->withUUIDs(array($uuid)); } else if ($remote_uri) { - $repository_query->withRemoteURIs(array($remote_uri)); + $repository_query->withURIs(array($remote_uri)); } $repository = $repository_query->executeOne(); diff --git a/src/applications/diffusion/engineextension/DiffusionRepositoryURIsIndexEngineExtension.php b/src/applications/diffusion/engineextension/DiffusionRepositoryURIsIndexEngineExtension.php new file mode 100644 index 0000000000..2c1e49b3da --- /dev/null +++ b/src/applications/diffusion/engineextension/DiffusionRepositoryURIsIndexEngineExtension.php @@ -0,0 +1,22 @@ +updateURIIndex(); + } + +} diff --git a/src/applications/repository/conduit/RepositoryQueryConduitAPIMethod.php b/src/applications/repository/conduit/RepositoryQueryConduitAPIMethod.php index f04558b518..a3c1061e6a 100644 --- a/src/applications/repository/conduit/RepositoryQueryConduitAPIMethod.php +++ b/src/applications/repository/conduit/RepositoryQueryConduitAPIMethod.php @@ -63,7 +63,7 @@ final class RepositoryQueryConduitAPIMethod $remote_uris = $request->getValue('remoteURIs', array()); if ($remote_uris) { - $query->withRemoteURIs($remote_uris); + $query->withURIs($remote_uris); } $uuids = $request->getValue('uuids', array()); diff --git a/src/applications/repository/editor/PhabricatorRepositoryEditor.php b/src/applications/repository/editor/PhabricatorRepositoryEditor.php index fc397daa1e..54f737c232 100644 --- a/src/applications/repository/editor/PhabricatorRepositoryEditor.php +++ b/src/applications/repository/editor/PhabricatorRepositoryEditor.php @@ -518,4 +518,8 @@ final class PhabricatorRepositoryEditor throw new PhabricatorApplicationTransactionValidationException($errors); } + protected function supportsSearch() { + return true; + } + } diff --git a/src/applications/repository/query/PhabricatorRepositoryQuery.php b/src/applications/repository/query/PhabricatorRepositoryQuery.php index b36b67c222..0a5e44304f 100644 --- a/src/applications/repository/query/PhabricatorRepositoryQuery.php +++ b/src/applications/repository/query/PhabricatorRepositoryQuery.php @@ -9,7 +9,7 @@ final class PhabricatorRepositoryQuery private $types; private $uuids; private $nameContains; - private $remoteURIs; + private $uris; private $datasourceQuery; private $slugs; @@ -118,8 +118,8 @@ final class PhabricatorRepositoryQuery return $this; } - public function withRemoteURIs(array $uris) { - $this->remoteURIs = $uris; + public function withURIs(array $uris) { + $this->uris = $uris; return $this; } @@ -263,17 +263,6 @@ final class PhabricatorRepositoryQuery } } - // TODO: Denormalize this, too. - if ($this->remoteURIs) { - $try_uris = $this->getNormalizedPaths(); - $try_uris = array_fuse($try_uris); - foreach ($repositories as $key => $repository) { - if (!isset($try_uris[$repository->getNormalizedPath()])) { - unset($repositories[$key]); - } - } - } - // Build the identifierMap if ($this->numericIdentifiers) { foreach ($this->numericIdentifiers as $id) { @@ -445,6 +434,8 @@ final class PhabricatorRepositoryQuery protected function buildSelectClauseParts(AphrontDatabaseConnection $conn) { $parts = parent::buildSelectClauseParts($conn); + $parts[] = 'r.*'; + if ($this->shouldJoinSummaryTable()) { $parts[] = 's.*'; } @@ -462,9 +453,28 @@ final class PhabricatorRepositoryQuery PhabricatorRepository::TABLE_SUMMARY); } + if ($this->shouldJoinURITable()) { + $joins[] = qsprintf( + $conn, + 'LEFT JOIN %T uri ON r.phid = uri.repositoryPHID', + id(new PhabricatorRepositoryURIIndex())->getTableName()); + } + return $joins; } + protected function shouldGroupQueryResultRows() { + if ($this->shouldJoinURITable()) { + return true; + } + + return parent::shouldGroupQueryResultRows(); + } + + private function shouldJoinURITable() { + return ($this->uris !== null); + } + private function shouldJoinSummaryTable() { if ($this->needCommitCounts) { return true; @@ -592,7 +602,7 @@ final class PhabricatorRepositoryQuery if (strlen($this->nameContains)) { $where[] = qsprintf( $conn, - 'name LIKE %~', + 'r.name LIKE %~', $this->nameContains); } @@ -619,6 +629,16 @@ final class PhabricatorRepositoryQuery $this->slugs); } + if ($this->uris !== null) { + $try_uris = $this->getNormalizedPaths(); + $try_uris = array_fuse($try_uris); + + $where[] = qsprintf( + $conn, + 'uri.repositoryURI IN (%Ls)', + $try_uris); + } + return $where; } @@ -635,7 +655,7 @@ final class PhabricatorRepositoryQuery // or an `svn+ssh` URI, we could deduce how to normalize it. However, this // would be more complicated and it's not clear if it matters in practice. - foreach ($this->remoteURIs as $uri) { + foreach ($this->uris as $uri) { $normalized_uris[] = new PhabricatorRepositoryURINormalizer( PhabricatorRepositoryURINormalizer::TYPE_GIT, $uri); diff --git a/src/applications/repository/storage/PhabricatorRepository.php b/src/applications/repository/storage/PhabricatorRepository.php index 28e74ca5af..9b38f911d4 100644 --- a/src/applications/repository/storage/PhabricatorRepository.php +++ b/src/applications/repository/storage/PhabricatorRepository.php @@ -823,30 +823,40 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO return $uri; } - public function getNormalizedPath() { - $uri = (string)$this->getCloneURIObject(); + public function updateURIIndex() { + $uris = array( + (string)$this->getCloneURIObject(), + ); + foreach ($uris as $key => $uri) { + $uris[$key] = $this->getNormalizedURI($uri) + ->getNormalizedPath(); + } + + PhabricatorRepositoryURIIndex::updateRepositoryURIs( + $this->getPHID(), + $uris); + + return $this; + } + + private function getNormalizedURI($uri) { switch ($this->getVersionControlSystem()) { case PhabricatorRepositoryType::REPOSITORY_TYPE_GIT: - $normalized_uri = new PhabricatorRepositoryURINormalizer( + return new PhabricatorRepositoryURINormalizer( PhabricatorRepositoryURINormalizer::TYPE_GIT, $uri); - break; case PhabricatorRepositoryType::REPOSITORY_TYPE_SVN: - $normalized_uri = new PhabricatorRepositoryURINormalizer( + return new PhabricatorRepositoryURINormalizer( PhabricatorRepositoryURINormalizer::TYPE_SVN, $uri); - break; case PhabricatorRepositoryType::REPOSITORY_TYPE_MERCURIAL: - $normalized_uri = new PhabricatorRepositoryURINormalizer( + return new PhabricatorRepositoryURINormalizer( PhabricatorRepositoryURINormalizer::TYPE_MERCURIAL, $uri); - break; default: throw new Exception(pht('Unrecognized version control system.')); } - - return $normalized_uri->getNormalizedPath(); } public function isTracked() { @@ -2231,13 +2241,17 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO public function destroyObjectPermanently( PhabricatorDestructionEngine $engine) { + $phid = $this->getPHID(); + $this->openTransaction(); $this->delete(); + PhabricatorRepositoryURIIndex::updateRepositoryURIs($phid, array()); + $books = id(new DivinerBookQuery()) ->setViewer($engine->getViewer()) - ->withRepositoryPHIDs(array($this->getPHID())) + ->withRepositoryPHIDs(array($phid)) ->execute(); foreach ($books as $book) { $engine->destroyObject($book); @@ -2245,7 +2259,7 @@ final class PhabricatorRepository extends PhabricatorRepositoryDAO $atoms = id(new DivinerAtomQuery()) ->setViewer($engine->getViewer()) - ->withRepositoryPHIDs(array($this->getPHID())) + ->withRepositoryPHIDs(array($phid)) ->execute(); foreach ($atoms as $atom) { $engine->destroyObject($atom); diff --git a/src/applications/repository/storage/PhabricatorRepositoryURIIndex.php b/src/applications/repository/storage/PhabricatorRepositoryURIIndex.php new file mode 100644 index 0000000000..2cfa0c4471 --- /dev/null +++ b/src/applications/repository/storage/PhabricatorRepositoryURIIndex.php @@ -0,0 +1,67 @@ + false, + self::CONFIG_COLUMN_SCHEMA => array( + 'repositoryURI' => 'text', + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_repository' => array( + 'columns' => array('repositoryPHID'), + ), + 'key_uri' => array( + 'columns' => array('repositoryURI(128)'), + ), + ), + ) + parent::getConfiguration(); + } + + public static function updateRepositoryURIs( + $repository_phid, + array $uris) { + + $table = new self(); + $conn_w = $table->establishConnection('w'); + + $sql = array(); + foreach ($uris as $key => $uri) { + if (!strlen($uri)) { + unset($uris[$key]); + continue; + } + + $sql[] = qsprintf( + $conn_w, + '(%s, %s)', + $repository_phid, + $uri); + } + + $table->openTransaction(); + + queryfx( + $conn_w, + 'DELETE FROM %T WHERE repositoryPHID = %s', + $table->getTableName(), + $repository_phid); + + if ($sql) { + queryfx( + $conn_w, + 'INSERT INTO %T (repositoryPHID, repositoryURI) VALUES %Q', + $table->getTableName(), + implode(', ', $sql)); + } + + $table->saveTransaction(); + + } + +}