DifferentialRevisionQuery: avoid nonsense query comparing r.authorPHID with group PHIDs
Open, LowPublic
Actions

Description

This bug was originally reported by the kind user jmeador proposing D25676. Here an in-depth description of the problem, to understand the possible solution.

Steps to reproduce:

visit Differentials
search by Responsible Users = Group

Preamble: have DarkConsole enabled in your personal Developer Settings, so you can inspect queries.

What happens:

The resulting query against differential_revision has this nonsense condition (or very similar):

r.authorPHID IN ('PHID-PROJ-plufiz5pclartkuv5ggz')

Just for the records, this is the full query:

(SELECT `r`.* FROM `differential_revision` r  WHERE (r.authorPHID IN ('PHID-PROJ-plufiz5pclartkuv5ggz'))   ORDER BY `r`.`id`
DESC LIMIT 101) UNION DISTINCT (SELECT `r`.* FROM `differential_revision` r LEFT JOIN `differential_reviewer` reviewer ON
reviewer.revisionPHID = r.phid
          AND reviewer.reviewerStatus != 'resigned'
          AND reviewer.reviewerPHID in ('PHID-PROJ-plufiz5pclartkuv5ggz') WHERE ((reviewer.reviewerPHID IS NOT NULL))   ORDER BY
`r`.`id` DESC LIMIT 101) ORDER BY `id` DESC LIMIT 101

Let's beautify it a bit:

(
  SELECT 
    `r`.* 
  FROM 
    `differential_revision` r 
  WHERE 
    (
      r.authorPHID IN (               -- ← ← ← ← ← ← ← NONSENSE 🫠 ASDLOL 🫠
        'PHID-PROJ-plufiz5pclartkuv5ggz'
      )
    ) 
  ORDER BY 
    `r`.`id` DESC 
  LIMIT 
    101
) 
UNION 
  DISTINCT (
    SELECT 
      `r`.* 
    FROM 
      `differential_revision` r 
      LEFT JOIN `differential_reviewer` reviewer ON reviewer.revisionPHID = r.phid 
      AND reviewer.reviewerStatus != 'resigned' 
      AND reviewer.reviewerPHID in (
        'PHID-PROJ-plufiz5pclartkuv5ggz'
      ) 
    WHERE 
      (
        (reviewer.reviewerPHID IS NOT NULL)
      ) 
    ORDER BY 
      `r`.`id` DESC 
    LIMIT 
      101
  ) 
ORDER BY 
  `id` DESC 
LIMIT 
  101

The reported condition in the first UNION has no sense since a PHID-PROJ cannot be compared with an authorPHID so that first selection is always false, so it seems it sends in short-circuit the UNION that always returns the result of the second query. Fortunately, that first nonsense query is very efficient because we have a database index on that column r.authorPHID so it's not a big trouble, so, probably low priority.

Proposed solution:

It would be more efficient to just avoid that nonsense first query in the union that returns no results. So, we can avoid an UNION at all, and just run the second query, to return only what we want with the current filters.