Provide a real object ("PhabricatorRepositoryPushEvent") to represent an entire push transaction

Summary:
Ref T4677. Currently, we record individual actions in a push as PhabricatorRepositoryPushLogs, but tie them together only loosely with a `transactionKey`.

Provide a real PushEvent object, and move some of the denormalized fields to it. This primarily just gives us more robust infrastructure for building, e.g., email about pushes, for T4677, since we can act on real PHIDs rather than passing awkward identifiers around.

Test Plan:
  - Performed migration.
  - Looked at database for consistency.
  - Browsed/queried push logs.
  - Pushed a bunch of stuff.

Reviewers: btrahan

Reviewed By: btrahan

Subscribers: epriestley

Maniphest Tasks: T4677

Differential Revision: https://secure.phabricator.com/D8615

resources/sql/autopatches/20140325.push.1.event.sql

@@ -0,0 +1,15 @@
+CREATE TABLE {$NAMESPACE}_repository.repository_pushevent (
+  id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY,
+  phid VARCHAR(64) NOT NULL COLLATE utf8_bin,
+  repositoryPHID  VARCHAR(64) NOT NULL COLLATE utf8_bin,
+  epoch INT UNSIGNED NOT NULL,
+  pusherPHID VARCHAR(64) NOT NULL COLLATE utf8_bin,
+  remoteAddress INT UNSIGNED,
+  remoteProtocol VARCHAR(32),
+  rejectCode INT UNSIGNED NOT NULL,
+  rejectDetails VARCHAR(64) COLLATE utf8_bin,
+
+  UNIQUE KEY `key_phid` (phid),
+  KEY `key_repository` (repositoryPHID)
+
+) ENGINE=InnoDB, COLLATE=utf8_general_ci;

resources/sql/autopatches/20140325.push.2.eventphid.sql

@@ -0,0 +1,5 @@
+ALTER TABLE {$NAMESPACE}_repository.repository_pushlog
+  ADD pushEventPHID VARCHAR(64) NOT NULL COLLATE utf8_bin AFTER epoch;
+
+ALTER TABLE {$NAMESPACE}_repository.repository_pushlog
+  ADD KEY `key_event` (pushEventPHID);

resources/sql/autopatches/20140325.push.3.groups.php

@@ -0,0 +1,43 @@
+<?php
+
+$conn_w = id(new PhabricatorRepository())->establishConnection('w');
+
+echo "Adding transaction log event groups...\n";
+
+$logs = queryfx_all(
+  $conn_w,
+  'SELECT * FROM %T GROUP BY transactionKey ORDER BY id ASC',
+  'repository_pushlog');
+foreach ($logs as $log) {
+  $id = $log['id'];
+  echo "Migrating log {$id}...\n";
+  if ($log['pushEventPHID']) {
+    continue;
+  }
+
+  $event_phid = id(new PhabricatorRepositoryPushEvent())->generatePHID();
+
+  queryfx(
+    $conn_w,
+    'INSERT INTO %T (phid, repositoryPHID, epoch, pusherPHID, remoteAddress,
+      remoteProtocol, rejectCode, rejectDetails)
+     VALUES (%s, %s, %d, %s, %d, %s, %d, %s)',
+    'repository_pushevent',
+    $event_phid,
+    $log['repositoryPHID'],
+    $log['epoch'],
+    $log['pusherPHID'],
+    $log['remoteAddress'],
+    $log['remoteProtocol'],
+    $log['rejectCode'],
+    $log['rejectDetails']);
+
+  queryfx(
+    $conn_w,
+    'UPDATE %T SET pushEventPHID = %s WHERE transactionKey = %s',
+    'repository_pushlog',
+    $event_phid,
+    $log['transactionKey']);
+}
+
+echo "Done.\n";

resources/sql/autopatches/20140325.push.4.prune.sql

@@ -0,0 +1,14 @@
+ALTER TABLE {$NAMESPACE}_repository.repository_pushlog
+  DROP remoteAddress;
+
+ALTER TABLE {$NAMESPACE}_repository.repository_pushlog
+  DROP remoteProtocol;
+
+ALTER TABLE {$NAMESPACE}_repository.repository_pushlog
+  DROP transactionKey;
+
+ALTER TABLE {$NAMESPACE}_repository.repository_pushlog
+  DROP rejectCode;
+
+ALTER TABLE {$NAMESPACE}_repository.repository_pushlog
+  DROP rejectDetails;

