Add GROUP BY to commit query
ClosedPublic
Actions

Authored by epriestley on Apr 28 2014, 12:30 AM.

Details

Reviewers

btrahan

Maniphest Tasks

T5433: Audit shows 143 Needs Attention, only 50 listed (no pager)
T4715: Move Audit to ApplicationSearch

Commits

Restricted Diffusion Commit
rP16648c28bcd7: Add GROUP BY to commit query

Summary

Ref T4715. Some minor stuff I caught locally while poking around:

Since we don't GROUP BY, we can still get duplicate commits. These get silently de-duplicated by loadAllFromArray() because that returns an array keyed by id, but we fetch too much data and this can cause us to execute too many queries to fill pages. Instead, GROUP BY if we joined the audit table.
After adding GROUP BY, getting the audit IDs out of the query is no longer reliable. Instead, query audits by the commit PHIDs. This is approximately equiavlent.
Since we always JOIN, we currently never return commits that don't have any audits. If we don't know that all results will have an audit, just LEFT JOIN.
Add some !== null to catch the withIDs(array()) issue that we hit with Khan Academy a little while ago.

Test Plan

Verified that "All Commits" shows commits with no audits of any kind.
Verified that the raw data comes out of the query without duplicates.

Diff Detail

Repository

rP Phabricator

Lint

Lint Skipped

Unit

Tests Skipped

Event Timeline

epriestley updated this revision to Diff 21061.Apr 28 2014, 12:30 AM

epriestley retitled this revision from to Minor tweaks to audit queries.

epriestley updated this object.

epriestley edited the test plan for this revision. (Show Details)

epriestley added a reviewer: btrahan.

epriestley added a task: T4715: Move Audit to ApplicationSearch.

Herald added a subscriber: epriestley. · View Herald TranscriptApr 28 2014, 12:30 AM

Harbormaster completed remote builds in B94: Diff 21061.Apr 28 2014, 12:31 AM

I should have commented the duplicate row thing, as that was intentional. I felt clever even. :D

I think when you query *all* audits by commitPHID you over load audit data (potentially) based on what you query. Something like ConduitAPI_audit_query_Method would get wonky with this change I think.

Basically, once you start querying by audit data, I think you need to do it like it was before (to get accurate audit data) OR do it like it is now AND add some sort of audit filtering stuff to get the audits that match the audit specific query parameter. A third approach would be to make the change as you have it but see if callsites like the audit conduit method can be otherwise tweaked or their use cases served otherwise.

Something like ConduitAPI_audit_query_Method would get wonky with this change I think.

Ah! Good point.

A third approach would be to make the change as you have it but see if callsites like the audit conduit method can be otherwise tweaked or their use cases served otherwise.

I think this is the right approach -- do you remember anything other than audit.query offhand? I can poke around.

For audit.query itself, we don't have any audit.query callsites in the codebase and I don't remember why we have the call at all. I can't think of anything you'd want to do with it that wouldn't be better served by exposing these filters on diffusion.querycommits, but can do a bit of digging.

I'm going to split this into the LEFT JOIN/JOIN and GROUP BY changes separately, since the LEFT JOIN doesn't cause issues and fixes T4911.

audit.query and bin/audit are the two I remember off hand.

(bin/audit tries lets you delete audits by ID. this would return some N audits right now, with 1 of the N having the right ID and the other N-1 just happening to be on the same commit as the one with the right ID.)

Fixes T5433.

Use GROUP BY.
I grepped for all the getAudits() callsites, and only audit.query and bin/audit delete used them in a way that expected a subset to be returned.
In both cases, I just manually filtered results in the caller. (We can probably deprecate audit.query, maybe.)

Test plan:

Paged through commits with multiple audits.
Used audit.query with various parameters.
Used bin/audit delete with various arguments.

Harbormaster completed remote builds in B1292: Diff 23187.Jun 22 2014, 4:25 PM

epriestley added inline comments.Jun 22 2014, 4:28 PM

src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
112–124	This fixed an existing bug -- we were iterating over `$audits`, but should have iterated over `$curr_audits`. Mostly, this just made the script take a really long time to run.