Page MenuHomePhabricator
Files F13188990

D8879.diff
No OneTemporary
Actions

D8879.diff
View Options

diff --git a/src/applications/audit/conduit/ConduitAPI_audit_query_Method.php b/src/applications/audit/conduit/ConduitAPI_audit_query_Method.php
--- a/src/applications/audit/conduit/ConduitAPI_audit_query_Method.php
+++ b/src/applications/audit/conduit/ConduitAPI_audit_query_Method.php
@@ -51,15 +51,27 @@
DiffusionCommitQuery::AUDIT_STATUS_ANY);
$query->withAuditStatus($status);
+ // NOTE: These affect the number of commits identified, which is sort of
+ // reasonable but means the method may return an arbitrary number of
+ // actual audit requests.
$query->setOffset($request->getValue('offset', 0));
$query->setLimit($request->getValue('limit', 100));
$commits = $query->execute();
+ $auditor_map = array_fuse($auditor_phids);
+
$results = array();
foreach ($commits as $commit) {
$requests = $commit->getAudits();
foreach ($requests as $request) {
+
+ // If this audit isn't triggered for one of the requested PHIDs,
+ // skip it.
+ if ($auditor_map && empty($auditor_map[$request->getAuditorPHID()])) {
+ continue;
+ }
+
$results[] = array(
'id' => $request->getID(),
'commitPHID' => $request->getCommitPHID(),
diff --git a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
--- a/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
+++ b/src/applications/audit/management/PhabricatorAuditManagementDeleteWorkflow.php
@@ -85,7 +85,9 @@
$query->withAuditStatus($status);
}
+ $id_map = array();
if ($ids) {
+ $id_map = array_fuse($ids);
$query->withAuditIDs($ids);
}
@@ -93,8 +95,10 @@
$query->withRepositoryIDs(mpull($repos, 'getID'));
}
+ $auditor_map = array();
if ($users) {
- $query->withAuditorPHIDs(mpull($users, 'getPHID'));
+ $auditor_map = array_fuse(mpull($users, 'getPHID'));
+ $query->withAuditorPHIDs($auditor_map);
}
if ($commits) {
@@ -105,19 +109,29 @@
$commits = mpull($commits, null, 'getPHID');
$audits = array();
foreach ($commits as $commit) {
- $curr_audits = $commit->getAudits();
- foreach ($audits as $key => $audit) {
+ $commit_audits = $commit->getAudits();
+ foreach ($commit_audits as $key => $audit) {
+ if ($id_map && empty($id_map[$audit->getID()])) {
+ unset($commit_audits[$key]);
+ continue;
+ }
+
+ if ($auditor_map && empty($auditor_map[$audit->getAuditorPHID()])) {
+ unset($commit_audits[$key]);
+ continue;
+ }
+
if ($min_date && $commit->getEpoch() < $min_date) {
- unset($audits[$key]);
+ unset($commit_audits[$key]);
continue;
}
if ($max_date && $commit->getEpoch() > $max_date) {
- unset($audits[$key]);
+ unset($commit_audits[$key]);
continue;
}
}
- $audits[] = $curr_audits;
+ $audits[] = $commit_audits;
}
$audits = array_mergev($audits);
diff --git a/src/applications/diffusion/query/DiffusionCommitQuery.php b/src/applications/diffusion/query/DiffusionCommitQuery.php
--- a/src/applications/diffusion/query/DiffusionCommitQuery.php
+++ b/src/applications/diffusion/query/DiffusionCommitQuery.php
@@ -19,7 +19,6 @@
const AUDIT_STATUS_ANY = 'audit-status-any';
const AUDIT_STATUS_OPEN = 'audit-status-open';
const AUDIT_STATUS_CONCERN = 'audit-status-concern';
- private $loadAuditIds;
private $needCommitData;
@@ -94,10 +93,8 @@
* rows must always have it.
*/
private function shouldJoinAudits() {
- return
- $this->needAuditRequests ||
- $this->auditStatus ||
- $this->rowsMustHaveAudits();
+ return $this->auditStatus ||
+ $this->rowsMustHaveAudits();
}
@@ -156,31 +153,17 @@
$data = queryfx_all(
$conn_r,
- 'SELECT commit.* %Q FROM %T commit %Q %Q %Q %Q',
- $this->buildAuditSelect($conn_r),
+ 'SELECT commit.* FROM %T commit %Q %Q %Q %Q %Q',
$table->getTableName(),
$this->buildJoinClause($conn_r),
$this->buildWhereClause($conn_r),
+ $this->buildGroupClause($conn_r),
$this->buildOrderClause($conn_r),
$this->buildLimitClause($conn_r));
- if ($this->shouldJoinAudits()) {
- $this->loadAuditIds = ipull($data, 'audit_id');
- }
-
return $table->loadAllFromArray($data);
}
- private function buildAuditSelect($conn_r) {
- if ($this->shouldJoinAudits()) {
- return qsprintf(
- $conn_r,
- ', audit.id as audit_id');
- }
-
- return '';
- }
-
protected function willFilterPage(array $commits) {
$repository_ids = mpull($commits, 'getRepositoryID', 'getRepositoryID');
$repos = id(new PhabricatorRepositoryQuery())
@@ -230,7 +213,7 @@
$result[$identifier] = head($matching_commits);
} else {
// This reference is ambiguous (it matches more than one commit) so
- // don't link it
+ // don't link it.
unset($result[$identifier]);
}
}
@@ -257,14 +240,13 @@
}
}
- if ($this->shouldJoinAudits()) {
- $load_ids = array_filter($this->loadAuditIds);
- if ($load_ids) {
- $requests = id(new PhabricatorRepositoryAuditRequest())
- ->loadAllWhere('id IN (%Ld)', $this->loadAuditIds);
- } else {
- $requests = array();
- }
+ // TODO: This should just be `needAuditRequests`, not `shouldJoinAudits()`,
+ // but leave that for a future diff.
+
+ if ($this->needAuditRequests || $this->shouldJoinAudits()) {
+ $requests = id(new PhabricatorRepositoryAuditRequest())->loadAllWhere(
+ 'commitPHID IN (%Ls)',
+ mpull($commits, 'getPHID'));
$requests = mgroup($requests, 'getCommitPHID');
foreach ($commits as $commit) {
@@ -508,6 +490,23 @@
}
}
+ private function buildGroupClause(AphrontDatabaseConnection $conn_r) {
+ $should_group = $this->shouldJoinAudits();
+
+ // TODO: Currently, the audit table is missing a unique key, so we may
+ // require a GROUP BY if we perform this join. See T1768. This can be
+ // removed once the table has the key.
+ if ($this->auditAwaitingUser) {
+ $should_group = true;
+ }
+
+ if ($should_group) {
+ return 'GROUP BY commit.id';
+ } else {
+ return '';
+ }
+ }
+
public function getQueryApplicationClass() {
return 'PhabricatorApplicationDiffusion';
}

File Metadata

Mime Type
text/plain
Expires
Sun, May 12, 5:39 AM (3 w, 23 h ago)
Storage Engine
blob
Storage Format
Encrypted (AES-256-CBC)
Storage Handle
6274929
Default Alt Text
D8879.diff (6 KB)

Event Timeline

Phabricator · Built by Phacility