From 509fbb6c20e24374506196ffa14bed143f0bc88f Mon Sep 17 00:00:00 2001
From: epriestley <git@epriestley.com>
Date: Thu, 7 Feb 2019 11:44:48 -0800
Subject: [PATCH] When building audit queries, prefilter possible "authorPHID"
 values

Summary:
Ref T13244. See PHI1057. Currently, if you're a member of a lot of projects/packages, you can end up with a very large `commit.authorPHID IN (...)` clause in part of the "Active Audits" query, since your `alice` token in "Responsible Users: alice" expands into every package and project you can audit on behalf of.

It's impossible for a commit to be authored by anything but a user, and evidence in PHI1057 suggests this giant `IN (...)` list can prevent MySQL from making effective utilization of the `<authorPHID, auditStatus, ...>` key on the table.

Prefilter the list of PHIDs to only PHIDs which can possibly author a commit.

(We'll also eventually need to convert the `authorPHIDs` into `identityPHIDs` anyway, for T12164, and this moves us slightly toward that.)

Test Plan: Loaded "Active Audits" before and after change, saw a more streamlined and sensible `authorPHID IN (...)` clause afterwards.

Reviewers: amckinley

Reviewed By: amckinley

Maniphest Tasks: T13244

Differential Revision: https://secure.phabricator.com/D20129
---
 .../diffusion/query/DiffusionCommitQuery.php  | 51 ++++++++++++++++---
 1 file changed, 44 insertions(+), 7 deletions(-)

diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php
index 05072e07c7..03eb0b9edf 100644
--- a/src/applications/diffusion/query/DiffusionCommitQuery.php
+++ b/src/applications/diffusion/query/DiffusionCommitQuery.php
@@ -202,6 +202,7 @@ final class DiffusionCommitQuery
     $table = $this->newResultObject();
     $conn = $table->establishConnection('r');
 
+    $empty_exception = null;
     $subqueries = array();
     if ($this->responsiblePHIDs) {
       $base_authors = $this->authorPHIDs;
@@ -222,21 +223,33 @@ final class DiffusionCommitQuery
 
       $this->authorPHIDs = $all_authors;
       $this->auditorPHIDs = $base_auditors;
-      $subqueries[] = $this->buildStandardPageQuery(
-        $conn,
-        $table->getTableName());
+      try {
+        $subqueries[] = $this->buildStandardPageQuery(
+          $conn,
+          $table->getTableName());
+      } catch (PhabricatorEmptyQueryException $ex) {
+        $empty_exception = $ex;
+      }
 
       $this->authorPHIDs = $base_authors;
       $this->auditorPHIDs = $all_auditors;
-      $subqueries[] = $this->buildStandardPageQuery(
-        $conn,
-        $table->getTableName());
+      try {
+        $subqueries[] = $this->buildStandardPageQuery(
+          $conn,
+          $table->getTableName());
+      } catch (PhabricatorEmptyQueryException $ex) {
+        $empty_exception = $ex;
+      }
     } else {
       $subqueries[] = $this->buildStandardPageQuery(
         $conn,
         $table->getTableName());
     }
 
+    if (!$subqueries) {
+      throw $empty_exception;
+    }
+
     if (count($subqueries) > 1) {
       $unions = null;
       foreach ($subqueries as $subquery) {
@@ -642,10 +655,19 @@ final class DiffusionCommitQuery
     }
 
     if ($this->authorPHIDs !== null) {
+      $author_phids = $this->authorPHIDs;
+      if ($author_phids) {
+        $author_phids = $this->selectPossibleAuthors($author_phids);
+        if (!$author_phids) {
+          throw new PhabricatorEmptyQueryException(
+            pht('Author PHIDs contain no possible authors.'));
+        }
+      }
+
       $where[] = qsprintf(
         $conn,
         'commit.authorPHID IN (%Ls)',
-        $this->authorPHIDs);
+        $author_phids);
     }
 
     if ($this->epochMin !== null) {
@@ -934,5 +956,20 @@ final class DiffusionCommitQuery
     ) + $parent;
   }
 
+  private function selectPossibleAuthors(array $phids) {
+    // See PHI1057. Select PHIDs which might possibly be commit authors from
+    // a larger list of PHIDs. This primarily filters out packages and projects
+    // from "Responsible Users: ..." queries. Our goal in performing this
+    // filtering is to improve the performance of the final query.
+
+    foreach ($phids as $key => $phid) {
+      if (phid_get_type($phid) !== PhabricatorPeopleUserPHIDType::TYPECONST) {
+        unset($phids[$key]);
+      }
+    }
+
+    return $phids;
+  }
+
 
 }