From 709c304d76d16e3d5585dd31f291a41b46883494 Mon Sep 17 00:00:00 2001 From: epriestley Date: Wed, 24 May 2017 11:47:40 -0700 Subject: [PATCH] Group query results under the "ANCESTOR" operator unconditionally Summary: Fixes T12753. See that task for reproduction instructions. We add a `GROUP BY` clause to queries with an "ANCESTOR" edge constraint only if the constaint has more than one PHID, but this is incorrect: the same row can be found twice by an ANCESTOR query if task T is tagged with both "B" and "C", children of "A", and the user queries for "tasks in A". Instead, always add GROUP BY for ANCESTOR queries. Test Plan: - Followed test plan in T12753. - Saw proper paging controls after change. - Saw `GROUP BY` in DarkConsole. Reviewers: chad Reviewed By: chad Maniphest Tasks: T12753 Differential Revision: https://secure.phabricator.com/D18012 --- .../policy/PhabricatorCursorPagedPolicyAwareQuery.php | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php index 2bd374d5db..3ef2a72e6f 100644 --- a/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php +++ b/src/infrastructure/query/policy/PhabricatorCursorPagedPolicyAwareQuery.php @@ -1811,11 +1811,16 @@ 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; } break; + case PhabricatorQueryConstraint::OPERATOR_ANCESTOR: + // NOTE: We must always group query results rows when using an + // "ANCESTOR" operator because a single task may be related to + // two different descendants of a particular ancestor. For + // discussion, see T12753. + return true; case PhabricatorQueryConstraint::OPERATOR_NULL: return true; }