diff --git a/src/applications/home/controller/PhabricatorHomeMainController.php b/src/applications/home/controller/PhabricatorHomeMainController.php --- a/src/applications/home/controller/PhabricatorHomeMainController.php +++ b/src/applications/home/controller/PhabricatorHomeMainController.php @@ -179,7 +179,10 @@ ->setViewer($user) ->withStatuses(ManiphestTaskStatus::getOpenStatusConstants()) ->withPriorities(array($needs_triage)) - ->withAnyProjects(mpull($projects, 'getPHID')) + ->withEdgeLogicPHIDs( + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, + PhabricatorQueryConstraint::OPERATOR_OR, + mpull($projects, 'getPHID')) ->needProjectPHIDs(true) ->setLimit(10); $tasks = $task_query->execute(); diff --git a/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php b/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php --- a/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php +++ b/src/applications/maniphest/conduit/ManiphestQueryConduitAPIMethod.php @@ -79,7 +79,10 @@ $projects = $request->getValue('projectPHIDs'); if ($projects) { - $query->withAllProjects($projects); + $query->withEdgeLogicPHIDs( + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, + PhabricatorQueryConstraint::OPERATOR_AND, + $projects); } $ccs = $request->getValue('ccPHIDs'); diff --git a/src/applications/maniphest/controller/ManiphestReportController.php b/src/applications/maniphest/controller/ManiphestReportController.php --- a/src/applications/maniphest/controller/ManiphestReportController.php +++ b/src/applications/maniphest/controller/ManiphestReportController.php @@ -411,7 +411,10 @@ $handles = $this->loadViewerHandles($phids); $project_handle = $handles[$project_phid]; - $query->withAnyProjects($phids); + $query->withEdgeLogicPHIDs( + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, + PhabricatorQueryConstraint::OPERATOR_OR, + $phids); } $tasks = $query->execute(); diff --git a/src/applications/maniphest/query/ManiphestTaskQuery.php b/src/applications/maniphest/query/ManiphestTaskQuery.php --- a/src/applications/maniphest/query/ManiphestTaskQuery.php +++ b/src/applications/maniphest/query/ManiphestTaskQuery.php @@ -11,12 +11,7 @@ private $authorPHIDs = array(); private $ownerPHIDs = array(); private $includeUnowned = null; - private $projectPHIDs = array(); - private $xprojectPHIDs = array(); private $subscriberPHIDs = array(); - private $anyProjectPHIDs = array(); - private $anyUserProjectPHIDs = array(); - private $includeNoProject = null; private $dateCreatedAfter; private $dateCreatedBefore; private $dateModifiedAfter; @@ -57,7 +52,6 @@ private $needProjectPHIDs; private $blockingTasks; private $blockedTasks; - private $projectPolicyCheckFailed = false; public function withAuthors(array $authors) { $this->authorPHIDs = $authors; @@ -89,39 +83,6 @@ return $this; } - public function withAllProjects(array $projects) { - $this->includeNoProject = false; - foreach ($projects as $k => $phid) { - if ($phid == ManiphestTaskOwner::PROJECT_NO_PROJECT) { - $this->includeNoProject = true; - unset($projects[$k]); - } - } - $this->projectPHIDs = $projects; - return $this; - } - - /** - * Add an additional "all projects" constraint to existing filters. - * - * This is used by boards to supplement queries. - * - * @param list List of project PHIDs to add to any existing constraint. - * @return this - */ - public function addWithAllProjects(array $projects) { - if ($this->projectPHIDs === null) { - $this->projectPHIDs = array(); - } - - return $this->withAllProjects(array_merge($this->projectPHIDs, $projects)); - } - - public function withoutProjects(array $projects) { - $this->xprojectPHIDs = $projects; - return $this; - } - public function withStatus($status) { $this->status = $status; return $this; @@ -168,16 +129,6 @@ return $this; } - public function withAnyProjects(array $projects) { - $this->anyProjectPHIDs = $projects; - return $this; - } - - public function withAnyUserProjects(array $users) { - $this->anyUserProjectPHIDs = $users; - return $this; - } - /** * True returns tasks that are blocking other tasks only. * False returns tasks that are not blocking other tasks only. @@ -241,27 +192,6 @@ } protected function willExecute() { - // Make sure the user can see any projects specified in this - // query FIRST. - if ($this->projectPHIDs) { - $projects = id(new PhabricatorProjectQuery()) - ->setViewer($this->getViewer()) - ->withPHIDs($this->projectPHIDs) - ->execute(); - $projects = mpull($projects, null, 'getPHID'); - foreach ($this->projectPHIDs as $index => $phid) { - $project = idx($projects, $phid); - if (!$project) { - unset($this->projectPHIDs[$index]); - continue; - } - } - if (!$this->projectPHIDs) { - $this->projectPolicyCheckFailed = true; - } - $this->projectPHIDs = array_values($this->projectPHIDs); - } - // If we already have an order vector, use it as provided. // TODO: This is a messy hack to make setOrderVector() stronger than // setPriority(). @@ -325,11 +255,6 @@ } protected function loadPage() { - - if ($this->projectPolicyCheckFailed) { - throw new PhabricatorEmptyQueryException(); - } - $task_dao = new ManiphestTask(); $conn = $task_dao->establishConnection('r'); @@ -341,10 +266,6 @@ $where[] = $this->buildDependenciesWhereClause($conn); $where[] = $this->buildAuthorWhereClause($conn); $where[] = $this->buildOwnerWhereClause($conn); - $where[] = $this->buildProjectWhereClause($conn); - $where[] = $this->buildAnyProjectWhereClause($conn); - $where[] = $this->buildAnyUserProjectWhereClause($conn); - $where[] = $this->buildXProjectWhereClause($conn); $where[] = $this->buildFullTextWhereClause($conn); if ($this->dateCreatedAfter) { @@ -407,11 +328,6 @@ $where = $this->formatWhereClause($where); - $count = ''; - if (count($this->projectPHIDs) > 1) { - $count = ', COUNT(project.dst) projectCount'; - } - $group_column = ''; switch ($this->groupBy) { case self::GROUP_PROJECT: @@ -423,9 +339,8 @@ $rows = queryfx_all( $conn, - '%Q %Q %Q FROM %T task %Q %Q %Q %Q %Q %Q', + '%Q %Q FROM %T task %Q %Q %Q %Q %Q %Q', $this->buildSelectClause($conn), - $count, $group_column, $task_dao->getTableName(), $this->buildJoinClause($conn), @@ -689,84 +604,11 @@ return '('.implode(') OR (', $parts).')'; } - private function buildProjectWhereClause(AphrontDatabaseConnection $conn) { - if (!$this->projectPHIDs && !$this->includeNoProject) { - return null; - } - - $parts = array(); - if ($this->projectPHIDs) { - $parts[] = qsprintf( - $conn, - 'project.dst in (%Ls)', - $this->projectPHIDs); - } - if ($this->includeNoProject) { - $parts[] = qsprintf( - $conn, - 'project.dst IS NULL'); - } - - return '('.implode(') OR (', $parts).')'; - } - - private function buildAnyProjectWhereClause(AphrontDatabaseConnection $conn) { - if (!$this->anyProjectPHIDs) { - return null; - } - - return qsprintf( - $conn, - 'anyproject.dst IN (%Ls)', - $this->anyProjectPHIDs); - } - - private function buildAnyUserProjectWhereClause( - AphrontDatabaseConnection $conn) { - if (!$this->anyUserProjectPHIDs) { - return null; - } - - $projects = id(new PhabricatorProjectQuery()) - ->setViewer($this->getViewer()) - ->withMemberPHIDs($this->anyUserProjectPHIDs) - ->execute(); - $any_user_project_phids = mpull($projects, 'getPHID'); - if (!$any_user_project_phids) { - throw new PhabricatorEmptyQueryException(); - } - - return qsprintf( - $conn, - 'anyproject.dst IN (%Ls)', - $any_user_project_phids); - } - - private function buildXProjectWhereClause(AphrontDatabaseConnection $conn) { - if (!$this->xprojectPHIDs) { - return null; - } - - return qsprintf( - $conn, - 'xproject.dst IS NULL'); - } - protected function buildJoinClauseParts(AphrontDatabaseConnection $conn_r) { $edge_table = PhabricatorEdgeConfig::TABLE_NAME_EDGE; $joins = array(); - if ($this->projectPHIDs || $this->includeNoProject) { - $joins[] = qsprintf( - $conn_r, - '%Q JOIN %T project ON project.src = task.phid - AND project.type = %d', - ($this->includeNoProject ? 'LEFT' : ''), - $edge_table, - PhabricatorProjectObjectHasProjectEdgeType::EDGECONST); - } - if ($this->shouldJoinBlockingTasks()) { $joins[] = qsprintf( $conn_r, @@ -788,26 +630,6 @@ id(new ManiphestTask())->getTableName()); } - if ($this->anyProjectPHIDs || $this->anyUserProjectPHIDs) { - $joins[] = qsprintf( - $conn_r, - 'JOIN %T anyproject ON anyproject.src = task.phid - AND anyproject.type = %d', - $edge_table, - PhabricatorProjectObjectHasProjectEdgeType::EDGECONST); - } - - if ($this->xprojectPHIDs) { - $joins[] = qsprintf( - $conn_r, - 'LEFT JOIN %T xproject ON xproject.src = task.phid - AND xproject.type = %d - AND xproject.dst IN (%Ls)', - $edge_table, - PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, - $this->xprojectPHIDs); - } - if ($this->subscriberPHIDs) { $joins[] = qsprintf( $conn_r, @@ -853,9 +675,7 @@ } protected function buildGroupClause(AphrontDatabaseConnection $conn_r) { - $joined_multiple_rows = (count($this->projectPHIDs) > 1) || - (count($this->anyProjectPHIDs) > 1) || - $this->shouldJoinBlockingTasks() || + $joined_multiple_rows = $this->shouldJoinBlockingTasks() || $this->shouldJoinBlockedTasks() || ($this->shouldGroupQueryResultRows()); @@ -894,35 +714,34 @@ * construction. */ private function getIgnoreGroupedProjectPHIDs() { + // Maybe we should also exclude the "OPERATOR_NOT" PHIDs? It won't + // impact the results, but we might end up with a better query plan. + // Investigate this on real data? This is likely very rare. + + $edge_types = array( + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, + ); + $phids = array(); - if ($this->projectPHIDs) { - $phids[] = $this->projectPHIDs; - } + $phids[] = $this->getEdgeLogicValues( + $edge_types, + array( + PhabricatorQueryConstraint::OPERATOR_AND, + )); - if (count($this->anyProjectPHIDs) == 1) { - $phids[] = $this->anyProjectPHIDs; + $any = $this->getEdgeLogicValues( + $edge_types, + array( + PhabricatorQueryConstraint::OPERATOR_OR, + )); + if (count($any) == 1) { + $phids[] = $any; } - // Maybe we should also exclude the "excludeProjectPHIDs"? It won't - // impact the results, but we might end up with a better query plan. - // Investigate this on real data? This is likely very rare. - return array_mergev($phids); } - // TODO: Remove this when moving fully to edge logic. - protected function buildHavingClauseParts(AphrontDatabaseConnection $conn) { - $having = parent::buildHavingClauseParts($conn); - if (count($this->projectPHIDs) > 1) { - $having[] = qsprintf( - $conn, - 'projectCount = %d', - count($this->projectPHIDs)); - } - return $having; - } - protected function getResultCursor($result) { $id = $result->getID(); diff --git a/src/applications/project/controller/PhabricatorProjectBoardViewController.php b/src/applications/project/controller/PhabricatorProjectBoardViewController.php --- a/src/applications/project/controller/PhabricatorProjectBoardViewController.php +++ b/src/applications/project/controller/PhabricatorProjectBoardViewController.php @@ -56,7 +56,6 @@ $column_query = id(new PhabricatorProjectColumnQuery()) ->setViewer($viewer) ->withProjectPHIDs(array($project->getPHID())); - if (!$show_hidden) { $column_query->withStatuses( array(PhabricatorProjectColumn::STATUS_ACTIVE)); @@ -160,7 +159,10 @@ $task_query = $engine->buildQueryFromSavedQuery($saved); $tasks = $task_query - ->addWithAllProjects(array($project->getPHID())) + ->withEdgeLogicPHIDs( + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, + PhabricatorQueryConstraint::OPERATOR_AND, + array($project->getPHID())) ->setOrderBy(ManiphestTaskQuery::ORDER_PRIORITY) ->setViewer($viewer) ->execute(); 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 @@ -22,6 +22,7 @@ private $orderVector; private $builtinOrder; private $edgeLogicConstraints = array(); + private $edgeLogicConstraintsAreValid = false; protected function getPageCursors(array $page) { return array( @@ -1261,6 +1262,26 @@ /** + * Convenience method for specifying edge logic constraints with a list of + * PHIDs. + * + * @param const Edge constant. + * @param const Constraint operator. + * @param list List of PHIDs. + * @return this + * @task edgelogic + */ + public function withEdgeLogicPHIDs($edge_type, $operator, array $phids) { + $constraints = array(); + foreach ($phids as $phid) { + $constraints[] = new PhabricatorQueryConstraint($operator, $phid); + } + + return $this->withEdgeLogicConstraints($edge_type, $constraints); + } + + + /** * @task edgelogic */ public function withEdgeLogicConstraints($edge_type, array $constraints) { @@ -1274,6 +1295,8 @@ } } + $this->edgeLogicConstraintsAreValid = false; + return $this; } @@ -1284,6 +1307,8 @@ public function buildEdgeLogicSelectClause(AphrontDatabaseConnection $conn) { $select = array(); + $this->validateEdgeLogicConstraints(); + foreach ($this->edgeLogicConstraints as $type => $constraints) { foreach ($constraints as $operator => $list) { $alias = $this->getEdgeLogicTableAlias($operator, $type); @@ -1508,4 +1533,81 @@ } + /** + * Select certain edge logic constraint values. + * + * @task edgelogic + */ + protected function getEdgeLogicValues( + array $edge_types, + array $operators) { + + $values = array(); + + $constraint_lists = $this->edgeLogicConstraints; + if ($edge_types) { + $constraint_lists = array_select_keys($constraint_lists, $edge_types); + } + + foreach ($constraint_lists as $type => $constraints) { + if ($operators) { + $constraints = array_select_keys($constraints, $operators); + } + foreach ($constraints as $operator => $list) { + foreach ($list as $constraint) { + $values[] = $constraint->getValue(); + } + } + } + + return $values; + } + + + /** + * Validate edge logic constraints for the query. + * + * @return this + * @task edgelogic + */ + private function validateEdgeLogicConstraints() { + if ($this->edgeLogicConstraintsAreValid) { + return $this; + } + + // This should probably be more modular, eventually, but we only do + // project-based edge logic today. + + $project_phids = $this->getEdgeLogicValues( + array( + PhabricatorProjectObjectHasProjectEdgeType::EDGECONST, + ), + array( + PhabricatorQueryConstraint::OPERATOR_AND, + PhabricatorQueryConstraint::OPERATOR_OR, + PhabricatorQueryConstraint::OPERATOR_NOT, + )); + if ($project_phids) { + $projects = id(new PhabricatorProjectQuery()) + ->setViewer($this->getViewer()) + ->setParentQuery($this) + ->withPHIDs($project_phids) + ->execute(); + $projects = mpull($projects, null, 'getPHID'); + foreach ($project_phids as $phid) { + if (empty($projects[$phid])) { + throw new PhabricatorEmptyQueryException( + pht( + 'This query is constrained by a project you do not have '. + 'permission to see.')); + } + } + } + + $this->edgeLogicConstraintsAreValid = true; + + return $this; + } + + }