From 5e7a0917372acefb18e52f473208a18eddbdebb0 Mon Sep 17 00:00:00 2001 From: epriestley Date: Mon, 30 Jan 2017 10:39:35 -0800 Subject: [PATCH] Write an explicit edge for commit membership in packages Summary: Ref T10978. Currently, during commit import, we write an "Audit Not Required" auditor for commits which don't require an audit. This auditor is used to power the "Commits in this package" query in Owners. This conflates audits and commit/package membership. I think it might even predate edges. Code needs to dance around this mess and we get the wrong result in some cases, since auditors are now editable. Instead, write an explicit edge which just says "this commit is part of such-and-such packages". Then use that to run the query. Logical! I'll issue guidance on this but I'm not migrating it, since it fixes itself going forward and only really affects the UI in Owners. Test Plan: - Ran `bin/audit update-owners` with various arguments. - Viewed packages in web UI, saw them load the proper commits. - Queried by packages in Diffusion explicitly. - Clicked the "View All" link in Owners and got to the right search UI. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10978 Differential Revision: https://secure.phabricator.com/D17264 --- src/__phutil_library_map__.php | 4 + ...torAuditUpdateOwnersManagementWorkflow.php | 117 ++++++++++++++++++ .../query/PhabricatorCommitSearchEngine.php | 10 ++ .../DiffusionCommitHasPackageEdgeType.php | 8 ++ .../diffusion/query/DiffusionCommitQuery.php | 30 +++++ .../PhabricatorOwnersDetailController.php | 6 +- .../storage/PhabricatorRepositoryCommit.php | 23 ++++ ...habricatorRepositoryCommitOwnersWorker.php | 2 + 8 files changed, 197 insertions(+), 3 deletions(-) create mode 100644 src/applications/audit/management/PhabricatorAuditUpdateOwnersManagementWorkflow.php create mode 100644 src/applications/diffusion/edge/DiffusionCommitHasPackageEdgeType.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index f145dd88ec..6b510144e5 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -635,6 +635,7 @@ phutil_register_library_map(array( 'DiffusionCommitEditController' => 'applications/diffusion/controller/DiffusionCommitEditController.php', 'DiffusionCommitEditEngine' => 'applications/diffusion/editor/DiffusionCommitEditEngine.php', 'DiffusionCommitFulltextEngine' => 'applications/repository/search/DiffusionCommitFulltextEngine.php', + 'DiffusionCommitHasPackageEdgeType' => 'applications/diffusion/edge/DiffusionCommitHasPackageEdgeType.php', 'DiffusionCommitHasRevisionEdgeType' => 'applications/diffusion/edge/DiffusionCommitHasRevisionEdgeType.php', 'DiffusionCommitHasRevisionRelationship' => 'applications/diffusion/relationships/DiffusionCommitHasRevisionRelationship.php', 'DiffusionCommitHasTaskEdgeType' => 'applications/diffusion/edge/DiffusionCommitHasTaskEdgeType.php', @@ -1891,6 +1892,7 @@ phutil_register_library_map(array( 'PhabricatorAuditTransactionComment' => 'applications/audit/storage/PhabricatorAuditTransactionComment.php', 'PhabricatorAuditTransactionQuery' => 'applications/audit/query/PhabricatorAuditTransactionQuery.php', 'PhabricatorAuditTransactionView' => 'applications/audit/view/PhabricatorAuditTransactionView.php', + 'PhabricatorAuditUpdateOwnersManagementWorkflow' => 'applications/audit/management/PhabricatorAuditUpdateOwnersManagementWorkflow.php', 'PhabricatorAuthAccountView' => 'applications/auth/view/PhabricatorAuthAccountView.php', 'PhabricatorAuthApplication' => 'applications/auth/application/PhabricatorAuthApplication.php', 'PhabricatorAuthAuthFactorPHIDType' => 'applications/auth/phid/PhabricatorAuthAuthFactorPHIDType.php', @@ -5343,6 +5345,7 @@ phutil_register_library_map(array( 'DiffusionCommitEditController' => 'DiffusionController', 'DiffusionCommitEditEngine' => 'PhabricatorEditEngine', 'DiffusionCommitFulltextEngine' => 'PhabricatorFulltextEngine', + 'DiffusionCommitHasPackageEdgeType' => 'PhabricatorEdgeType', 'DiffusionCommitHasRevisionEdgeType' => 'PhabricatorEdgeType', 'DiffusionCommitHasRevisionRelationship' => 'DiffusionCommitRelationship', 'DiffusionCommitHasTaskEdgeType' => 'PhabricatorEdgeType', @@ -6786,6 +6789,7 @@ phutil_register_library_map(array( 'PhabricatorAuditTransactionComment' => 'PhabricatorApplicationTransactionComment', 'PhabricatorAuditTransactionQuery' => 'PhabricatorApplicationTransactionQuery', 'PhabricatorAuditTransactionView' => 'PhabricatorApplicationTransactionView', + 'PhabricatorAuditUpdateOwnersManagementWorkflow' => 'PhabricatorAuditManagementWorkflow', 'PhabricatorAuthAccountView' => 'AphrontView', 'PhabricatorAuthApplication' => 'PhabricatorApplication', 'PhabricatorAuthAuthFactorPHIDType' => 'PhabricatorPHIDType', diff --git a/src/applications/audit/management/PhabricatorAuditUpdateOwnersManagementWorkflow.php b/src/applications/audit/management/PhabricatorAuditUpdateOwnersManagementWorkflow.php new file mode 100644 index 0000000000..d0bb7e1039 --- /dev/null +++ b/src/applications/audit/management/PhabricatorAuditUpdateOwnersManagementWorkflow.php @@ -0,0 +1,117 @@ +setName('update-owners') + ->setExamples('**update-owners** ...') + ->setSynopsis(pht('Update package relationships for commits.')) + ->setArguments( + array( + array( + 'name' => 'all', + 'help' => pht('Update all commits in all repositories.'), + ), + array( + 'name' => 'objects', + 'wildcard' => true, + 'help' => pht('Update named commits and repositories.'), + ), + )); + } + + public function execute(PhutilArgumentParser $args) { + $viewer = $this->getViewer(); + + $all = $args->getArg('all'); + $names = $args->getArg('objects'); + + if (!$names && !$all) { + throw new PhutilArgumentUsageException( + pht( + 'Specify "--all" to update everything, or a list of specific '. + 'commits or repositories to update.')); + } else if ($names && $all) { + throw new PhutilArgumentUsageException( + pht( + 'Specify either a list of objects to update or "--all", but not '. + 'both.')); + } + + if ($all) { + $objects = new LiskMigrationIterator(new PhabricatorRepository()); + } else { + $query = id(new PhabricatorObjectQuery()) + ->setViewer($viewer) + ->withNames($names); + + $query->execute(); + + $objects = array(); + + $results = $query->getNamedResults(); + foreach ($names as $name) { + if (!isset($results[$name])) { + throw new PhutilArgumentUsageException( + pht( + 'Object "%s" is not a valid object.', + $name)); + } + + $object = $results[$name]; + if (!($object instanceof PhabricatorRepository) && + !($object instanceof PhabricatorRepositoryCommit)) { + throw new PhutilArgumentUsageException( + pht( + 'Object "%s" is not a valid repository or commit.', + $name)); + } + + $objects[] = $object; + } + } + + foreach ($objects as $object) { + if ($object instanceof PhabricatorRepository) { + $commits = id(new DiffusionCommitQuery()) + ->setViewer($viewer) + ->withRepository($object) + ->execute(); + } else { + $commits = array($object); + } + + foreach ($commits as $commit) { + $repository = $commit->getRepository(); + + $affected_paths = PhabricatorOwnerPathQuery::loadAffectedPaths( + $repository, + $commit, + $viewer); + + $affected_packages = PhabricatorOwnersPackage::loadAffectedPackages( + $repository, + $affected_paths); + + $monograms = mpull($affected_packages, 'getMonogram'); + if ($monograms) { + $monograms = implode(', ', $monograms); + } else { + $monograms = pht('none'); + } + + echo tsprintf( + "%s\n", + pht( + 'Updating "%s" (%s)...', + $commit->getDisplayName(), + $monograms)); + + $commit->writeOwnersEdges(mpull($affected_packages, 'getPHID')); + } + } + } + +} diff --git a/src/applications/audit/query/PhabricatorCommitSearchEngine.php b/src/applications/audit/query/PhabricatorCommitSearchEngine.php index f24b12d2b8..0b7d01ad86 100644 --- a/src/applications/audit/query/PhabricatorCommitSearchEngine.php +++ b/src/applications/audit/query/PhabricatorCommitSearchEngine.php @@ -45,6 +45,10 @@ final class PhabricatorCommitSearchEngine $query->withRepositoryPHIDs($map['repositoryPHIDs']); } + if ($map['packagePHIDs']) { + $query->withPackagePHIDs($map['packagePHIDs']); + } + return $query; } @@ -78,6 +82,12 @@ final class PhabricatorCommitSearchEngine ->setConduitKey('repositories') ->setAliases(array('repository', 'repositories', 'repositoryPHID')) ->setDatasource(new DiffusionRepositoryDatasource()), + id(new PhabricatorSearchDatasourceField()) + ->setLabel(pht('Packages')) + ->setKey('packagePHIDs') + ->setConduitKey('packages') + ->setAliases(array('package', 'packages', 'packagePHID')) + ->setDatasource(new PhabricatorOwnersPackageDatasource()), ); } diff --git a/src/applications/diffusion/edge/DiffusionCommitHasPackageEdgeType.php b/src/applications/diffusion/edge/DiffusionCommitHasPackageEdgeType.php new file mode 100644 index 0000000000..7a35b30a76 --- /dev/null +++ b/src/applications/diffusion/edge/DiffusionCommitHasPackageEdgeType.php @@ -0,0 +1,8 @@ +packagePHIDs = $package_phids; + return $this; + } + public function withStatuses(array $statuses) { $this->statuses = $statuses; return $this; @@ -498,6 +504,13 @@ final class DiffusionCommitQuery $this->statuses); } + if ($this->packagePHIDs !== null) { + $where[] = qsprintf( + $conn, + 'package.dst IN (%Ls)', + $this->packagePHIDs); + } + return $where; } @@ -519,6 +532,10 @@ final class DiffusionCommitQuery return (bool)$this->responsiblePHIDs; } + private function shouldJoinOwners() { + return (bool)$this->packagePHIDs; + } + protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { $join = parent::buildJoinClauseParts($conn); $audit_request = new PhabricatorRepositoryAuditRequest(); @@ -537,6 +554,15 @@ final class DiffusionCommitQuery $audit_request->getTableName()); } + if ($this->shouldJoinOwners()) { + $join[] = qsprintf( + $conn, + 'JOIN %T package ON commit.phid = package.src + AND package.type = %s', + PhabricatorEdgeConfig::TABLE_NAME_EDGE, + DiffusionCommitHasPackageEdgeType::EDGECONST); + } + return $join; } @@ -549,6 +575,10 @@ final class DiffusionCommitQuery return true; } + if ($this->shouldJoinOwners()) { + return true; + } + return parent::shouldGroupQueryResultRows(); } diff --git a/src/applications/owners/controller/PhabricatorOwnersDetailController.php b/src/applications/owners/controller/PhabricatorOwnersDetailController.php index b641290f56..b6260416da 100644 --- a/src/applications/owners/controller/PhabricatorOwnersDetailController.php +++ b/src/applications/owners/controller/PhabricatorOwnersDetailController.php @@ -68,14 +68,14 @@ final class PhabricatorOwnersDetailController $commit_uri = id(new PhutilURI('/diffusion/commit/')) ->setQueryParams( array( - 'auditorPHIDs' => $package->getPHID(), + 'package' => $package->getPHID(), )); $status_concern = PhabricatorAuditCommitStatusConstants::CONCERN_RAISED; $attention_commits = id(new DiffusionCommitQuery()) ->setViewer($request->getUser()) - ->withAuditorPHIDs(array($package->getPHID())) + ->withPackagePHIDs(array($package->getPHID())) ->withStatuses( array( $status_concern, @@ -102,7 +102,7 @@ final class PhabricatorOwnersDetailController $all_commits = id(new DiffusionCommitQuery()) ->setViewer($request->getUser()) - ->withAuditorPHIDs(array($package->getPHID())) + ->withPackagePHIDs(array($package->getPHID())) ->needCommitData(true) ->needAuditRequests(true) ->setLimit(25) diff --git a/src/applications/repository/storage/PhabricatorRepositoryCommit.php b/src/applications/repository/storage/PhabricatorRepositoryCommit.php index 5d826f9118..9eac5e6bf6 100644 --- a/src/applications/repository/storage/PhabricatorRepositoryCommit.php +++ b/src/applications/repository/storage/PhabricatorRepositoryCommit.php @@ -262,6 +262,29 @@ final class PhabricatorRepositoryCommit return isset($map[$audit->getAuditorPHID()]); } + public function writeOwnersEdges(array $package_phids) { + $src_phid = $this->getPHID(); + $edge_type = DiffusionCommitHasPackageEdgeType::EDGECONST; + + $editor = new PhabricatorEdgeEditor(); + + $dst_phids = PhabricatorEdgeQuery::loadDestinationPHIDs( + $src_phid, + $edge_type); + + foreach ($dst_phids as $dst_phid) { + $editor->removeEdge($src_phid, $edge_type, $dst_phid); + } + + foreach ($package_phids as $package_phid) { + $editor->addEdge($src_phid, $edge_type, $package_phid); + } + + $editor->save(); + + return $this; + } + public function getAuditorPHIDsForEdit() { $audits = $this->getAudits(); return mpull($audits, 'getAuditorPHID'); diff --git a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php index 20869ce273..bee7546671 100644 --- a/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php +++ b/src/applications/repository/worker/PhabricatorRepositoryCommitOwnersWorker.php @@ -42,6 +42,8 @@ final class PhabricatorRepositoryCommitOwnersWorker $repository, $affected_paths); + $commit->writeOwnersEdges(mpull($affected_packages, 'getPHID')); + if (!$affected_packages) { return; }