1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-09-20 01:08:50 +02:00

Differential - make DifferentialDiffEditor into a real transaction editor.

Summary: Ref T6237. This sets us up for some future work like T6152, T6200 and generally cleaning up this workflow a bit. Tried to do as little as possible so not exposing transaction view yet. (Though that timeline is going to be a little funky in the common case of just the lone create transaction.)

Test Plan: made a diff from web ui and it worked. made a herald rule to block certain diffs then tried to make such a diff and saw UI letting me know i was blocked

Reviewers: epriestley

Reviewed By: epriestley

Subscribers: Korvin, epriestley

Maniphest Tasks: T6237

Differential Revision: https://secure.phabricator.com/D10869
This commit is contained in:
Bob Trahan 2014-11-18 15:32:23 -08:00
parent edf88225f5
commit ffe0765b50
8 changed files with 376 additions and 143 deletions

View file

@ -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};

View file

@ -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',

View file

@ -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);

View file

@ -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);
}

View file

@ -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);

View file

@ -1,80 +1,236 @@
<?php
final class DifferentialDiffEditor extends PhabricatorEditor {
final class DifferentialDiffEditor
extends PhabricatorApplicationTransactionEditor {
private $contentSource;
private $diffDataDict;
public function setContentSource($content_source) {
$this->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;
}
}

View file

@ -1523,8 +1523,6 @@ final class DifferentialTransactionEditor
$adapter->setExplicitReviewers($reviewer_phids);
$adapter->setForbiddenCCs($unsubscribed_phids);
$adapter->setIsNewObject($this->getIsNewObject());
return $adapter;
}

View file

@ -0,0 +1,64 @@
<?php
final class DifferentialDiffTransaction
extends PhabricatorApplicationTransaction {
const TYPE_DIFF_CREATE = 'differential:diff:create';
public function getApplicationName() {
return 'differential';
}
public function getApplicationTransactionType() {
return DifferentialDiffPHIDType::TYPECONST;
}
public function shouldHideForMail(array $xactions) {
return true;
}
public function getActionName() {
switch ($this->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();
}
}