From 14ae3c099c2e4ce6c706bbaa4aa9fcb4bc7d3dec Mon Sep 17 00:00:00 2001 From: epriestley Date: Tue, 29 Dec 2015 05:06:47 -0800 Subject: [PATCH] Make queries for Project "X" mean "X, or any subproject of X" Summary: Ref T10010. I think this is the desired/expected default behavior (e.g., searching for "Maniphest" should find tasks in any subproject or sprint of that project). I'll probably add an "exact(...)" function later to mean "only the Maniphest superproject, exactly, not any of its children". Test Plan: - Added and executed unit tests. - Ran various queries from the web UI. - Got sensible-seeming results. Reviewers: chad Reviewed By: chad Maniphest Tasks: T10010 Differential Revision: https://secure.phabricator.com/D14910 --- src/__phutil_library_map__.php | 4 +- .../PhabricatorProjectCoreTestCase.php | 151 ++++++++++++++++++ ...icatorProjectLogicalAncestorDatasource.php | 95 +++++++++++ ...PhabricatorProjectLogicalAndDatasource.php | 36 ----- .../PhabricatorProjectLogicalDatasource.php | 2 +- .../constraint/PhabricatorQueryConstraint.php | 1 + ...PhabricatorCursorPagedPolicyAwareQuery.php | 82 +++++++++- 7 files changed, 327 insertions(+), 44 deletions(-) create mode 100644 src/applications/project/typeahead/PhabricatorProjectLogicalAncestorDatasource.php delete mode 100644 src/applications/project/typeahead/PhabricatorProjectLogicalAndDatasource.php diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 0e06a9198a..03dae199af 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2857,7 +2857,7 @@ phutil_register_library_map(array( 'PhabricatorProjectListController' => 'applications/project/controller/PhabricatorProjectListController.php', 'PhabricatorProjectListView' => 'applications/project/view/PhabricatorProjectListView.php', 'PhabricatorProjectLockController' => 'applications/project/controller/PhabricatorProjectLockController.php', - 'PhabricatorProjectLogicalAndDatasource' => 'applications/project/typeahead/PhabricatorProjectLogicalAndDatasource.php', + 'PhabricatorProjectLogicalAncestorDatasource' => 'applications/project/typeahead/PhabricatorProjectLogicalAncestorDatasource.php', 'PhabricatorProjectLogicalDatasource' => 'applications/project/typeahead/PhabricatorProjectLogicalDatasource.php', 'PhabricatorProjectLogicalOrNotDatasource' => 'applications/project/typeahead/PhabricatorProjectLogicalOrNotDatasource.php', 'PhabricatorProjectLogicalUserDatasource' => 'applications/project/typeahead/PhabricatorProjectLogicalUserDatasource.php', @@ -7209,7 +7209,7 @@ phutil_register_library_map(array( 'PhabricatorProjectListController' => 'PhabricatorProjectController', 'PhabricatorProjectListView' => 'AphrontView', 'PhabricatorProjectLockController' => 'PhabricatorProjectController', - 'PhabricatorProjectLogicalAndDatasource' => 'PhabricatorTypeaheadCompositeDatasource', + 'PhabricatorProjectLogicalAncestorDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorProjectLogicalDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorProjectLogicalOrNotDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'PhabricatorProjectLogicalUserDatasource' => 'PhabricatorTypeaheadCompositeDatasource', diff --git a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php index c31c69fda9..36c78386f3 100644 --- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php +++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php @@ -711,6 +711,157 @@ final class PhabricatorProjectCoreTestCase extends PhabricatorTestCase { pht('Leave allowed without any permission.')); } + + public function testComplexConstraints() { + $user = $this->createUser(); + $user->save(); + + $engineering = $this->createProject($user); + $engineering_scan = $this->createProject($user, $engineering); + $engineering_warp = $this->createProject($user, $engineering); + + $exploration = $this->createProject($user); + $exploration_diplomacy = $this->createProject($user, $exploration); + + $task_engineering = $this->newTask( + $user, + array($engineering), + pht('Engineering Only')); + + $task_exploration = $this->newTask( + $user, + array($exploration), + pht('Exploration Only')); + + $task_warp_explore = $this->newTask( + $user, + array($engineering_warp, $exploration), + pht('Warp to New Planet')); + + $task_diplomacy_scan = $this->newTask( + $user, + array($engineering_scan, $exploration_diplomacy), + pht('Scan Diplomat')); + + $task_diplomacy = $this->newTask( + $user, + array($exploration_diplomacy), + pht('Diplomatic Meeting')); + + $task_warp_scan = $this->newTask( + $user, + array($engineering_scan, $engineering_warp), + pht('Scan Warp Drives')); + + $this->assertQueryByProjects( + $user, + array( + $task_engineering, + $task_warp_explore, + $task_diplomacy_scan, + $task_warp_scan, + ), + array($engineering), + pht('All Engineering')); + + $this->assertQueryByProjects( + $user, + array( + $task_diplomacy_scan, + $task_warp_scan, + ), + array($engineering_scan), + pht('All Scan')); + + $this->assertQueryByProjects( + $user, + array( + $task_warp_explore, + $task_diplomacy_scan, + ), + array($engineering, $exploration), + pht('Engineering + Exploration')); + + // This is testing that a query for "Parent" and "Parent > Child" works + // properly. + $this->assertQueryByProjects( + $user, + array( + $task_diplomacy_scan, + $task_warp_scan, + ), + array($engineering, $engineering_scan), + pht('Engineering + Scan')); + } + + private function newTask( + PhabricatorUser $viewer, + array $projects, + $name = null) { + + $task = ManiphestTask::initializeNewTask($viewer); + + if (!strlen($name)) { + $name = pht('Test Task'); + } + + $xactions = array(); + + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(ManiphestTransaction::TYPE_TITLE) + ->setNewValue($name); + + if ($projects) { + $xactions[] = id(new ManiphestTransaction()) + ->setTransactionType(PhabricatorTransactions::TYPE_EDGE) + ->setMetadataValue( + 'edge:type', + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST) + ->setNewValue( + array( + '=' => array_fuse(mpull($projects, 'getPHID')), + )); + } + + $editor = id(new ManiphestTransactionEditor()) + ->setActor($viewer) + ->setContentSource(PhabricatorContentSource::newConsoleSource()) + ->setContinueOnNoEffect(true) + ->applyTransactions($task, $xactions); + + return $task; + } + + private function assertQueryByProjects( + PhabricatorUser $viewer, + array $expect, + array $projects, + $label = null) { + + $datasource = id(new PhabricatorProjectLogicalDatasource()) + ->setViewer($viewer); + + $project_phids = mpull($projects, 'getPHID'); + $constraints = $datasource->evaluateTokens($project_phids); + + $query = id(new ManiphestTaskQuery()) + ->setViewer($viewer); + + $query->withEdgeLogicConstraints( + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, + $constraints); + + $tasks = $query->execute(); + + $expect_phids = mpull($expect, 'getTitle', 'getPHID'); + ksort($expect_phids); + + $actual_phids = mpull($tasks, 'getTitle', 'getPHID'); + ksort($actual_phids); + + $this->assertEqual($expect_phids, $actual_phids, $label); + } + private function refreshProject( PhabricatorProject $project, PhabricatorUser $viewer, diff --git a/src/applications/project/typeahead/PhabricatorProjectLogicalAncestorDatasource.php b/src/applications/project/typeahead/PhabricatorProjectLogicalAncestorDatasource.php new file mode 100644 index 0000000000..070cb88485 --- /dev/null +++ b/src/applications/project/typeahead/PhabricatorProjectLogicalAncestorDatasource.php @@ -0,0 +1,95 @@ +getViewer(); + + $all_projects = id(new PhabricatorProjectQuery()) + ->setViewer($viewer) + ->withAncestorProjectPHIDs($phids) + ->execute(); + + foreach ($phids as $phid) { + $map[$phid][] = $phid; + } + + foreach ($all_projects as $project) { + $project_phid = $project->getPHID(); + $map[$project_phid][] = $project_phid; + foreach ($project->getAncestorProjects() as $ancestor) { + $ancestor_phid = $ancestor->getPHID(); + + if (isset($phids[$project_phid]) && isset($phids[$ancestor_phid])) { + // This is a descendant of some other project in the query, so + // we don't need to query for that project. This happens if a user + // runs a query for both "Engineering" and "Engineering > Warp + // Drive". We can only ever match the "Warp Drive" results, so + // we do not need to add the weaker "Engineering" constraint. + $skip[$ancestor_phid] = true; + } + + $map[$ancestor_phid][] = $project_phid; + } + } + } + + foreach ($results as $key => $result) { + if (!is_string($result)) { + continue; + } + + if (empty($map[$result])) { + continue; + } + + // This constraint is implied by another, stronger constraint. + if (isset($skip[$result])) { + unset($results[$key]); + continue; + } + + // If we have duplicates, don't apply the second constraint. + $skip[$result] = true; + + $results[$key] = new PhabricatorQueryConstraint( + PhabricatorQueryConstraint::OPERATOR_ANCESTOR, + $map[$result]); + } + + return $results; + } + +} diff --git a/src/applications/project/typeahead/PhabricatorProjectLogicalAndDatasource.php b/src/applications/project/typeahead/PhabricatorProjectLogicalAndDatasource.php deleted file mode 100644 index a3016820a0..0000000000 --- a/src/applications/project/typeahead/PhabricatorProjectLogicalAndDatasource.php +++ /dev/null @@ -1,36 +0,0 @@ - $result) { - if (is_string($result)) { - $results[$key] = new PhabricatorQueryConstraint( - PhabricatorQueryConstraint::OPERATOR_AND, - $result); - } - } - - return $results; - } - -} diff --git a/src/applications/project/typeahead/PhabricatorProjectLogicalDatasource.php b/src/applications/project/typeahead/PhabricatorProjectLogicalDatasource.php index 1bce988339..b724e9f8ca 100644 --- a/src/applications/project/typeahead/PhabricatorProjectLogicalDatasource.php +++ b/src/applications/project/typeahead/PhabricatorProjectLogicalDatasource.php @@ -18,7 +18,7 @@ final class PhabricatorProjectLogicalDatasource public function getComponentDatasources() { return array( new PhabricatorProjectNoProjectsDatasource(), - new PhabricatorProjectLogicalAndDatasource(), + new PhabricatorProjectLogicalAncestorDatasource(), new PhabricatorProjectLogicalOrNotDatasource(), new PhabricatorProjectLogicalViewerDatasource(), new PhabricatorProjectLogicalUserDatasource(), diff --git a/src/infrastructure/query/constraint/PhabricatorQueryConstraint.php b/src/infrastructure/query/constraint/PhabricatorQueryConstraint.php index 7a04d9d7db..5b5702673b 100644 --- a/src/infrastructure/query/constraint/PhabricatorQueryConstraint.php +++ b/src/infrastructure/query/constraint/PhabricatorQueryConstraint.php @@ -6,6 +6,7 @@ final class PhabricatorQueryConstraint extends Phobject { const OPERATOR_OR = 'or'; const OPERATOR_NOT = 'not'; const OPERATOR_NULL = 'null'; + const OPERATOR_ANCESTOR = 'ancestor'; private $operator; private $value; diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 58df7aea33..9996f03ae0 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -1516,8 +1516,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $constraints = mgroup($constraints, 'getOperator'); foreach ($constraints as $operator => $list) { foreach ($list as $item) { - $value = $item->getValue(); - $this->edgeLogicConstraints[$edge_type][$operator][$value] = $item; + $this->edgeLogicConstraints[$edge_type][$operator][] = $item; } } @@ -1548,6 +1547,47 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $this->buildEdgeLogicTableAliasCount($alias)); } break; + case PhabricatorQueryConstraint::OPERATOR_ANCESTOR: + // This is tricky. We have a query which specifies multiple + // projects, each of which may have an arbitrarily large number + // of descendants. + + // Suppose the projects are "Engineering" and "Operations", and + // "Engineering" has subprojects X, Y and Z. + + // We first use `FIELD(dst, X, Y, Z)` to produce a 0 if a row + // is not part of Engineering at all, or some number other than + // 0 if it is. + + // Then we use `IF(..., idx, NULL)` to convert the 0 to a NULL and + // any other value to an index (say, 1) for the ancestor. + + // We build these up for every ancestor, then use `COALESCE(...)` + // to select the non-null one, giving us an ancestor which this + // row is a member of. + + // From there, we use `COUNT(DISTINCT(...))` to make sure that + // each result row is a member of all ancestors. + if (count($list) > 1) { + $idx = 1; + $parts = array(); + foreach ($list as $constraint) { + $parts[] = qsprintf( + $conn, + 'IF(FIELD(%T.dst, %Ls) != 0, %d, NULL)', + $alias, + (array)$constraint->getValue(), + $idx++); + } + $parts = implode(', ', $parts); + + $select[] = qsprintf( + $conn, + 'COUNT(DISTINCT(COALESCE(%Q))) %T', + $parts, + $this->buildEdgeLogicTableAliasAncestor($alias)); + } + break; default: break; } @@ -1573,6 +1613,16 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery foreach ($constraints as $operator => $list) { $alias = $this->getEdgeLogicTableAlias($operator, $type); + + $phids = array(); + foreach ($list as $constraint) { + $value = (array)$constraint->getValue(); + foreach ($value as $v) { + $phids[$v] = $v; + } + } + $phids = array_keys($phids); + switch ($operator) { case PhabricatorQueryConstraint::OPERATOR_NOT: $joins[] = qsprintf( @@ -1586,8 +1636,9 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $alias, $type, $alias, - mpull($list, 'getValue')); + $phids); break; + case PhabricatorQueryConstraint::OPERATOR_ANCESTOR: case PhabricatorQueryConstraint::OPERATOR_AND: case PhabricatorQueryConstraint::OPERATOR_OR: // If we're including results with no matches, we have to degrade @@ -1611,7 +1662,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery $alias, $type, $alias, - mpull($list, 'getValue')); + $phids); break; case PhabricatorQueryConstraint::OPERATOR_NULL: $joins[] = qsprintf( @@ -1711,6 +1762,15 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery count($list)); } break; + case PhabricatorQueryConstraint::OPERATOR_ANCESTOR: + if (count($list) > 1) { + $having[] = qsprintf( + $conn, + '%T = %d', + $this->buildEdgeLogicTableAliasAncestor($alias), + count($list)); + } + break; } } } @@ -1729,6 +1789,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery case PhabricatorQueryConstraint::OPERATOR_NOT: case PhabricatorQueryConstraint::OPERATOR_AND: case PhabricatorQueryConstraint::OPERATOR_OR: + case PhabricatorQueryConstraint::OPERATOR_ANCESTOR: if (count($list) > 1) { return true; } @@ -1758,6 +1819,13 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery return $alias.'_count'; } + /** + * @task edgelogic + */ + private function buildEdgeLogicTableAliasAncestor($alias) { + return $alias.'_ancestor'; + } + /** * Select certain edge logic constraint values. @@ -1781,7 +1849,10 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery } foreach ($constraints as $operator => $list) { foreach ($list as $constraint) { - $values[] = $constraint->getValue(); + $value = (array)$constraint->getValue(); + foreach ($value as $v) { + $values[] = $v; + } } } } @@ -1812,6 +1883,7 @@ abstract class PhabricatorCursorPagedPolicyAwareQuery PhabricatorQueryConstraint::OPERATOR_AND, PhabricatorQueryConstraint::OPERATOR_OR, PhabricatorQueryConstraint::OPERATOR_NOT, + PhabricatorQueryConstraint::OPERATOR_ANCESTOR, )); if ($project_phids) { $projects = id(new PhabricatorProjectQuery())