diff --git a/resources/sql/autopatches/20141118.diffxaction.sql b/resources/sql/autopatches/20141118.diffxaction.sql new file mode 100644 index 0000000000..4ee0554131 --- /dev/null +++ b/resources/sql/autopatches/20141118.diffxaction.sql @@ -0,0 +1,19 @@ +CREATE TABLE {$NAMESPACE}_differential.differential_difftransaction ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + phid VARBINARY(64) NOT NULL, + authorPHID VARBINARY(64) NOT NULL, + objectPHID VARBINARY(64) NOT NULL, + viewPolicy VARBINARY(64) NOT NULL, + editPolicy VARBINARY(64) NOT NULL, + commentPHID VARBINARY(64), + commentVersion INT UNSIGNED NOT NULL, + transactionType VARCHAR(32) COLLATE {$COLLATE_TEXT} NOT NULL, + oldValue LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + newValue LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + contentSource LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + metadata LONGTEXT COLLATE {$COLLATE_TEXT} NOT NULL, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY `key_phid` (`phid`), + KEY `key_object` (`objectPHID`) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index e71d3ba61f..b87309f229 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -325,6 +325,7 @@ phutil_register_library_map(array( 'DifferentialDiffQuery' => 'applications/differential/query/DifferentialDiffQuery.php', 'DifferentialDiffTableOfContentsView' => 'applications/differential/view/DifferentialDiffTableOfContentsView.php', 'DifferentialDiffTestCase' => 'applications/differential/storage/__tests__/DifferentialDiffTestCase.php', + 'DifferentialDiffTransaction' => 'applications/differential/storage/DifferentialDiffTransaction.php', 'DifferentialDiffViewController' => 'applications/differential/controller/DifferentialDiffViewController.php', 'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'applications/differential/doorkeeper/DifferentialDoorkeeperRevisionFeedStoryPublisher.php', 'DifferentialDraft' => 'applications/differential/storage/DifferentialDraft.php', @@ -3313,12 +3314,13 @@ phutil_register_library_map(array( ), 'DifferentialDiffCreateController' => 'DifferentialController', 'DifferentialDiffCreationRejectException' => 'Exception', - 'DifferentialDiffEditor' => 'PhabricatorEditor', + 'DifferentialDiffEditor' => 'PhabricatorApplicationTransactionEditor', 'DifferentialDiffPHIDType' => 'PhabricatorPHIDType', 'DifferentialDiffProperty' => 'DifferentialDAO', 'DifferentialDiffQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DifferentialDiffTableOfContentsView' => 'AphrontView', 'DifferentialDiffTestCase' => 'ArcanistPhutilTestCase', + 'DifferentialDiffTransaction' => 'PhabricatorApplicationTransaction', 'DifferentialDiffViewController' => 'DifferentialController', 'DifferentialDoorkeeperRevisionFeedStoryPublisher' => 'DoorkeeperFeedStoryPublisher', 'DifferentialDraft' => 'DifferentialDAO', diff --git a/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php index 8da95dc19a..3ea3c1768f 100644 --- a/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialCreateDiffConduitAPIMethod.php @@ -70,19 +70,13 @@ final class DifferentialCreateDiffConduitAPIMethod } $diff = DifferentialDiff::newFromRawChanges($changes); - $diff->setSourcePath($request->getValue('sourcePath')); - $diff->setSourceMachine($request->getValue('sourceMachine')); - $diff->setBranch($request->getValue('branch')); - $diff->setCreationMethod($request->getValue('creationMethod')); - $diff->setAuthorPHID($viewer->getPHID()); - $diff->setBookmark($request->getValue('bookmark')); - - // TODO: Remove this eventually; for now continue writing the UUID. Note - // that we'll overwrite it below if we identify a repository, and `arc` - // no longer sends it. This stuff is retained for backward compatibility. - $diff->setRepositoryUUID($request->getValue('repositoryUUID')); + // TODO: Remove repository UUID eventually; for now continue writing + // the UUID. Note that we'll overwrite it below if we identify a + // repository, and `arc` no longer sends it. This stuff is retained for + // backward compatibility. + $repository_uuid = $request->getValue('repositoryUUID'); $repository_phid = $request->getValue('repositoryPHID'); if ($repository_phid) { $repository = id(new PhabricatorRepositoryQuery()) @@ -90,17 +84,11 @@ final class DifferentialCreateDiffConduitAPIMethod ->withPHIDs(array($repository_phid)) ->executeOne(); if ($repository) { - $diff->setRepositoryPHID($repository->getPHID()); - $diff->setRepositoryUUID($repository->getUUID()); + $repository_phid = $repository->getPHID(); + $repository_uuid = $repository->getUUID(); } } - $system = $request->getValue('sourceControlSystem'); - $diff->setSourceControlSystem($system); - $diff->setSourceControlPath($request->getValue('sourceControlPath')); - $diff->setSourceControlBaseRevision( - $request->getValue('sourceControlBaseRevision')); - $project_name = $request->getValue('arcanistProject'); $project_phid = null; if ($project_name) { @@ -116,73 +104,75 @@ final class DifferentialCreateDiffConduitAPIMethod $project_phid = $arcanist_project->getPHID(); } - $diff->setArcanistProjectPHID($project_phid); - switch ($request->getValue('lintStatus')) { case 'skip': - $diff->setLintStatus(DifferentialLintStatus::LINT_SKIP); + $lint_status = DifferentialLintStatus::LINT_SKIP; break; case 'okay': - $diff->setLintStatus(DifferentialLintStatus::LINT_OKAY); + $lint_status = DifferentialLintStatus::LINT_OKAY; break; case 'warn': - $diff->setLintStatus(DifferentialLintStatus::LINT_WARN); + $lint_status = DifferentialLintStatus::LINT_WARN; break; case 'fail': - $diff->setLintStatus(DifferentialLintStatus::LINT_FAIL); + $lint_status = DifferentialLintStatus::LINT_FAIL; break; case 'postponed': - $diff->setLintStatus(DifferentialLintStatus::LINT_POSTPONED); + $lint_status = DifferentialLintStatus::LINT_POSTPONED; break; case 'none': default: - $diff->setLintStatus(DifferentialLintStatus::LINT_NONE); + $lint_status = DifferentialLintStatus::LINT_NONE; break; } switch ($request->getValue('unitStatus')) { case 'skip': - $diff->setUnitStatus(DifferentialUnitStatus::UNIT_SKIP); + $unit_status = DifferentialUnitStatus::UNIT_SKIP; break; case 'okay': - $diff->setUnitStatus(DifferentialUnitStatus::UNIT_OKAY); + $unit_status = DifferentialUnitStatus::UNIT_OKAY; break; case 'warn': - $diff->setUnitStatus(DifferentialUnitStatus::UNIT_WARN); + $unit_status = DifferentialUnitStatus::UNIT_WARN; break; case 'fail': - $diff->setUnitStatus(DifferentialUnitStatus::UNIT_FAIL); + $unit_status = DifferentialUnitStatus::UNIT_FAIL; break; case 'postponed': - $diff->setUnitStatus(DifferentialUnitStatus::UNIT_POSTPONED); + $unit_status = DifferentialUnitStatus::UNIT_POSTPONED; break; case 'none': default: - $diff->setUnitStatus(DifferentialUnitStatus::UNIT_NONE); + $unit_status = DifferentialUnitStatus::UNIT_NONE; break; } + $diff_data_dict = array( + 'sourcePath' => $request->getValue('sourcePath'), + 'sourceMachine' => $request->getValue('sourceMachine'), + 'branch' => $request->getValue('branch'), + 'creationMethod' => $request->getValue('creationMethod'), + 'authorPHID' => $viewer->getPHID(), + 'bookmark' => $request->getValue('bookmark'), + 'repositoryUUID' => $repository_uuid, + 'repositoryPHID' => $repository_phid, + 'sourceControlSystem' => $request->getValue('sourceControlSystem'), + 'sourceControlPath' => $request->getValue('sourceControlPath'), + 'sourceControlBaseRevision' => + $request->getValue('sourceControlBaseRevision'), + 'arcanistProjectPHID' => $project_phid, + 'lintStatus' => $lint_status, + 'unitStatus' => $unit_status,); + + $xactions = array(id(new DifferentialTransaction()) + ->setTransactionType(DifferentialDiffTransaction::TYPE_DIFF_CREATE) + ->setNewValue($diff_data_dict),); + id(new DifferentialDiffEditor()) ->setActor($viewer) - ->setContentSource( - PhabricatorContentSource::newFromConduitRequest($request)) - ->saveDiff($diff); - - // If we didn't get an explicit `repositoryPHID` (which means the client is - // old, or couldn't figure out which repository the working copy belongs - // to), apply heuristics to try to figure it out. - - if (!$repository_phid) { - $repository = id(new DifferentialRepositoryLookup()) - ->setDiff($diff) - ->setViewer($viewer) - ->lookupRepository(); - if ($repository) { - $diff->setRepositoryPHID($repository->getPHID()); - $diff->setRepositoryUUID($repository->getUUID()); - $diff->save(); - } - } + ->setContentSourceFromConduitRequest($request) + ->applyTransactions($diff, $xactions); $path = '/differential/diff/'.$diff->getID().'/'; $uri = PhabricatorEnv::getURI($path); diff --git a/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php b/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php index f87dcb1a73..219f5e6dd2 100644 --- a/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php +++ b/src/applications/differential/conduit/DifferentialCreateRawDiffConduitAPIMethod.php @@ -41,29 +41,27 @@ final class DifferentialCreateRawDiffConduitAPIMethod throw new Exception( pht('No such repository "%s"!', $repository_phid)); } - } else { - $repository = null; } $parser = new ArcanistDiffParser(); $changes = $parser->parseDiff($raw_diff); $diff = DifferentialDiff::newFromRawChanges($changes); - $diff->setLintStatus(DifferentialLintStatus::LINT_SKIP); - $diff->setUnitStatus(DifferentialUnitStatus::UNIT_SKIP); + $diff_data_dict = array( + 'creationMethod' => 'web', + 'authorPHID' => $viewer->getPHID(), + 'repositoryPHID' => $repository_phid, + 'lintStatus' => DifferentialLintStatus::LINT_SKIP, + 'unitStatus' => DifferentialUnitStatus::UNIT_SKIP,); - $diff->setAuthorPHID($viewer->getPHID()); - $diff->setCreationMethod('web'); - - if ($repository) { - $diff->setRepositoryPHID($repository->getPHID()); - } + $xactions = array(id(new DifferentialTransaction()) + ->setTransactionType(DifferentialDiffTransaction::TYPE_DIFF_CREATE) + ->setNewValue($diff_data_dict),); id(new DifferentialDiffEditor()) ->setActor($viewer) - ->setContentSource( - PhabricatorContentSource::newFromConduitRequest($request)) - ->saveDiff($diff); + ->setContentSourceFromConduitRequest($request) + ->applyTransactions($diff, $xactions); return $this->buildDiffInfoDictionary($diff); } diff --git a/src/applications/differential/controller/DifferentialDiffCreateController.php b/src/applications/differential/controller/DifferentialDiffCreateController.php index 00f7854d12..d2493b9f60 100644 --- a/src/applications/differential/controller/DifferentialDiffCreateController.php +++ b/src/applications/differential/controller/DifferentialDiffCreateController.php @@ -6,11 +6,12 @@ final class DifferentialDiffCreateController extends DifferentialController { $request = $this->getRequest(); + $diff = null; $errors = array(); $e_diff = null; $e_file = null; + $validation_exception = null; if ($request->isFormPost()) { - $diff = null; if ($request->getFileExists('diff-file')) { $diff = PhabricatorFile::readUploadedFileData($_FILES['diff-file']); @@ -27,16 +28,19 @@ final class DifferentialDiffCreateController extends DifferentialController { } if (!$errors) { - $call = new ConduitCall( - 'differential.createrawdiff', - array( - 'diff' => $diff, + try { + $call = new ConduitCall( + 'differential.createrawdiff', + array( + 'diff' => $diff, )); - $call->setUser($request->getUser()); - $result = $call->execute(); - - $path = id(new PhutilURI($result['uri']))->getPath(); - return id(new AphrontRedirectResponse())->setURI($path); + $call->setUser($request->getUser()); + $result = $call->execute(); + $path = id(new PhutilURI($result['uri']))->getPath(); + return id(new AphrontRedirectResponse())->setURI($path); + } catch (PhabricatorApplicationTransactionValidationException $ex) { + $validation_exception = $ex; + } } } @@ -69,6 +73,7 @@ final class DifferentialDiffCreateController extends DifferentialController { id(new AphrontFormTextAreaControl()) ->setLabel(pht('Raw Diff')) ->setName('diff') + ->setValue($diff) ->setHeight(AphrontFormTextAreaControl::HEIGHT_VERY_TALL) ->setError($e_diff)) ->appendChild( @@ -83,6 +88,7 @@ final class DifferentialDiffCreateController extends DifferentialController { $form_box = id(new PHUIObjectBoxView()) ->setHeaderText(pht('Create New Diff')) + ->setValidationException($validation_exception) ->setForm($form) ->setFormErrors($errors); diff --git a/src/applications/differential/editor/DifferentialDiffEditor.php b/src/applications/differential/editor/DifferentialDiffEditor.php index 738dd7afeb..0d87df60de 100644 --- a/src/applications/differential/editor/DifferentialDiffEditor.php +++ b/src/applications/differential/editor/DifferentialDiffEditor.php @@ -1,80 +1,236 @@ contentSource = $content_source; - return $this; + public function getEditorApplicationClass() { + return 'PhabricatorDifferentialApplication'; } - public function getContentSource() { - return $this->contentSource; + public function getEditorObjectsDescription() { + return pht('Differential Diffs'); } - public function saveDiff(DifferentialDiff $diff) { - $actor = $this->requireActor(); + public function getTransactionTypes() { + $types = parent::getTransactionTypes(); - // Generate a PHID first, so the transcript will point at the object if - // we deicde to preserve it. - $phid = $diff->generatePHID(); - $diff->setPHID($phid); + $types[] = DifferentialDiffTransaction::TYPE_DIFF_CREATE; + + return $types; + } + + protected function getCustomTransactionOldValue( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case DifferentialDiffTransaction::TYPE_DIFF_CREATE: + return null; + } + + return parent::getCustomTransactionOldValue($object, $xaction); + } + + protected function getCustomTransactionNewValue( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case DifferentialDiffTransaction::TYPE_DIFF_CREATE: + $this->diffDataDict = $xaction->getNewValue(); + return true; + } + + return parent::getCustomTransactionNewValue($object, $xaction); + } + + protected function applyCustomInternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case DifferentialDiffTransaction::TYPE_DIFF_CREATE: + $dict = $this->diffDataDict; + $this->updateDiffFromDict($object, $dict); + return; + } + + return parent::applyCustomInternalTransaction($object, $xaction); + } + + protected function applyCustomExternalTransaction( + PhabricatorLiskDAO $object, + PhabricatorApplicationTransaction $xaction) { + + switch ($xaction->getTransactionType()) { + case DifferentialDiffTransaction::TYPE_DIFF_CREATE: + return; + } + + return parent::applyCustomExternalTransaction($object, $xaction); + } + + protected function applyFinalEffects( + PhabricatorLiskDAO $object, + array $xactions) { + + // If we didn't get an explicit `repositoryPHID` (which means the client + // is old, or couldn't figure out which repository the working copy + // belongs to), apply heuristics to try to figure it out. + + if (!$object->getRepositoryPHID()) { + $repository = id(new DifferentialRepositoryLookup()) + ->setDiff($object) + ->setViewer($this->getActor()) + ->lookupRepository(); + if ($repository) { + $object->setRepositoryPHID($repository->getPHID()); + $object->setRepositoryUUID($repository->getUUID()); + $object->save(); + } + } + + return $xactions; + } + + /** + * We run Herald as part of transaction validation because Herald can + * block diff creation for Differential diffs. Its important to do this + * separately so no Herald logs are saved; these logs could expose + * information the Herald rules are inteneded to block. + */ + protected function validateTransaction( + PhabricatorLiskDAO $object, + $type, + array $xactions) { + + $errors = parent::validateTransaction($object, $type, $xactions); + + foreach ($xactions as $xaction) { + switch ($type) { + case DifferentialDiffTransaction::TYPE_DIFF_CREATE: + $diff = clone $object; + $diff = $this->updateDiffFromDict($diff, $xaction->getNewValue()); + + $adapter = $this->buildHeraldAdapter($diff, $xactions); + $adapter->setContentSource($this->getContentSource()); + $adapter->setIsNewObject($this->getIsNewObject()); + + $engine = new HeraldEngine(); + + $rules = $engine->loadRulesForAdapter($adapter); + $rules = mpull($rules, null, 'getID'); + + $effects = $engine->applyRules($rules, $adapter); + + $blocking_effect = null; + foreach ($effects as $effect) { + if ($effect->getAction() == HeraldAdapter::ACTION_BLOCK) { + $blocking_effect = $effect; + break; + } + } + + if ($blocking_effect) { + $rule = idx($rules, $effect->getRuleID()); + if ($rule && strlen($rule->getName())) { + $rule_name = $rule->getName(); + } else { + $rule_name = pht('Unnamed Herald Rule'); + } + + $message = $effect->getTarget(); + if (!strlen($message)) { + $message = pht('(None.)'); + } + + $errors[] = new PhabricatorApplicationTransactionValidationError( + $type, + pht('Rejected by Herald'), + pht( + "Creation of this diff was rejected by Herald rule %s.\n". + " Rule: %s\n". + "Reason: %s", + 'H'.$effect->getRuleID(), + $rule_name, + $message)); + } + break; + } + } + + return $errors; + } + + + protected function shouldPublishFeedStory( + PhabricatorLiskDAO $object, + array $xactions) { + return false; + } + + protected function shouldSendMail( + PhabricatorLiskDAO $object, + array $xactions) { + return false; + } + + protected function supportsSearch() { + return false; + } + +/* -( Herald Integration )------------------------------------------------- */ + + /** + * See @{method:validateTransaction}. The only Herald action is to block + * the creation of Diffs. We thus have to be careful not to save any + * data and do this validation very early. + */ + protected function shouldApplyHeraldRules( + PhabricatorLiskDAO $object, + array $xactions) { + + return false; + } + + protected function buildHeraldAdapter( + PhabricatorLiskDAO $object, + array $xactions) { $adapter = id(new HeraldDifferentialDiffAdapter()) - ->setDiff($diff); - - $adapter->setContentSource($this->getContentSource()); - $adapter->setIsNewObject(true); - - $engine = new HeraldEngine(); - - $rules = $engine->loadRulesForAdapter($adapter); - $rules = mpull($rules, null, 'getID'); - - $effects = $engine->applyRules($rules, $adapter); - - $blocking_effect = null; - foreach ($effects as $effect) { - if ($effect->getAction() == HeraldAdapter::ACTION_BLOCK) { - $blocking_effect = $effect; - break; - } - } - - if ($blocking_effect) { - $rule = idx($rules, $effect->getRuleID()); - if ($rule && strlen($rule->getName())) { - $rule_name = $rule->getName(); - } else { - $rule_name = pht('Unnamed Herald Rule'); - } - - $message = $effect->getTarget(); - if (!strlen($message)) { - $message = pht('(None.)'); - } - - throw new DifferentialDiffCreationRejectException( - pht( - "Creation of this diff was rejected by Herald rule %s.\n". - " Rule: %s\n". - "Reason: %s", - 'H'.$effect->getRuleID(), - $rule_name, - $message)); - } - - $diff->save(); - - // NOTE: We only save the transcript if we didn't block the diff. - // Otherwise, we might save some of the diff's content in the transcript - // table, which would defeat the purpose of allowing rules to block - // storage of key material. - - $engine->applyEffects($effects, $adapter, $rules); - $xscript = $engine->getTranscript(); + ->setDiff($object); + return $adapter; } + protected function didApplyHeraldRules( + PhabricatorLiskDAO $object, + HeraldAdapter $adapter, + HeraldTranscript $transcript) { + + $xactions = array(); + return $xactions; + } + + private function updateDiffFromDict(DifferentialDiff $diff, $dict) { + $diff + ->setSourcePath(idx($dict, 'sourcePath')) + ->setSourceMachine(idx($dict, 'sourceMachine')) + ->setBranch(idx($dict, 'branch')) + ->setCreationMethod(idx($dict, 'creationMethod')) + ->setAuthorPHID(idx($dict, 'authorPHID', $this->getActor())) + ->setBookmark(idx($dict, 'bookmark')) + ->setRepositoryPHID(idx($dict, 'repositoryPHID')) + ->setRepositoryUUID(idx($dict, 'repositoryUUID')) + ->setSourceControlSystem(idx($dict, 'sourceControlSystem')) + ->setSourceControlPath(idx($dict, 'sourceControlPath')) + ->setSourceControlBaseRevision(idx($dict, 'sourceControlBaseRevision')) + ->setLintStatus(idx($dict, 'lintStatus')) + ->setUnitStatus(idx($dict, 'unitStatus')) + ->setArcanistProjectPHID(idx($dict, 'arcanistProjectPHID')); + + return $diff; + } } diff --git a/src/applications/differential/editor/DifferentialTransactionEditor.php b/src/applications/differential/editor/DifferentialTransactionEditor.php index dc0058b0f3..a90ead7c0a 100644 --- a/src/applications/differential/editor/DifferentialTransactionEditor.php +++ b/src/applications/differential/editor/DifferentialTransactionEditor.php @@ -1523,8 +1523,6 @@ final class DifferentialTransactionEditor $adapter->setExplicitReviewers($reviewer_phids); $adapter->setForbiddenCCs($unsubscribed_phids); - $adapter->setIsNewObject($this->getIsNewObject()); - return $adapter; } diff --git a/src/applications/differential/storage/DifferentialDiffTransaction.php b/src/applications/differential/storage/DifferentialDiffTransaction.php new file mode 100644 index 0000000000..430638c3c2 --- /dev/null +++ b/src/applications/differential/storage/DifferentialDiffTransaction.php @@ -0,0 +1,64 @@ +getTransactionType()) { + case self::TYPE_DIFF_CREATE; + return pht('Created'); + } + + return parent::getActionName(); + } + + public function getTitle() { + $author_phid = $this->getAuthorPHID(); + $author_handle = $this->renderHandleLink($author_phid); + + $old = $this->getOldValue(); + $new = $this->getNewValue(); + + switch ($this->getTransactionType()) { + case self::TYPE_DIFF_CREATE; + return pht( + '%s created this diff.', + $author_handle); + } + + return parent::getTitle(); + } + + public function getIcon() { + switch ($this->getTransactionType()) { + case self::TYPE_DIFF_CREATE: + return 'fa-refresh'; + } + + return parent::getIcon(); + } + + public function getColor() { + switch ($this->getTransactionType()) { + case self::TYPE_DIFF_CREATE: + return PhabricatorTransactions::COLOR_SKY; + } + + return parent::getColor(); + } + +}