From 26ba4e87172037be19c53507c691df473da78ca6 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 23 Dec 2015 03:12:20 -0800 Subject: [PATCH] Materialize parent project memberships Summary: Ref T10010. Subprojects have the following general membership rule: if you are a member of a subproject ("Engineering > Backend"), you are also a member of the parent project. It would be unreasonably difficult to implement this rule directly in SQL when querying `withMemberPHIDs()`, because we'd have to do an arbitrarily large number of arbitrarily deep joins, or fetch and then requery a lot of data. Instead, introduce "materailized members", which are just a copy of all the effective members of a project. When a subproject has a membership change, we go recompute the effective membership of all the parent projects. Then we can just JOIN to satisfy `withMemberPHIDs()`. Having this process avialable will also be useful in the future, when a project's membership might be defined by some external source. Also make milestones mostly work like we'd expect them to with respect to membership and visibility. Test Plan: - Added and executed unit tests. - Changed project members, verified materialized members populated correctly in the database. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10010 Differential Revision: https://secure.phabricator.com/D14863 --- src/__phutil_library_map__.php | 4 + .../PhabricatorProjectCoreTestCase.php | 72 +++++++++++++- ...catorProjectMaterializedMemberEdgeType.php | 8 ++ .../PhabricatorProjectTransactionEditor.php | 50 ++++++++++ ...ProjectsMembershipIndexEngineExtension.php | 98 +++++++++++++++++++ .../project/query/PhabricatorProjectQuery.php | 44 +++++++-- .../storage/PhabricatorProjectTransaction.php | 1 + 7 files changed, 268 insertions(+), 9 deletions(-) create mode 100644 src/applications/project/edge/PhabricatorProjectMaterializedMemberEdgeType.php create mode 100644 src/applications/project/engineextension/PhabricatorProjectsMembershipIndexEngineExtension.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index d5c72b501d..bd304f920e 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2855,6 +2855,7 @@ phutil_register_library_map(array( 'PhabricatorProjectLogicalOrNotDatasource' => 'applications/project/typeahead/PhabricatorProjectLogicalOrNotDatasource.php', 'PhabricatorProjectLogicalUserDatasource' => 'applications/project/typeahead/PhabricatorProjectLogicalUserDatasource.php', 'PhabricatorProjectLogicalViewerDatasource' => 'applications/project/typeahead/PhabricatorProjectLogicalViewerDatasource.php', + 'PhabricatorProjectMaterializedMemberEdgeType' => 'applications/project/edge/PhabricatorProjectMaterializedMemberEdgeType.php', 'PhabricatorProjectMemberOfProjectEdgeType' => 'applications/project/edge/PhabricatorProjectMemberOfProjectEdgeType.php', 'PhabricatorProjectMembersDatasource' => 'applications/project/typeahead/PhabricatorProjectMembersDatasource.php', 'PhabricatorProjectMembersEditController' => 'applications/project/controller/PhabricatorProjectMembersEditController.php', @@ -2889,6 +2890,7 @@ phutil_register_library_map(array( 'PhabricatorProjectsEditEngineExtension' => 'applications/project/engineextension/PhabricatorProjectsEditEngineExtension.php', 'PhabricatorProjectsEditField' => 'applications/transactions/editfield/PhabricatorProjectsEditField.php', 'PhabricatorProjectsFulltextEngineExtension' => 'applications/project/engineextension/PhabricatorProjectsFulltextEngineExtension.php', + 'PhabricatorProjectsMembershipIndexEngineExtension' => 'applications/project/engineextension/PhabricatorProjectsMembershipIndexEngineExtension.php', 'PhabricatorProjectsPolicyRule' => 'applications/project/policyrule/PhabricatorProjectsPolicyRule.php', 'PhabricatorProjectsSearchEngineAttachment' => 'applications/project/engineextension/PhabricatorProjectsSearchEngineAttachment.php', 'PhabricatorProjectsSearchEngineExtension' => 'applications/project/engineextension/PhabricatorProjectsSearchEngineExtension.php', @@ -7189,6 +7191,7 @@ phutil_register_library_map(array( 'PhabricatorProjectLogicalOrNotDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorProjectLogicalUserDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorProjectLogicalViewerDatasource' => 'PhabricatorTypeaheadDatasource', + 'PhabricatorProjectMaterializedMemberEdgeType' => 'PhabricatorEdgeType', 'PhabricatorProjectMemberOfProjectEdgeType' => 'PhabricatorEdgeType', 'PhabricatorProjectMembersDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorProjectMembersEditController' => 'PhabricatorProjectController', @@ -7226,6 +7229,7 @@ phutil_register_library_map(array( 'PhabricatorProjectsEditEngineExtension' => 'PhabricatorEditEngineExtension', 'PhabricatorProjectsEditField' => 'PhabricatorTokenizerEditField', 'PhabricatorProjectsFulltextEngineExtension' => 'PhabricatorFulltextEngineExtension', + 'PhabricatorProjectsMembershipIndexEngineExtension' => 'PhabricatorIndexEngineExtension', 'PhabricatorProjectsPolicyRule' => 'PhabricatorPolicyRule', 'PhabricatorProjectsSearchEngineAttachment' => 'PhabricatorSearchEngineAttachment', 'PhabricatorProjectsSearchEngineExtension' => 'PhabricatorSearchEngineExtension', diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php index 7925b8849c..e239bd645f 100644 --- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php +++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php @@ -187,6 +187,68 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase { $this->assertEqual(2, count($projects)); } + public function testMemberMaterialization() { + $material_type = PhabricatorProjectMaterializedMemberEdgeType::EDGECONST; + + $user = $this->createUser(); + $user->save(); + + $parent = $this->createProject($user); + $child = $this->createProject($user, $parent); + + $this->joinProject($child, $user); + + $parent_material = PhabricatorEdgeQuery::loadDestinationPHIDs( + $parent->getPHID(), + $material_type); + + $this->assertEqual( + array($user->getPHID()), + $parent_material); + } + + public function testMilestones() { + $user = $this->createUser(); + $user->save(); + + $parent = $this->createProject($user); + + $m1 = $this->createProject($user, $parent, true); + $m2 = $this->createProject($user, $parent, true); + $m3 = $this->createProject($user, $parent, true); + + $this->assertEqual(1, $m1->getMilestoneNumber()); + $this->assertEqual(2, $m2->getMilestoneNumber()); + $this->assertEqual(3, $m3->getMilestoneNumber()); + } + + public function testMilestoneMembership() { + $user = $this->createUser(); + $user->save(); + + $parent = $this->createProject($user); + $milestone = $this->createProject($user, $parent, true); + + $this->joinProject($parent, $user); + + $milestone = id(new PhabricatorProjectQuery()) + ->setViewer($user) + ->withPHIDs(array($milestone->getPHID())) + ->executeOne(); + + $this->assertTrue($milestone->isUserMember($user->getPHID())); + + $milestone = id(new PhabricatorProjectQuery()) + ->setViewer($user) + ->withPHIDs(array($milestone->getPHID())) + ->needMembers(true) + ->executeOne(); + + $this->assertEqual( + array($user->getPHID()), + $milestone->getMemberPHIDs()); + } + public function testParentProject() { $user = $this->createUser(); $user->save(); @@ -396,10 +458,12 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase { private function createProject( PhabricatorUser $user, - PhabricatorProject $parent = null) { + PhabricatorProject $parent = null, + $is_milestone = false) { $project = PhabricatorProject::initializeNewProject($user); + $name = pht('Test Project %d', mt_rand()); $xactions = array(); @@ -414,6 +478,12 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase { ->setNewValue($parent->getPHID()); } + if ($is_milestone) { + $xactions[] = id(new PhabricatorProjectTransaction()) + ->setTransactionType(PhabricatorProjectTransaction::TYPE_MILESTONE) + ->setNewValue(true); + } + $this->applyTransactions($project, $user, $xactions); return $project; diff --git a/src/applications/project/edge/PhabricatorProjectMaterializedMemberEdgeType.php b/src/applications/project/edge/PhabricatorProjectMaterializedMemberEdgeType.php new file mode 100644 index 0000000000..7860495917 --- /dev/null +++ b/src/applications/project/edge/PhabricatorProjectMaterializedMemberEdgeType.php @@ -0,0 +1,8 @@ +getIsMembershipLocked(); case PhabricatorProjectTransaction::TYPE_PARENT: + case PhabricatorProjectTransaction::TYPE_MILESTONE: return null; } @@ -74,6 +76,20 @@ final class PhabricatorProjectTransactionEditor case PhabricatorProjectTransaction::TYPE_LOCKED: case PhabricatorProjectTransaction::TYPE_PARENT: return $xaction->getNewValue(); + case PhabricatorProjectTransaction::TYPE_MILESTONE: + $current = queryfx_one( + $object->establishConnection('w'), + 'SELECT MAX(milestoneNumber) n + FROM %T + WHERE parentProjectPHID = %s', + $object->getTableName(), + $object->getParentProject()->getPHID()); + if (!$current) { + $number = 1; + } else { + $number = (int)$current['n'] + 1; + } + return $number; } return parent::getCustomTransactionNewValue($object, $xaction); @@ -109,6 +125,9 @@ final class PhabricatorProjectTransactionEditor case PhabricatorProjectTransaction::TYPE_PARENT: $object->setParentProjectPHID($xaction->getNewValue()); return; + case PhabricatorProjectTransaction::TYPE_MILESTONE: + $object->setMilestoneNumber($xaction->getNewValue()); + return; } return parent::applyCustomInternalTransaction($object, $xaction); @@ -161,6 +180,7 @@ final class PhabricatorProjectTransactionEditor case PhabricatorProjectTransaction::TYPE_COLOR: case PhabricatorProjectTransaction::TYPE_LOCKED: case PhabricatorProjectTransaction::TYPE_PARENT: + case PhabricatorProjectTransaction::TYPE_MILESTONE: return; } @@ -590,4 +610,34 @@ final class PhabricatorProjectTransactionEditor ->setProjectPHID($object->getPHID()) ->save(); } + + + protected function applyFinalEffects( + PhabricatorLiskDAO $object, + array $xactions) { + + $materialize = false; + foreach ($xactions as $xaction) { + switch ($xaction->getTransactionType()) { + case PhabricatorTransactions::TYPE_EDGE: + switch ($xaction->getMetadataValue('edge:type')) { + case PhabricatorProjectProjectHasMemberEdgeType::EDGECONST: + $materialize = true; + break; + } + break; + case PhabricatorProjectTransaction::TYPE_PARENT: + $materialize = true; + break; + } + } + + if ($materialize) { + id(new PhabricatorProjectsMembershipIndexEngineExtension()) + ->rematerialize($object); + } + + return parent::applyFinalEffects($object, $xactions); + } + } diff --git a/src/applications/project/engineextension/PhabricatorProjectsMembershipIndexEngineExtension.php b/src/applications/project/engineextension/PhabricatorProjectsMembershipIndexEngineExtension.php new file mode 100644 index 0000000000..e1460381ed --- /dev/null +++ b/src/applications/project/engineextension/PhabricatorProjectsMembershipIndexEngineExtension.php @@ -0,0 +1,98 @@ +rematerialize($object); + } + + public function rematerialize(PhabricatorProject $project) { + $materialize = $project->getAncestorProjects(); + array_unshift($materialize, $project); + + foreach ($materialize as $project) { + $this->materializeProject($project); + } + } + + private function materializeProject(PhabricatorProject $project) { + if ($project->isMilestone()) { + return; + } + + $material_type = PhabricatorProjectMaterializedMemberEdgeType::EDGECONST; + $member_type = PhabricatorProjectProjectHasMemberEdgeType::EDGECONST; + + $project_phid = $project->getPHID(); + + $descendants = id(new PhabricatorProjectQuery()) + ->setViewer($this->getViewer()) + ->withAncestorProjectPHIDs(array($project->getPHID())) + ->withIsMilestone(false) + ->withHasSubprojects(false) + ->execute(); + $descendant_phids = mpull($descendants, 'getPHID'); + + if ($descendant_phids) { + $source_phids = $descendant_phids; + $has_subprojects = true; + } else { + $source_phids = array($project->getPHID()); + $has_subprojects = false; + } + + $conn_w = $project->establishConnection('w'); + + $project->openTransaction(); + + // Delete any existing materialized member edges. + queryfx( + $conn_w, + 'DELETE FROM %T WHERE src = %s AND type = %s', + PhabricatorEdgeConfig::TABLE_NAME_EDGE, + $project_phid, + $material_type); + + // Copy current member edges to create new materialized edges. + queryfx( + $conn_w, + 'INSERT IGNORE INTO %T (src, type, dst, dateCreated, seq) + SELECT %s, %d, dst, dateCreated, seq FROM %T + WHERE src IN (%Ls) AND type = %d', + PhabricatorEdgeConfig::TABLE_NAME_EDGE, + $project_phid, + $material_type, + PhabricatorEdgeConfig::TABLE_NAME_EDGE, + $source_phids, + $member_type); + + // Update the hasSubprojects flag. + queryfx( + $conn_w, + 'UPDATE %T SET hasSubprojects = %d WHERE id = %d', + $project->getTableName(), + (int)$has_subprojects, + $project->getID()); + + $project->saveTransaction(); + } + +} diff --git a/src/applications/project/query/PhabricatorProjectQuery.php b/src/applications/project/query/PhabricatorProjectQuery.php index bd0134143e..264cce9431 100644 --- a/src/applications/project/query/PhabricatorProjectQuery.php +++ b/src/applications/project/query/PhabricatorProjectQuery.php @@ -14,6 +14,7 @@ final class PhabricatorProjectQuery private $ancestorPHIDs; private $parentPHIDs; private $isMilestone; + private $hasSubprojects; private $minDepth; private $maxDepth; @@ -89,6 +90,15 @@ final class PhabricatorProjectQuery return $this; } + public function withHasSubprojects($has_subprojects) { + $this->hasSubprojects = $has_subprojects; + return $this; + } + + public function getProperty() { + return $this->property; + } + public function withDepthBetween($min, $max) { $this->minDepth = $min; $this->maxDepth = $max; @@ -156,12 +166,8 @@ final class PhabricatorProjectQuery } protected function willFilterPage(array $projects) { - $project_phids = array(); $ancestor_paths = array(); - foreach ($projects as $project) { - $project_phids[] = $project->getPHID(); - foreach ($project->getAncestorProjectPaths() as $path) { $ancestor_paths[$path] = $path; } @@ -178,7 +184,6 @@ final class PhabricatorProjectQuery $projects = $this->linkProjectGraph($projects, $ancestors); $viewer_phid = $this->getViewer()->getPHID(); - $project_phids = mpull($projects, 'getPHID'); $member_type = PhabricatorProjectProjectHasMemberEdgeType::EDGECONST; $watcher_type = PhabricatorObjectHasWatcherEdgeType::EDGECONST; @@ -189,8 +194,18 @@ final class PhabricatorProjectQuery $types[] = $watcher_type; } + $all_sources = array(); + foreach ($projects as $project) { + if ($project->isMilestone()) { + $phid = $project->getParentProjectPHID(); + } else { + $phid = $project->getPHID(); + } + $all_sources[$phid] = $phid; + } + $edge_query = id(new PhabricatorEdgeQuery()) - ->withSourcePHIDs($project_phids) + ->withSourcePHIDs($all_sources) ->withEdgeTypes($types); // If we only need to know if the viewer is a member, we can restrict @@ -205,8 +220,14 @@ final class PhabricatorProjectQuery foreach ($projects as $project) { $project_phid = $project->getPHID(); + if ($project->isMilestone()) { + $source_phids = array($project->getParentProjectPHID()); + } else { + $source_phids = array($project_phid); + } + $member_phids = $edge_query->getDestinationPHIDs( - array($project_phid), + $source_phids, array($member_type)); if (in_array($viewer_phid, $member_phids)) { @@ -219,7 +240,7 @@ final class PhabricatorProjectQuery if ($this->needWatchers) { $watcher_phids = $edge_query->getDestinationPHIDs( - array($project_phid), + $source_phids, array($watcher_type)); $project->attachWatcherPHIDs($watcher_phids); $project->setIsUserWatcher( @@ -408,6 +429,13 @@ final class PhabricatorProjectQuery } } + if ($this->hasSubprojects !== null) { + $where[] = qsprintf( + $conn, + 'hasSubprojects = %d', + (int)$this->hasSubprojects); + } + if ($this->minDepth !== null) { $where[] = qsprintf( $conn, diff --git a/src/applications/project/storage/PhabricatorProjectTransaction.php b/src/applications/project/storage/PhabricatorProjectTransaction.php index f22c763da1..b928d43a91 100644 --- a/src/applications/project/storage/PhabricatorProjectTransaction.php +++ b/src/applications/project/storage/PhabricatorProjectTransaction.php @@ -11,6 +11,7 @@ final class PhabricatorProjectTransaction const TYPE_COLOR = 'project:color'; const TYPE_LOCKED = 'project:locked'; const TYPE_PARENT = 'project:parent'; + const TYPE_MILESTONE = 'project:milestone'; // NOTE: This is deprecated, members are just a normal edge now. const TYPE_MEMBERS = 'project:members';