diff --git a/resources/sql/autopatches/20200417.viewstate.01.storage.sql b/resources/sql/autopatches/20200417.viewstate.01.storage.sql new file mode 100644 --- /dev/null +++ b/resources/sql/autopatches/20200417.viewstate.01.storage.sql @@ -0,0 +1,9 @@ +CREATE TABLE {$NAMESPACE}_differential.differential_viewstate ( + id INT UNSIGNED NOT NULL AUTO_INCREMENT PRIMARY KEY, + viewerPHID VARBINARY(64) NOT NULL, + objectPHID VARBINARY(64) NOT NULL, + viewState LONGTEXT NOT NULL COLLATE {$COLLATE_TEXT}, + dateCreated INT UNSIGNED NOT NULL, + dateModified INT UNSIGNED NOT NULL, + UNIQUE KEY `key_viewer` (viewerPHID, objectPHID) +) ENGINE=InnoDB, COLLATE {$COLLATE_TEXT}; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -713,6 +713,8 @@ 'DifferentialUnitStatus' => 'applications/differential/constants/DifferentialUnitStatus.php', 'DifferentialUnitTestResult' => 'applications/differential/constants/DifferentialUnitTestResult.php', 'DifferentialUpdateRevisionConduitAPIMethod' => 'applications/differential/conduit/DifferentialUpdateRevisionConduitAPIMethod.php', + 'DifferentialViewState' => 'applications/differential/storage/DifferentialViewState.php', + 'DifferentialViewStateQuery' => 'applications/differential/query/DifferentialViewStateQuery.php', 'DiffusionAuditorDatasource' => 'applications/diffusion/typeahead/DiffusionAuditorDatasource.php', 'DiffusionAuditorFunctionDatasource' => 'applications/diffusion/typeahead/DiffusionAuditorFunctionDatasource.php', 'DiffusionAuditorsAddAuditorsHeraldAction' => 'applications/diffusion/herald/DiffusionAuditorsAddAuditorsHeraldAction.php', @@ -2735,6 +2737,8 @@ 'PhabricatorChangePasswordUserLogType' => 'applications/people/userlog/PhabricatorChangePasswordUserLogType.php', 'PhabricatorChangesetCachePurger' => 'applications/cache/purger/PhabricatorChangesetCachePurger.php', 'PhabricatorChangesetResponse' => 'infrastructure/diff/PhabricatorChangesetResponse.php', + 'PhabricatorChangesetViewState' => 'infrastructure/diff/viewstate/PhabricatorChangesetViewState.php', + 'PhabricatorChangesetViewStateEngine' => 'infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php', 'PhabricatorChartAxis' => 'applications/fact/chart/PhabricatorChartAxis.php', 'PhabricatorChartDataQuery' => 'applications/fact/chart/PhabricatorChartDataQuery.php', 'PhabricatorChartDataset' => 'applications/fact/chart/PhabricatorChartDataset.php', @@ -6783,6 +6787,11 @@ 'DifferentialUnitStatus' => 'Phobject', 'DifferentialUnitTestResult' => 'Phobject', 'DifferentialUpdateRevisionConduitAPIMethod' => 'DifferentialConduitAPIMethod', + 'DifferentialViewState' => array( + 'DifferentialDAO', + 'PhabricatorPolicyInterface', + ), + 'DifferentialViewStateQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'DiffusionAuditorDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DiffusionAuditorFunctionDatasource' => 'PhabricatorTypeaheadCompositeDatasource', 'DiffusionAuditorsAddAuditorsHeraldAction' => 'DiffusionAuditorsHeraldAction', @@ -9130,6 +9139,8 @@ 'PhabricatorChangePasswordUserLogType' => 'PhabricatorUserLogType', 'PhabricatorChangesetCachePurger' => 'PhabricatorCachePurger', 'PhabricatorChangesetResponse' => 'AphrontProxyResponse', + 'PhabricatorChangesetViewState' => 'Phobject', + 'PhabricatorChangesetViewStateEngine' => 'Phobject', 'PhabricatorChartAxis' => 'Phobject', 'PhabricatorChartDataQuery' => 'Phobject', 'PhabricatorChartDataset' => 'Phobject', diff --git a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php b/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php --- a/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php +++ b/src/applications/differential/__tests__/DifferentialParseRenderTestCase.php @@ -89,13 +89,16 @@ $engine = new PhabricatorMarkupEngine(); $engine->setViewer(new PhabricatorUser()); + $viewstate = new PhabricatorChangesetViewState(); + $parsers = array(); foreach ($changesets as $changeset) { - $cparser = new DifferentialChangesetParser(); - $cparser->setUser(new PhabricatorUser()); - $cparser->setDisableCache(true); - $cparser->setChangeset($changeset); - $cparser->setMarkupEngine($engine); + $cparser = id(new DifferentialChangesetParser()) + ->setViewer(new PhabricatorUser()) + ->setDisableCache(true) + ->setChangeset($changeset) + ->setMarkupEngine($engine) + ->setViewState($viewstate); if ($type == 'one') { $cparser->setRenderer(new DifferentialChangesetOneUpTestRenderer()); diff --git a/src/applications/differential/controller/DifferentialChangesetViewController.php b/src/applications/differential/controller/DifferentialChangesetViewController.php --- a/src/applications/differential/controller/DifferentialChangesetViewController.php +++ b/src/applications/differential/controller/DifferentialChangesetViewController.php @@ -165,21 +165,6 @@ list($range_s, $range_e, $mask) = DifferentialChangesetParser::parseRangeSpecification($spec); - $parser = id(new DifferentialChangesetParser()) - ->setViewer($viewer) - ->setCoverage($coverage) - ->setChangeset($changeset) - ->setRenderingReference($rendering_reference) - ->setRenderCacheKey($render_cache_key) - ->setRightSideCommentMapping($right_source, $right_new) - ->setLeftSideCommentMapping($left_source, $left_new); - - $parser->readParametersFromRequest($request); - - if ($left && $right) { - $parser->setOriginals($left, $right); - } - $diff = $changeset->getDiff(); $revision_id = $diff->getRevisionID(); @@ -197,6 +182,35 @@ } } + if ($revision) { + $container_phid = $revision->getPHID(); + } else { + $container_phid = $diff->getPHID(); + } + + $viewstate_engine = id(new PhabricatorChangesetViewStateEngine()) + ->setViewer($viewer) + ->setObjectPHID($container_phid) + ->setChangeset($changeset); + + $viewstate = $viewstate_engine->newViewStateFromRequest($request); + + $parser = id(new DifferentialChangesetParser()) + ->setViewer($viewer) + ->setViewState($viewstate) + ->setCoverage($coverage) + ->setChangeset($changeset) + ->setRenderingReference($rendering_reference) + ->setRenderCacheKey($render_cache_key) + ->setRightSideCommentMapping($right_source, $right_new) + ->setLeftSideCommentMapping($left_source, $left_new); + + $parser->readParametersFromRequest($request); + + if ($left && $right) { + $parser->setOriginals($left, $right); + } + // Load both left-side and right-side inline comments. if ($revision) { $query = id(new DifferentialInlineCommentQuery()) @@ -249,7 +263,7 @@ $engine->process(); $parser - ->setUser($viewer) + ->setViewer($viewer) ->setMarkupEngine($engine) ->setShowEditAndReplyLinks(true) ->setCanMarkDone($can_mark) diff --git a/src/applications/differential/parser/DifferentialChangesetParser.php b/src/applications/differential/parser/DifferentialChangesetParser.php --- a/src/applications/differential/parser/DifferentialChangesetParser.php +++ b/src/applications/differential/parser/DifferentialChangesetParser.php @@ -45,7 +45,6 @@ private $disableCache; private $renderer; private $characterEncoding; - private $highlightAs; private $highlightingDisabled; private $showEditAndReplyLinks = true; private $canMarkDone; @@ -61,6 +60,8 @@ private $viewer; private $documentEngineKey; + private $viewState; + public function setRange($start, $end) { $this->rangeStart = $start; $this->rangeEnd = $end; @@ -85,13 +86,13 @@ return $this->showEditAndReplyLinks; } - public function setHighlightAs($highlight_as) { - $this->highlightAs = $highlight_as; + public function setViewState(PhabricatorChangesetViewState $view_state) { + $this->viewState = $view_state; return $this; } - public function getHighlightAs() { - return $this->highlightAs; + public function getViewState() { + return $this->viewState; } public function setCharacterEncoding($character_encoding) { @@ -183,7 +184,6 @@ public function readParametersFromRequest(AphrontRequest $request) { $this->setCharacterEncoding($request->getStr('encoding')); - $this->setHighlightAs($request->getStr('highlight')); $this->setDocumentEngineKey($request->getStr('engine')); $renderer = null; @@ -378,15 +378,6 @@ return $this; } - public function setUser(PhabricatorUser $user) { - $this->user = $user; - return $this; - } - - public function getUser() { - return $this->user; - } - public function setCoverage($coverage) { $this->coverage = $coverage; return $this; @@ -604,7 +595,7 @@ } private function getHighlightFuture($corpus) { - $language = $this->highlightAs; + $language = $this->getViewState()->getHighlightLanguage(); if (!$language) { $language = $this->highlightEngine->getLanguageFromFilename( @@ -634,6 +625,8 @@ } private function tryCacheStuff() { + $viewstate = $this->getViewState(); + $skip_cache = false; if ($this->disableCache) { @@ -644,7 +637,8 @@ $skip_cache = true; } - if ($this->highlightAs) { + $highlight_language = $viewstate->getHighlightLanguage(); + if ($highlight_language !== null) { $skip_cache = true; } @@ -844,7 +838,7 @@ count($this->new)); $renderer = $this->getRenderer() - ->setUser($this->getUser()) + ->setUser($this->getViewer()) ->setChangeset($this->changeset) ->setRenderPropertyChangeHeader($render_pch) ->setIsTopLevel($this->isTopLevel) diff --git a/src/applications/differential/query/DifferentialViewStateQuery.php b/src/applications/differential/query/DifferentialViewStateQuery.php new file mode 100644 --- /dev/null +++ b/src/applications/differential/query/DifferentialViewStateQuery.php @@ -0,0 +1,64 @@ +ids = $ids; + return $this; + } + + public function withViewerPHIDs(array $phids) { + $this->viewerPHIDs = $phids; + return $this; + } + + public function withObjectPHIDs(array $phids) { + $this->objectPHIDs = $phids; + return $this; + } + + public function newResultObject() { + return new DifferentialViewState(); + } + + protected function loadPage() { + return $this->loadStandardPage($this->newResultObject()); + } + + protected function buildWhereClauseParts(AphrontDatabaseConnection $conn) { + $where = parent::buildWhereClauseParts($conn); + + if ($this->ids !== null) { + $where[] = qsprintf( + $conn, + 'id IN (%Ld)', + $this->ids); + } + + if ($this->viewerPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'viewerPHID IN (%Ls)', + $this->viewerPHIDs); + } + + if ($this->objectPHIDs !== null) { + $where[] = qsprintf( + $conn, + 'objectPHID IN (%Ls)', + $this->objectPHIDs); + } + + return $where; + } + + public function getQueryApplicationClass() { + return 'PhabricatorDifferentialApplication'; + } + +} diff --git a/src/applications/differential/storage/DifferentialDiff.php b/src/applications/differential/storage/DifferentialDiff.php --- a/src/applications/differential/storage/DifferentialDiff.php +++ b/src/applications/differential/storage/DifferentialDiff.php @@ -716,6 +716,8 @@ public function destroyObjectPermanently( PhabricatorDestructionEngine $engine) { + $viewer = $engine->getViewer(); + $this->openTransaction(); $this->delete(); @@ -730,6 +732,13 @@ $prop->delete(); } + $viewstates = id(new DifferentialViewStateQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($this->getPHID())); + foreach ($viewstates as $viewstate) { + $viewstate->delete(); + } + $this->saveTransaction(); } diff --git a/src/applications/differential/storage/DifferentialRevision.php b/src/applications/differential/storage/DifferentialRevision.php --- a/src/applications/differential/storage/DifferentialRevision.php +++ b/src/applications/differential/storage/DifferentialRevision.php @@ -1002,9 +1002,11 @@ public function destroyObjectPermanently( PhabricatorDestructionEngine $engine) { + $viewer = $engine->getViewer(); + $this->openTransaction(); $diffs = id(new DifferentialDiffQuery()) - ->setViewer($engine->getViewer()) + ->setViewer($viewer) ->withRevisionIDs(array($this->getID())) ->execute(); foreach ($diffs as $diff) { @@ -1022,6 +1024,13 @@ $dummy_path->getTableName(), $this->getID()); + $viewstates = id(new DifferentialViewStateQuery()) + ->setViewer($viewer) + ->withObjectPHIDs(array($this->getPHID())); + foreach ($viewstates as $viewstate) { + $viewstate->delete(); + } + $this->delete(); $this->saveTransaction(); } diff --git a/src/applications/differential/storage/DifferentialViewState.php b/src/applications/differential/storage/DifferentialViewState.php new file mode 100644 --- /dev/null +++ b/src/applications/differential/storage/DifferentialViewState.php @@ -0,0 +1,132 @@ + array( + 'viewState' => self::SERIALIZATION_JSON, + ), + self::CONFIG_KEY_SCHEMA => array( + 'key_viewer' => array( + 'columns' => array('viewerPHID', 'objectPHID'), + 'unique' => true, + ), + 'key_object' => array( + 'columns' => array('objectPHID'), + ), + ), + ) + parent::getConfiguration(); + } + + public function setChangesetProperty( + DifferentialChangeset $changeset, + $key, + $value) { + + if ($this->getChangesetProperty($changeset, $key) === $value) { + return; + } + + $properties = array( + 'value' => $value, + 'epoch' => PhabricatorTime::getNow(), + ); + + $diff_id = $changeset->getDiffID(); + if ($diff_id !== null) { + $properties['diffID'] = (int)$diff_id; + } + + $path_hash = $this->getChangesetPathHash($changeset); + $changeset_phid = $this->getChangesetKey($changeset); + + $this->hasModifications = true; + + $this->viewState['changesets'][$path_hash][$key][$changeset_phid] = + $properties; + } + + public function getChangesetProperty( + DifferentialChangeset $changeset, + $key, + $default = null) { + + $path_hash = $this->getChangesetPathHash($changeset); + + $entries = idxv($this->viewState, array('changesets', $path_hash, $key)); + if (!is_array($entries)) { + $entries = array(); + } + + $entries = isort($entries, 'epoch'); + + $entry = last($entries); + if (!is_array($entry)) { + $entry = array(); + } + + return idx($entry, 'value', $default); + } + + public function getHasModifications() { + return $this->hasModifications; + } + + private function getChangesetPathHash(DifferentialChangeset $changeset) { + $path = $changeset->getFilename(); + return PhabricatorHash::digestForIndex($path); + } + + private function getChangesetKey(DifferentialChangeset $changeset) { + $key = $changeset->getID(); + + if ($key === null) { + return '*'; + } + + return (string)$key; + } + + public static function copyViewStatesToObject($src_phid, $dst_phid) { + $table = new self(); + $conn = $table->establishConnection('w'); + + queryfx( + $conn, + 'INSERT IGNORE INTO %R + (viewerPHID, objectPHID, viewState, dateCreated, dateModified) + SELECT viewerPHID, %s, viewState, dateCreated, dateModified + FROM %R WHERE objectPHID = %s', + $table, + $dst_phid, + $table, + $src_phid); + } + +/* -( PhabricatorPolicyInterface )----------------------------------------- */ + + + public function getCapabilities() { + return array( + PhabricatorPolicyCapability::CAN_VIEW, + ); + } + + public function getPolicy($capability) { + return PhabricatorPolicies::POLICY_NOONE; + } + + public function hasAutomaticCapability($capability, PhabricatorUser $viewer) { + return ($viewer->getPHID() === $this->getViewerPHID()); + } + +} diff --git a/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php b/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php --- a/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php +++ b/src/applications/differential/xaction/DifferentialRevisionUpdateTransaction.php @@ -100,6 +100,14 @@ HarbormasterMessageType::BUILDABLE_CONTAINER, true); } + + // See T13455. If users have set view properites on a diff and the diff + // is then attached to a revision, attempt to copy their view preferences + // to the revision. + + DifferentialViewState::copyViewStatesToObject( + $diff->getPHID(), + $object->getPHID()); } public function getColor() { diff --git a/src/applications/diffusion/controller/DiffusionDiffController.php b/src/applications/diffusion/controller/DiffusionDiffController.php --- a/src/applications/diffusion/controller/DiffusionDiffController.php +++ b/src/applications/diffusion/controller/DiffusionDiffController.php @@ -60,9 +60,20 @@ return new Aphront404Response(); } - $parser = new DifferentialChangesetParser(); - $parser->setUser($viewer); - $parser->setChangeset($changeset); + $commit = $drequest->loadCommit(); + + $viewstate_engine = id(new PhabricatorChangesetViewStateEngine()) + ->setViewer($viewer) + ->setObjectPHID($commit->getPHID()) + ->setChangeset($changeset); + + $viewstate = $viewstate_engine->newViewStateFromRequest($request); + + $parser = id(new DifferentialChangesetParser()) + ->setViewer($viewer) + ->setChangeset($changeset) + ->setViewState($viewstate); + $parser->setRenderingReference($drequest->generateURI( array( 'action' => 'rendering-ref', @@ -75,8 +86,6 @@ $parser->setCoverage($coverage); } - $commit = $drequest->loadCommit(); - $pquery = new DiffusionPathIDQuery(array($changeset->getFilename())); $ids = $pquery->loadPathIDs(); $path_id = $ids[$changeset->getFilename()]; diff --git a/src/infrastructure/diff/viewstate/PhabricatorChangesetViewState.php b/src/infrastructure/diff/viewstate/PhabricatorChangesetViewState.php new file mode 100644 --- /dev/null +++ b/src/infrastructure/diff/viewstate/PhabricatorChangesetViewState.php @@ -0,0 +1,17 @@ +highlightLanguage = $highlight_language; + return $this; + } + + public function getHighlightLanguage() { + return $this->highlightLanguage; + } + +} diff --git a/src/infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php b/src/infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php new file mode 100644 --- /dev/null +++ b/src/infrastructure/diff/viewstate/PhabricatorChangesetViewStateEngine.php @@ -0,0 +1,145 @@ +viewer = $viewer; + return $this; + } + + public function getViewer() { + return $this->viewer; + } + + public function setObjectPHID($object_phid) { + $this->objectPHID = $object_phid; + return $this; + } + + public function getObjectPHID() { + return $this->objectPHID; + } + + public function setChangeset(DifferentialChangeset $changeset) { + $this->changeset = $changeset; + return $this; + } + + public function getChangeset() { + return $this->changeset; + } + + public function newViewStateFromRequest(AphrontRequest $request) { + $storage = $this->loadViewStateStorage(); + + $this->setStorage($storage); + + $highlight = $request->getStr('highlight'); + if ($highlight !== null && strlen($highlight)) { + $this->setChangesetProperty('highlight', $highlight); + } + + $this->saveViewStateStorage(); + + $state = new PhabricatorChangesetViewState(); + + $highlight_language = $this->getChangesetProperty('highlight'); + $state->setHighlightLanguage($highlight_language); + + return $state; + } + + private function setStorage(DifferentialViewState $storage) { + $this->storage = $storage; + return $this; + } + + private function getStorage() { + return $this->storage; + } + + private function setChangesetProperty( + $key, + $value) { + + $storage = $this->getStorage(); + $changeset = $this->getChangeset(); + + $storage->setChangesetProperty($changeset, $key, $value); + } + + private function getChangesetProperty( + $key, + $default = null) { + + $storage = $this->getStorage(); + $changeset = $this->getChangeset(); + + return $storage->getChangesetProperty($changeset, $key, $default); + } + + private function loadViewStateStorage() { + $viewer = $this->getViewer(); + + $object_phid = $this->getObjectPHID(); + $viewer_phid = $viewer->getPHID(); + + $storage = null; + + if ($viewer_phid !== null) { + $storage = id(new DifferentialViewStateQuery()) + ->setViewer($viewer) + ->withViewerPHIDs(array($viewer_phid)) + ->withObjectPHIDs(array($object_phid)) + ->executeOne(); + } + + if ($storage === null) { + $storage = id(new DifferentialViewState()) + ->setObjectPHID($object_phid); + + if ($viewer_phid !== null) { + $storage->setViewerPHID($viewer_phid); + } else { + $storage->makeEphemeral(); + } + } + + return $storage; + } + + private function saveViewStateStorage() { + if (PhabricatorEnv::isReadOnly()) { + return; + } + + $storage = $this->getStorage(); + + $viewer_phid = $storage->getViewerPHID(); + if ($viewer_phid === null) { + return; + } + + if (!$storage->getHasModifications()) { + return; + } + + $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); + + try { + $storage->save(); + } catch (AphrontDuplicateKeyQueryException $ex) { + // We may race another process to save view state. For now, just discard + // our state if we do. + } + + unset($unguarded); + } + +}