src/__phutil_library_map__.php

@@ -1958,11 +1958,14 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryPHIDTypeArcanistProject' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypeArcanistProject.php',
     'PhabricatorRepositoryPHIDTypeCommit' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypeCommit.php',
     'PhabricatorRepositoryPHIDTypeMirror' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypeMirror.php',
+    'PhabricatorRepositoryPHIDTypePushEvent' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypePushEvent.php',
     'PhabricatorRepositoryPHIDTypePushLog' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypePushLog.php',
     'PhabricatorRepositoryPHIDTypeRepository' => 'applications/repository/phid/PhabricatorRepositoryPHIDTypeRepository.php',
     'PhabricatorRepositoryParsedChange' => 'applications/repository/data/PhabricatorRepositoryParsedChange.php',
     'PhabricatorRepositoryPullEngine' => 'applications/repository/engine/PhabricatorRepositoryPullEngine.php',
     'PhabricatorRepositoryPullLocalDaemon' => 'applications/repository/daemon/PhabricatorRepositoryPullLocalDaemon.php',
+    'PhabricatorRepositoryPushEvent' => 'applications/repository/storage/PhabricatorRepositoryPushEvent.php',
+    'PhabricatorRepositoryPushEventQuery' => 'applications/repository/query/PhabricatorRepositoryPushEventQuery.php',
     'PhabricatorRepositoryPushLog' => 'applications/repository/storage/PhabricatorRepositoryPushLog.php',
     'PhabricatorRepositoryPushLogQuery' => 'applications/repository/query/PhabricatorRepositoryPushLogQuery.php',
     'PhabricatorRepositoryPushLogSearchEngine' => 'applications/repository/query/PhabricatorRepositoryPushLogSearchEngine.php',
