revi-archive/phorge-phorge
1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-29 10:12:41 +01:00
Code Issues Releases Wiki Activity

Add a missing "GROUP BY" to MailQuery when querying for multiple recipients

Browse source 
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
This commit is contained in:
epriestley 2020-10-30 12:34:19 -07:00
parent bc4f86d279
commit 671986592b
2 changed files with 26 additions and 31 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

55
src/applications/metamta/query/PhabricatorMetaMTAMailQuery.php
View file

@ -64,24 +64,6 @@ final class PhabricatorMetaMTAMailQuery
$this->actorPHIDs); $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) { if ($this->createdMin !== null) {
$where[] = qsprintf( $where[] = qsprintf(
$conn, $conn,
@ -102,26 +84,29 @@ final class PhabricatorMetaMTAMailQuery
protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) { protected function buildJoinClauseParts(AphrontDatabaseConnection $conn) {
$joins = parent::buildJoinClauseParts($conn); $joins = parent::buildJoinClauseParts($conn);
if ($this->actorPHIDs === null && $this->recipientPHIDs === null) { if ($this->shouldJoinRecipients()) {
$joins[] = qsprintf( $joins[] = qsprintf(
$conn, $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, PhabricatorEdgeConfig::TABLE_NAME_EDGE,
PhabricatorMetaMTAMailHasRecipientEdgeType::EDGECONST); PhabricatorMetaMTAMailHasRecipientEdgeType::EDGECONST,
} $this->recipientPHIDs);
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);
} }
return $joins; return $joins;
} }
private function shouldJoinRecipients() {
if ($this->recipientPHIDs === null) {
return false;
}
return true;
}
protected function getPrimaryTableAlias() { protected function getPrimaryTableAlias() {
return 'mail'; return 'mail';
} }
@ -134,4 +119,14 @@ final class PhabricatorMetaMTAMailQuery
return 'PhabricatorMetaMTAApplication'; return 'PhabricatorMetaMTAApplication';
} }
protected function shouldGroupQueryResultRows() {
if ($this->shouldJoinRecipients()) {
if (count($this->recipientPHIDs) > 1) {
return true;
}
}
return parent::shouldGroupQueryResultRows();
}
} }

2
src/infrastructure/query/policy/PhabricatorPolicyAwareQuery.php
View file

@ -234,7 +234,7 @@ abstract class PhabricatorPolicyAwareQuery extends PhabricatorOffsetPagedQuery {
// T11773 for some discussion. // T11773 for some discussion.
$this->isOverheated = false; $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. // to base the overheating limit on both the offset and limit.
$overheat_limit = $need * 10; $overheat_limit = $need * 10;
$total_seen = 0; $total_seen = 0;