diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -2857,7 +2857,7 @@ '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 @@ '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 --- a/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php +++ b/src/applications/project/__tests__/PhabricatorProjectCoreTestCase.php @@ -711,6 +711,157 @@ 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 --- /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 --- 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 --- a/src/applications/project/typeahead/PhabricatorProjectLogicalDatasource.php +++ b/src/applications/project/typeahead/PhabricatorProjectLogicalDatasource.php @@ -18,7 +18,7 @@ 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 --- a/src/infrastructure/query/constraint/PhabricatorQueryConstraint.php +++ b/src/infrastructure/query/constraint/PhabricatorQueryConstraint.php @@ -6,6 +6,7 @@ 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 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -1516,8 +1516,7 @@ $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 @@ $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 @@ 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 @@ $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 @@ $alias, $type, $alias, - mpull($list, 'getValue')); + $phids); break; case PhabricatorQueryConstraint::OPERATOR_NULL: $joins[] = qsprintf( @@ -1711,6 +1762,15 @@ 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 @@ 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 @@ return $alias.'_count'; } + /** + * @task edgelogic + */ + private function buildEdgeLogicTableAliasAncestor($alias) { + return $alias.'_ancestor'; + } + /** * Select certain edge logic constraint values. @@ -1781,7 +1849,10 @@ } 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 @@ PhabricatorQueryConstraint::OPERATOR_AND, PhabricatorQueryConstraint::OPERATOR_OR, PhabricatorQueryConstraint::OPERATOR_NOT, + PhabricatorQueryConstraint::OPERATOR_ANCESTOR, )); if ($project_phids) { $projects = id(new PhabricatorProjectQuery())