From 671986592bafc4b2f059319d9e320cab46445ac2 Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Fri, 30 Oct 2020 12:34:19 -0700
Subject: [PATCH] Add a missing "GROUP BY" to MailQuery when querying for
 multiple recipients

Summary:
See <https://discourse.phabricator-community.org/t/mail-details-view-broken/4315>. The change in D21400 detects a missing "GROUP BY" in some variations of this query.

Specifically, we may join multiple recipient rows (since mail may have multiple recipients) and then fail to group the results.

Fix this by adding the "GROUP BY". Additionally, remove the special-cased behavior when no authors or recipients are specified -- it's complicated and not entirely correct (e.g., may produce a "no object" instead of a policy error when querying by ID), and likely predates overheating.

Test Plan:
  - Disabled `metamta.one-mail-per-recipient` in Config.
  - Generated a message to 2+ recipients.
  - Viewed the message detail; queried for the message by specifying 2+ recipients.
  - Viewed the unfiltered list of messages, saw the query overheat.

Differential Revision: https://secure.phabricator.com/D21486
---
 .../query/PhabricatorMetaMTAMailQuery.php     | 55 +++++++++----------
 .../policy/PhabricatorPolicyAwareQuery.php    |  2 +-
 2 files changed, 26 insertions(+), 31 deletions(-)

diff --git a/src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php b/src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php
index a1fad69c0b..903b385ceb 100644
--- a/src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php
+++ b/src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php
@@ -64,24 +64,6 @@ final class PhabricatorMetaMTAMailQuery
         $this->actorPHIDs);
     }
 
-    if ($this->recipientPHIDs !== null) {
-      $where[] = qsprintf(
-        $conn,
-        'recipient.dst IN (%Ls)',
-        $this->recipientPHIDs);
-    }
-
-    if ($this->actorPHIDs === null && $this->recipientPHIDs === null) {
-      $viewer = $this->getViewer();
-      if (!$viewer->isOmnipotent()) {
-        $where[] = qsprintf(
-          $conn,
-          'edge.dst = %s OR actorPHID = %s',
-          $viewer->getPHID(),
-          $viewer->getPHID());
-      }
-    }
-
     if ($this->createdMin !== null) {
       $where[] = qsprintf(
         $conn,
@@ -102,26 +84,29 @@ final class PhabricatorMetaMTAMailQuery
   protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
     $joins = parent::buildJoinClauseParts($conn);
 
-    if ($this->actorPHIDs === null && $this->recipientPHIDs === null) {
+    if ($this->shouldJoinRecipients()) {
       $joins[] = qsprintf(
         $conn,
-        'LEFT JOIN %T edge ON mail.phid = edge.src AND edge.type = %d',
+        'JOIN %T recipient
+          ON mail.phid = recipient.src
+            AND recipient.type = %d
+            AND recipient.dst IN (%Ls)',
         PhabricatorEdgeConfig::TABLE_NAME_EDGE,
-        PhabricatorMetaMTAMailHasRecipientEdgeType::EDGECONST);
-    }
-
-    if ($this->recipientPHIDs !== null) {
-      $joins[] = qsprintf(
-        $conn,
-        'LEFT JOIN %T recipient '.
-        'ON mail.phid = recipient.src AND recipient.type = %d',
-        PhabricatorEdgeConfig::TABLE_NAME_EDGE,
-        PhabricatorMetaMTAMailHasRecipientEdgeType::EDGECONST);
+        PhabricatorMetaMTAMailHasRecipientEdgeType::EDGECONST,
+        $this->recipientPHIDs);
     }
 
     return $joins;
   }
 
+  private function shouldJoinRecipients() {
+    if ($this->recipientPHIDs === null) {
+      return false;
+    }
+
+    return true;
+  }
+
   protected function getPrimaryTableAlias() {
     return 'mail';
   }
@@ -134,4 +119,14 @@ final class PhabricatorMetaMTAMailQuery
     return 'PhabricatorMetaMTAApplication';
   }
 
+  protected function shouldGroupQueryResultRows() {
+    if ($this->shouldJoinRecipients()) {
+      if (count($this->recipientPHIDs) > 1) {
+        return true;
+      }
+    }
+
+    return parent::shouldGroupQueryResultRows();
+  }
+
 }
diff --git a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
index a770c326f9..c43edaefcb 100644
--- a/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
+++ b/src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
@@ -234,7 +234,7 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery {
     // T11773 for some discussion.
     $this->isOverheated = false;
 
-    // See T13386. If we on an old offset-based paging workflow, we need
+    // See T13386. If we are on an old offset-based paging workflow, we need
     // to base the overheating limit on both the offset and limit.
     $overheat_limit = $need * 10;
     $total_seen = 0;