@@ -4792,11 +4795,18 @@ phutil_register_library_map(array(
     'PhabricatorRepositoryPHIDTypeArcanistProject' => 'PhabricatorPHIDType',
     'PhabricatorRepositoryPHIDTypeCommit' => 'PhabricatorPHIDType',
     'PhabricatorRepositoryPHIDTypeMirror' => 'PhabricatorPHIDType',
+    'PhabricatorRepositoryPHIDTypePushEvent' => 'PhabricatorPHIDType',
     'PhabricatorRepositoryPHIDTypePushLog' => 'PhabricatorPHIDType',
     'PhabricatorRepositoryPHIDTypeRepository' => 'PhabricatorPHIDType',
     'PhabricatorRepositoryParsedChange' => 'Phobject',
     'PhabricatorRepositoryPullEngine' => 'PhabricatorRepositoryEngine',
     'PhabricatorRepositoryPullLocalDaemon' => 'PhabricatorDaemon',
+    'PhabricatorRepositoryPushEvent' =>
+    array(
+      0 => 'PhabricatorRepositoryDAO',
+      1 => 'PhabricatorPolicyInterface',
+    ),
+    'PhabricatorRepositoryPushEventQuery' => 'PhabricatorCursorPagedPolicyAwareQuery',
     'PhabricatorRepositoryPushLog' =>
     array(
       0 => 'PhabricatorRepositoryDAO',

src/applications/diffusion/controller/DiffusionPushLogListController.php

@@ -52,7 +52,7 @@ final class DiffusionPushLogListController extends DiffusionController
       // Reveal this if it's valid and the user can edit the repository.
       $remote_addr = '-';
       if (isset($editable_repos[$log->getRepositoryPHID()])) {
-        $remote_long = $log->getRemoteAddress();
+        $remote_long = $log->getPushEvent()->getRemoteAddress();
         if ($remote_long) {
           $remote_addr = long2ip($remote_long);
         }
@@ -60,6 +60,7 @@ final class DiffusionPushLogListController extends DiffusionController
 
       $callsign = $log->getRepository()->getCallsign();
       $rows[] = array(
+        $log->getPushEvent()->getID(),
         phutil_tag(
           'a',
           array(
@@ -68,7 +69,7 @@ final class DiffusionPushLogListController extends DiffusionController
           $callsign),
         $this->getHandle($log->getPusherPHID())->renderLink(),
         $remote_addr,
-        $log->getRemoteProtocol(),
+        $log->getPushEvent()->getRemoteProtocol(),
         $log->getRefType(),
         $log->getRefName(),
         phutil_tag(
@@ -86,7 +87,7 @@ final class DiffusionPushLogListController extends DiffusionController
 
         // TODO: Make these human-readable.
         $log->getChangeFlags(),
-        $log->getRejectCode(),
+        $log->getPushEvent()->getRejectCode(),
         phabricator_datetime($log->getEpoch(), $viewer),
       );
     }
@@ -94,6 +95,7 @@ final class DiffusionPushLogListController extends DiffusionController
     $table = id(new AphrontTableView($rows))
       ->setHeaders(
         array(
+          pht('Push'),
           pht('Repository'),
           pht('Pusher'),
           pht('From'),
@@ -113,6 +115,7 @@ final class DiffusionPushLogListController extends DiffusionController
           '',
           '',
           '',
+          '',
           'wide',
           'n',
           'n',

src/applications/diffusion/engine/DiffusionCommitHookEngine.php

@@ -30,6 +30,8 @@ final class DiffusionCommitHookEngine extends Phobject {
   private $gitCommits = array();
 
   private $heraldViewerProjects;
+  private $rejectCode = PhabricatorRepositoryPushLog::REJECT_BROKEN;
+  private $rejectDetails;
 
 
 /* -(  Config  )------------------------------------------------------------- */
@@ -62,14 +64,6 @@ final class DiffusionCommitHookEngine extends Phobject {
     return $remote_address;
   }
 
-  private function getTransactionKey() {
-    if (!$this->transactionKey) {
-      $entropy = Filesystem::readRandomBytes(64);
-      $this->transactionKey = PhabricatorHash::digestForIndex($entropy);
-    }
-    return $this->transactionKey;
-  }
-
   public function setSubversionTransactionInfo($transaction, $repository) {
     $this->subversionTransaction = $transaction;
     $this->subversionRepository = $repository;
@@ -137,10 +131,7 @@ final class DiffusionCommitHookEngine extends Phobject {
       } catch (DiffusionCommitHookRejectException $ex) {
         // If we're rejecting dangerous changes, flag everything that we've
         // seen as rejected so it's clear that none of it was accepted.
-        foreach ($all_updates as $update) {
-          $update->setRejectCode(
-            PhabricatorRepositoryPushLog::REJECT_DANGEROUS);
-        }
+        $this->rejectCode = PhabricatorRepositoryPushLog::REJECT_DANGEROUS;
         throw $ex;
       }
 
@@ -156,9 +147,7 @@ final class DiffusionCommitHookEngine extends Phobject {
 
       // If we make it this far, we're accepting these changes. Mark all the
       // logs as accepted.
-      foreach ($all_updates as $update) {
-        $update->setRejectCode(PhabricatorRepositoryPushLog::REJECT_ACCEPT);
-      }
+      $this->rejectCode = PhabricatorRepositoryPushLog::REJECT_ACCEPT;
     } catch (Exception $ex) {
       // We'll throw this again in a minute, but we want to save all the logs
       // first.
@@ -166,9 +155,18 @@ final class DiffusionCommitHookEngine extends Phobject {
     }
 
     // Save all the logs no matter what the outcome was.
-    foreach ($all_updates as $update) {
-      $update->save();
-    }
+    $event = $this->newPushEvent();
+
+    $event->setRejectCode($this->rejectCode);
+    $event->setRejectDetails($this->rejectDetails);
+
+    $event->openTransaction();
+      $event->save();
+      foreach ($all_updates as $update) {
+        $update->setPushEventPHID($event->getPHID());
+        $update->save();
+      }
+    $event->saveTransaction();
 
     if ($caught) {
       throw $caught;
@@ -296,10 +294,8 @@ final class DiffusionCommitHookEngine extends Phobject {
     }
 
     if ($blocking_effect) {
-      foreach ($all_updates as $update) {
-        $update->setRejectCode(PhabricatorRepositoryPushLog::REJECT_HERALD);
-        $update->setRejectDetails($blocking_effect->getRulePHID());
-      }
+      $this->rejectCode = PhabricatorRepositoryPushLog::REJECT_HERALD;
+      $this->rejectDetails = $blocking_effect->getRulePHID();
 
       $message = $blocking_effect->getTarget();
       if (!strlen($message)) {
@@ -596,12 +592,8 @@ final class DiffusionCommitHookEngine extends Phobject {
           continue;
         }
 
-        // Mark everything as rejected by this hook.
-        foreach ($updates as $update) {
-          $update->setRejectCode(
-            PhabricatorRepositoryPushLog::REJECT_EXTERNAL);
-          $update->setRejectDetails(basename($hook));
-        }
+        $this->rejectCode = PhabricatorRepositoryPushLog::REJECT_EXTERNAL;
+        $this->rejectDetails = basename($hook);
 
         throw new DiffusionCommitHookRejectException(
           pht(
@@ -983,24 +975,23 @@ final class DiffusionCommitHookEngine extends Phobject {
 
 
   private function newPushLog() {
-    // NOTE: By default, we create these with REJECT_BROKEN as the reject
-    // code. This indicates a broken hook, and covers the case where we
-    // encounter some unexpected exception and consequently reject the changes.
-
     // NOTE: We generate PHIDs up front so the Herald transcripts can pick them
     // up.
     $phid = id(new PhabricatorRepositoryPushLog())->generatePHID();
 
     return PhabricatorRepositoryPushLog::initializeNewLog($this->getViewer())
       ->setPHID($phid)
-      ->attachRepository($this->getRepository())
       ->setRepositoryPHID($this->getRepository()->getPHID())
-      ->setEpoch(time())
+      ->setEpoch(time());
+  }
+
+  private function newPushEvent() {
+    $viewer = $this->getViewer();
+    return PhabricatorRepositoryPushEvent::initializeNewEvent($viewer)
+      ->setRepositoryPHID($this->getRepository()->getPHID())
       ->setRemoteAddress($this->getRemoteAddressForLog())
       ->setRemoteProtocol($this->getRemoteProtocol())
-      ->setTransactionKey($this->getTransactionKey())
-      ->setRejectCode(PhabricatorRepositoryPushLog::REJECT_BROKEN)
-      ->setRejectDetails(null);
+      ->setEpoch(time());
   }
 
   public function loadChangesetsForCommit($identifier) {

src/applications/repository/phid/PhabricatorRepositoryPHIDTypePushEvent.php

@@ -0,0 +1,40 @@
+<?php
+
+final class PhabricatorRepositoryPHIDTypePushEvent
+  extends PhabricatorPHIDType {
+
+  const TYPECONST = 'PSHE';
+
+  public function getTypeConstant() {
+    return self::TYPECONST;
+  }
+
+  public function getTypeName() {
+    return pht('Push Event');
+  }
+
+  public function newObject() {
+    return new PhabricatorRepositoryPushEvent();
+  }
+
+  protected function buildQueryForObjects(
+    PhabricatorObjectQuery $query,
+    array $phids) {
+
+    return id(new PhabricatorRepositoryPushEventQuery())
+      ->withPHIDs($phids);
+  }
+
+  public function loadHandles(
+    PhabricatorHandleQuery $query,
+    array $handles,
+    array $objects) {
+
+    foreach ($handles as $phid => $handle) {
+      $event = $objects[$phid];
+
+      $handle->setName(pht('Push Event %d', $event->getID()));
+    }
+  }
+
+}

src/applications/repository/query/PhabricatorRepositoryPushEventQuery.php

@@ -0,0 +1,108 @@
+<?php
+
+final class PhabricatorRepositoryPushEventQuery
+  extends PhabricatorCursorPagedPolicyAwareQuery {
+
+  private $ids;
+  private $phids;
+  private $repositoryPHIDs;
+  private $pusherPHIDs;
+
+  public function withIDs(array $ids) {
+    $this->ids = $ids;
+    return $this;
+  }
+
+  public function withPHIDs(array $phids) {
+    $this->phids = $phids;
+    return $this;
+  }
+
+  public function withRepositoryPHIDs(array $repository_phids) {
+    $this->repositoryPHIDs = $repository_phids;
+    return $this;
+  }
+
+  public function withPusherPHIDs(array $pusher_phids) {
+    $this->pusherPHIDs = $pusher_phids;
+    return $this;
+  }
+
+  protected function loadPage() {
+    $table = new PhabricatorRepositoryPushEvent();
+    $conn_r = $table->establishConnection('r');
+
+    $data = queryfx_all(
+      $conn_r,
+      'SELECT * FROM %T %Q %Q %Q',
+      $table->getTableName(),
+      $this->buildWhereClause($conn_r),
+      $this->buildOrderClause($conn_r),
+      $this->buildLimitClause($conn_r));
+
+    return $table->loadAllFromArray($data);
+  }
+
+  public function willFilterPage(array $events) {
+    $repository_phids = mpull($events, 'getRepositoryPHID');
+    $repositories = id(new PhabricatorRepositoryQuery())
+      ->setViewer($this->getViewer())
+      ->withPHIDs($repository_phids)
+      ->execute();
+    $repositories = mpull($repositories, null, 'getPHID');
+
+    foreach ($events as $key => $event) {
+      $phid = $event->getRepositoryPHID();
+      if (empty($repositories[$phid])) {
+        unset($events[$key]);
+        continue;
+      }
+      $event->attachRepository($repositories[$phid]);
+    }
+
+    return $events;
+  }
+
+
+  private function buildWhereClause(AphrontDatabaseConnection $conn_r) {
+    $where = array();
+
+    if ($this->ids) {
+      $where[] = qsprintf(
+        $conn_r,
+        'id IN (%Ld)',
+        $this->ids);
+    }
+
+    if ($this->phids) {
+      $where[] = qsprintf(
+        $conn_r,
+        'phid IN (%Ls)',
+        $this->phids);
+    }
+
+    if ($this->repositoryPHIDs) {
+      $where[] = qsprintf(
+        $conn_r,
+        'repositoryPHID IN (%Ls)',
+        $this->repositoryPHIDs);
+    }
+
+    if ($this->pusherPHIDs) {
+      $where[] = qsprintf(
+        $conn_r,
+        'pusherPHID in (%Ls)',
+        $this->pusherPHIDs);
+    }
+
+    $where[] = $this->buildPagingClause($conn_r);
+
+    return $this->formatWhereClause($where);
+  }
+
+
+  public function getQueryApplicationClass() {
+    return 'PhabricatorApplicationDiffusion';
+  }
+
+}

src/applications/repository/query/PhabricatorRepositoryPushLogQuery.php

@@ -56,24 +56,20 @@ final class PhabricatorRepositoryPushLogQuery
   }
 
   public function willFilterPage(array $logs) {
-    $repository_phids = mpull($logs, 'getRepositoryPHID');
-    if ($repository_phids) {
-      $repositories = id(new PhabricatorRepositoryQuery())
-        ->setViewer($this->getViewer())
-        ->withPHIDs($repository_phids)
-        ->execute();
-      $repositories = mpull($repositories, null, 'getPHID');
-    } else {
-      $repositories = array();
-    }
+    $event_phids = mpull($logs, 'getPushEventPHID');
+    $events = id(new PhabricatorRepositoryPushEventQuery())
+      ->setViewer($this->getViewer())
+      ->withPHIDs($event_phids)
+      ->execute();
+    $events = mpull($events, null, 'getPHID');
 
     foreach ($logs as $key => $log) {
-      $phid = $log->getRepositoryPHID();
-      if (empty($repositories[$phid])) {
+      $event = idx($events, $log->getPushEventPHID());
+      if (!$event) {
         unset($logs[$key]);
         continue;
       }
-      $log->attachRepository($repositories[$phid]);
+      $log->attachPushEvent($event);
     }
 
     return $logs;

src/applications/repository/storage/PhabricatorRepositoryPushEvent.php

@@ -0,0 +1,71 @@
+<?php
+
+/**
+ * Groups a set of push logs corresponding to changes which were all pushed in
+ * the same transaction.
+ */
+final class PhabricatorRepositoryPushEvent
+  extends PhabricatorRepositoryDAO
+  implements PhabricatorPolicyInterface {
+
+  protected $repositoryPHID;
+  protected $epoch;
+  protected $pusherPHID;
+  protected $remoteAddress;
+  protected $remoteProtocol;
+  protected $rejectCode;
+  protected $rejectDetails;
+
+  private $repository = self::ATTACHABLE;
+
+  public static function initializeNewEvent(PhabricatorUser $viewer) {
+    return id(new PhabricatorRepositoryPushEvent())
+      ->setPusherPHID($viewer->getPHID());
+  }
+
+  public function getConfiguration() {
+    return array(
+      self::CONFIG_AUX_PHID => true,
+      self::CONFIG_TIMESTAMPS => false,
+    ) + parent::getConfiguration();
+  }
+
+  public function generatePHID() {
+    return PhabricatorPHID::generateNewPHID(
+      PhabricatorRepositoryPHIDTypePushEvent::TYPECONST);
+  }
+
+  public function attachRepository(PhabricatorRepository $repository) {
+    $this->repository = $repository;
+    return $this;
+  }
+
+  public function getRepository() {
+    return $this->assertAttached($this->repository);
+  }
+
+
+/* -(  PhabricatorPolicyInterface  )----------------------------------------- */
+
+
+  public function getCapabilities() {
+    return array(
+      PhabricatorPolicyCapability::CAN_VIEW,
+    );
+  }
+
+  public function getPolicy($capability) {
+    return $this->getRepository()->getPolicy($capability);
+  }
+
+  public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
+    return $this->getRepository()->hasAutomaticCapability($capability, $viewer);
+  }
+
+  public function describeAutomaticCapability($capability) {
+    return pht(
+      "A repository's push events are visible to users who can see the ".
+      "repository.");
+  }
+
+}

src/applications/repository/storage/PhabricatorRepositoryPushLog.php

@@ -33,9 +33,7 @@ final class PhabricatorRepositoryPushLog
   protected $repositoryPHID;
   protected $epoch;
   protected $pusherPHID;
-  protected $remoteAddress;
-  protected $remoteProtocol;
-  protected $transactionKey;
+  protected $pushEventPHID;
   protected $refType;
   protected $refNameHash;
   protected $refNameRaw;
@@ -44,11 +42,9 @@ final class PhabricatorRepositoryPushLog
   protected $refNew;
   protected $mergeBase;
   protected $changeFlags;
-  protected $rejectCode;
-  protected $rejectDetails;
 
   private $dangerousChangeDescription = self::ATTACHABLE;
-  private $repository = self::ATTACHABLE;
+  private $pushEvent = self::ATTACHABLE;
 
   public static function initializeNewLog(PhabricatorUser $viewer) {
     return id(new PhabricatorRepositoryPushLog())
@@ -70,13 +66,17 @@ final class PhabricatorRepositoryPushLog
       PhabricatorRepositoryPHIDTypePushLog::TYPECONST);
   }
 
-  public function attachRepository(PhabricatorRepository $repository) {
-    $this->repository = $repository;
+  public function getRepository() {
+    return $this->getPushEvent()->getRepository();
+  }
+
+  public function attachPushEvent(PhabricatorRepositoryPushEvent $push_event) {
+    $this->pushEvent = $push_event;
     return $this;
   }
 
-  public function getRepository() {
-    return $this->assertAttached($this->repository);
+  public function getPushEvent() {
+    return $this->assertAttached($this->pushEvent);
   }
 
   public function getRefName() {
@@ -131,11 +131,11 @@ final class PhabricatorRepositoryPushLog
   }
 
   public function getPolicy($capability) {
-    return $this->getRepository()->getPolicy($capability);
+    return $this->getPushEvent()->getPolicy($capability);
   }
 
   public function hasAutomaticCapability($capability, PhabricatorUser $viewer) {
-    return $this->getRepository()->hasAutomaticCapability($capability, $viewer);
+    return $this->getPushEvent()->hasAutomaticCapability($capability, $viewer);
   }
 
   public function describeAutomaticCapability($capability) {