1
0
Fork 0
mirror of https://we.phorge.it/source/phorge.git synced 2024-11-29 10:12:41 +01:00

Move Phriction internal document/content references from IDs to PHIDs

Summary:
Ref T13077. This is mostly just a small cleanup change, even though the actual change is large.

We currently reference content and document objects from one another with `contentID` and `documentID`, but this means that `contentID` must be nullable. Switching to PHIDs allows the column to be non-nullable.

This also supports reorienting some current and future transactions around PHIDs, which is preferable for the API. In particular, I'm adding a "publish version X" transaction soon, and would rather callers pass a PHID than an ID or version number, since this will make the API more consistent and powerful.

Today, `contentID` gets used as a cheaty way to order documents by (content) edit time. Since PHIDs aren't orderable and stuff is going to become actually-revertible soon, replace this with an epoch timestamp.

Test Plan:
  - Created, edited, moved, retitled, and deleted Phriction documents.
  - Grepped for `documentID` and `contentID`.
  - This probably breaks //something// but I'll be in this code for a bit and am likely to catch whatever breaks.

Reviewers: amckinley

Subscribers: PHID-OPKG-gm6ozazyms6q6i22gyam

Maniphest Tasks: T13077

Differential Revision: https://secure.phabricator.com/D19619
This commit is contained in:
epriestley 2018-08-28 16:09:24 -07:00
parent 04f8270a74
commit 64cee4a902
21 changed files with 183 additions and 122 deletions

View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_phriction.phriction_document
ADD contentPHID VARBINARY(64) NOT NULL;

View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_phriction.phriction_content
ADD documentPHID VARBINARY(64) NOT NULL;

View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_phriction.phriction_document
ADD editedEpoch INT UNSIGNED NOT NULL;

View file

@ -0,0 +1,57 @@
<?php
// Update the PhrictionDocument and PhrictionContent tables to refer to one
// another by PHID instead of by ID.
$document_table = new PhrictionDocument();
$content_table = new PhrictionContent();
$conn = $document_table->establishConnection('w');
$document_iterator = new LiskRawMigrationIterator(
$conn,
$document_table->getTableName());
foreach ($document_iterator as $row) {
$content_id = $row['contentID'];
$content_row = queryfx_one(
$conn,
'SELECT phid, dateCreated FROM %T WHERE id = %d',
$content_table->getTableName(),
$content_id);
if (!$content_row) {
continue;
}
queryfx(
$conn,
'UPDATE %T SET contentPHID = %s, editedEpoch = %d WHERE id = %d',
$document_table->getTableName(),
$content_row['phid'],
$content_row['dateCreated'],
$row['id']);
}
$content_iterator = new LiskRawMigrationIterator(
$conn,
$content_table->getTableName());
foreach ($content_iterator as $row) {
$document_id = $row['documentID'];
$document_row = queryfx_one(
$conn,
'SELECT phid FROM %T WHERE id = %d',
$document_table->getTableName(),
$document_id);
if (!$document_row) {
continue;
}
queryfx(
$conn,
'UPDATE %T SET documentPHID = %s WHERE id = %d',
$content_table->getTableName(),
$document_row['phid'],
$row['id']);
}

View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_phriction.phriction_document
DROP contentID;

View file

@ -0,0 +1,2 @@
ALTER TABLE {$NAMESPACE}_phriction.phriction_content
DROP documentID;

View file

@ -5036,6 +5036,7 @@ phutil_register_library_map(array(
'PhrictionDocumentTitleHeraldField' => 'applications/phriction/herald/PhrictionDocumentTitleHeraldField.php',
'PhrictionDocumentTitleTransaction' => 'applications/phriction/xaction/PhrictionDocumentTitleTransaction.php',
'PhrictionDocumentTransactionType' => 'applications/phriction/xaction/PhrictionDocumentTransactionType.php',
'PhrictionDocumentVersionTransaction' => 'applications/phriction/xaction/PhrictionDocumentVersionTransaction.php',
'PhrictionEditConduitAPIMethod' => 'applications/phriction/conduit/PhrictionEditConduitAPIMethod.php',
'PhrictionEditController' => 'applications/phriction/controller/PhrictionEditController.php',
'PhrictionHistoryConduitAPIMethod' => 'applications/phriction/conduit/PhrictionHistoryConduitAPIMethod.php',
@ -11129,17 +11130,17 @@ phutil_register_library_map(array(
),
'PhrictionDocumentAuthorHeraldField' => 'PhrictionDocumentHeraldField',
'PhrictionDocumentContentHeraldField' => 'PhrictionDocumentHeraldField',
'PhrictionDocumentContentTransaction' => 'PhrictionDocumentTransactionType',
'PhrictionDocumentContentTransaction' => 'PhrictionDocumentVersionTransaction',
'PhrictionDocumentController' => 'PhrictionController',
'PhrictionDocumentDatasource' => 'PhabricatorTypeaheadDatasource',
'PhrictionDocumentDeleteTransaction' => 'PhrictionDocumentTransactionType',
'PhrictionDocumentDeleteTransaction' => 'PhrictionDocumentVersionTransaction',
'PhrictionDocumentFerretEngine' => 'PhabricatorFerretEngine',
'PhrictionDocumentFulltextEngine' => 'PhabricatorFulltextEngine',
'PhrictionDocumentHeraldAdapter' => 'HeraldAdapter',
'PhrictionDocumentHeraldField' => 'HeraldField',
'PhrictionDocumentHeraldFieldGroup' => 'HeraldFieldGroup',
'PhrictionDocumentMoveAwayTransaction' => 'PhrictionDocumentTransactionType',
'PhrictionDocumentMoveToTransaction' => 'PhrictionDocumentTransactionType',
'PhrictionDocumentMoveAwayTransaction' => 'PhrictionDocumentVersionTransaction',
'PhrictionDocumentMoveToTransaction' => 'PhrictionDocumentVersionTransaction',
'PhrictionDocumentPHIDType' => 'PhabricatorPHIDType',
'PhrictionDocumentPathHeraldField' => 'PhrictionDocumentHeraldField',
'PhrictionDocumentPolicyCodex' => 'PhabricatorPolicyCodex',
@ -11148,8 +11149,9 @@ phutil_register_library_map(array(
'PhrictionDocumentSearchEngine' => 'PhabricatorApplicationSearchEngine',
'PhrictionDocumentStatus' => 'PhabricatorObjectStatus',
'PhrictionDocumentTitleHeraldField' => 'PhrictionDocumentHeraldField',
'PhrictionDocumentTitleTransaction' => 'PhrictionDocumentTransactionType',
'PhrictionDocumentTitleTransaction' => 'PhrictionDocumentVersionTransaction',
'PhrictionDocumentTransactionType' => 'PhabricatorModularTransactionType',
'PhrictionDocumentVersionTransaction' => 'PhrictionDocumentTransactionType',
'PhrictionEditConduitAPIMethod' => 'PhrictionConduitAPIMethod',
'PhrictionEditController' => 'PhrictionController',
'PhrictionHistoryConduitAPIMethod' => 'PhrictionConduitAPIMethod',

View file

@ -65,8 +65,8 @@ final class PhrictionDiffController extends PhrictionController {
$slug = $document->getSlug();
$revert_l = $this->renderRevertButton($content_l, $current);
$revert_r = $this->renderRevertButton($content_r, $current);
$revert_l = $this->renderRevertButton($document, $content_l, $current);
$revert_r = $this->renderRevertButton($document, $content_r, $current);
$crumbs = $this->buildApplicationCrumbs();
$crumb_views = $this->renderBreadcrumbs($slug);
@ -179,10 +179,11 @@ final class PhrictionDiffController extends PhrictionController {
}
private function renderRevertButton(
PhrictionDocument $document,
PhrictionContent $content,
PhrictionContent $current) {
$document_id = $content->getDocumentID();
$document_id = $document->getID();
$version = $content->getVersion();
$hidden_statuses = array(

View file

@ -78,7 +78,7 @@ final class PhrictionDocumentController
return new Aphront404Response();
}
if ($content->getID() != $document->getContentID()) {
if ($content->getPHID() != $document->getContentPHID()) {
$version_note = id(new PHUIInfoView())
->setSeverity(PHUIInfoView::SEVERITY_NOTICE)
->appendChild(

View file

@ -52,7 +52,7 @@ final class PhrictionHistoryController
}
$vs_head = null;
if ($content->getID() != $document->getContentID()) {
if ($content->getPHID() != $document->getContentPHID()) {
$vs_head = $diff_uri
->alter('l', $content->getVersion())
->alter('r', $current->getVersion());

View file

@ -93,29 +93,13 @@ final class PhrictionTransactionEditor
return $types;
}
protected function shouldApplyInitialEffects(
PhabricatorLiskDAO $object,
array $xactions) {
foreach ($xactions as $xaction) {
switch ($xaction->getTransactionType()) {
case PhrictionDocumentTitleTransaction::TRANSACTIONTYPE:
case PhrictionDocumentContentTransaction::TRANSACTIONTYPE:
case PhrictionDocumentDeleteTransaction::TRANSACTIONTYPE:
case PhrictionDocumentMoveToTransaction::TRANSACTIONTYPE:
case PhrictionDocumentMoveAwayTransaction::TRANSACTIONTYPE:
return true;
}
}
return parent::shouldApplyInitialEffects($object, $xactions);
}
protected function applyInitialEffects(
protected function expandTransactions(
PhabricatorLiskDAO $object,
array $xactions) {
$this->setOldContent($object->getContent());
$this->setNewContent($this->buildNewContentTemplate($object));
return parent::expandTransactions($object, $xactions);
}
protected function expandTransaction(
@ -148,7 +132,6 @@ final class PhrictionTransactionEditor
break;
default:
break;
}
return $xactions;
@ -158,29 +141,12 @@ final class PhrictionTransactionEditor
PhabricatorLiskDAO $object,
array $xactions) {
$save_content = false;
foreach ($xactions as $xaction) {
switch ($xaction->getTransactionType()) {
case PhrictionDocumentTitleTransaction::TRANSACTIONTYPE:
case PhrictionDocumentMoveToTransaction::TRANSACTIONTYPE:
case PhrictionDocumentMoveAwayTransaction::TRANSACTIONTYPE:
case PhrictionDocumentDeleteTransaction::TRANSACTIONTYPE:
case PhrictionDocumentContentTransaction::TRANSACTIONTYPE:
$save_content = true;
break;
default:
break;
}
}
if ($this->hasNewDocumentContent()) {
$content = $this->getNewDocumentContent($object);
if ($save_content) {
$content = $this->getNewContent();
$content->setDocumentID($object->getID());
$content->save();
$object->setContentID($content->getID());
$object->save();
$object->attachContent($content);
$content
->setDocumentPHID($object->getPHID())
->save();
}
if ($this->getIsNewObject() && !$this->getSkipAncestorCheck()) {
@ -535,24 +501,47 @@ final class PhrictionTransactionEditor
->setDocument($object);
}
private function buildNewContentTemplate(
PhrictionDocument $document) {
private function hasNewDocumentContent() {
return (bool)$this->newContent;
}
$new_content = id(new PhrictionContent())
public function getNewDocumentContent(PhrictionDocument $document) {
if (!$this->hasNewDocumentContent()) {
$content = $this->newDocumentContent($document);
// Generate a PHID now so we can populate "contentPHID" before saving
// the document to the database: the column is not nullable so we need
// a value.
$content_phid = $content->generatePHID();
$content->setPHID($content_phid);
$document->setContentPHID($content_phid);
$document->attachContent($content);
$document->setEditedEpoch(PhabricatorTime::getNow());
$this->newContent = $content;
}
return $this->newContent;
}
private function newDocumentContent(PhrictionDocument $document) {
$content = id(new PhrictionContent())
->setSlug($document->getSlug())
->setAuthorPHID($this->getActor()->getPHID())
->setAuthorPHID($this->getActingAsPHID())
->setChangeType(PhrictionChangeType::CHANGE_EDIT)
->setTitle($this->getOldContent()->getTitle())
->setContent($this->getOldContent()->getContent())
->setDescription('');
if (strlen($this->getDescription())) {
$new_content->setDescription($this->getDescription());
$content->setDescription($this->getDescription());
}
$new_content->setVersion($this->getOldContent()->getVersion() + 1);
$content->setVersion($this->getOldContent()->getVersion() + 1);
return $new_content;
return $content;
}
protected function getCustomWorkerState() {

View file

@ -76,7 +76,7 @@ final class PhrictionContentQuery
if ($this->shouldJoinDocumentTable()) {
$joins[] = qsprintf(
$conn,
'JOIN %T d ON d.id = c.documentID',
'JOIN %T d ON d.phid = c.documentPHID',
id(new PhrictionDocument())->getTableName());
}
@ -84,19 +84,19 @@ final class PhrictionContentQuery
}
protected function willFilterPage(array $contents) {
$document_ids = mpull($contents, 'getDocumentID');
$document_phids = mpull($contents, 'getDocumentPHID');
$documents = id(new PhrictionDocumentQuery())
->setViewer($this->getViewer())
->setParentQuery($this)
->withIDs($document_ids)
->withPHIDs($document_phids)
->execute();
$documents = mpull($documents, null, 'getID');
$documents = mpull($documents, null, 'getPHID');
foreach ($contents as $key => $content) {
$document_id = $content->getDocumentID();
$document_phid = $content->getDocumentPHID();
$document = idx($documents, $document_id);
$document = idx($documents, $document_phid);
if (!$document) {
unset($contents[$key]);
$this->didRejectResult($content);

View file

@ -151,17 +151,17 @@ final class PhrictionDocumentQuery
$contents = id(new PhrictionContentQuery())
->setViewer($this->getViewer())
->setParentQuery($this)
->withIDs(mpull($documents, 'getContentID'))
->withPHIDs(mpull($documents, 'getContentPHID'))
->execute();
$contents = mpull($contents, null, 'getID');
$contents = mpull($contents, null, 'getPHID');
foreach ($documents as $key => $document) {
$content_id = $document->getContentID();
if (empty($contents[$content_id])) {
$content_phid = $document->getContentPHID();
if (empty($contents[$content_phid])) {
unset($documents[$key]);
continue;
}
$document->attachContent($contents[$content_id]);
$document->attachContent($contents[$content_phid]);
}
}
@ -175,7 +175,7 @@ final class PhrictionDocumentQuery
$content_dao = new PhrictionContent();
$joins[] = qsprintf(
$conn,
'JOIN %T c ON d.contentID = c.id',
'JOIN %T c ON d.contentPHID = c.phid',
$content_dao->getTableName());
}
@ -321,7 +321,7 @@ final class PhrictionDocumentQuery
public function getBuiltinOrders() {
return parent::getBuiltinOrders() + array(
self::ORDER_HIERARCHY => array(
'vector' => array('depth', 'title', 'updated'),
'vector' => array('depth', 'title', 'updated', 'id'),
'name' => pht('Hierarchy'),
),
);
@ -343,9 +343,9 @@ final class PhrictionDocumentQuery
),
'updated' => array(
'table' => 'd',
'column' => 'contentID',
'column' => 'editedEpoch',
'type' => 'int',
'unique' => true,
'unique' => false,
),
);
}
@ -356,7 +356,7 @@ final class PhrictionDocumentQuery
$map = array(
'id' => $document->getID(),
'depth' => $document->getDepth(),
'updated' => $document->getContentID(),
'updated' => $document->getEditedEpoch(),
);
foreach ($keys as $key) {

View file

@ -7,7 +7,7 @@ final class PhrictionContent
PhabricatorDestructibleInterface,
PhabricatorConduitResultInterface {
protected $documentID;
protected $documentPHID;
protected $version;
protected $authorPHID;
@ -35,7 +35,7 @@ final class PhrictionContent
),
self::CONFIG_KEY_SCHEMA => array(
'documentID' => array(
'columns' => array('documentID', 'version'),
'columns' => array('documentPHID', 'version'),
'unique' => true,
),
'authorPHID' => array(

View file

@ -17,12 +17,13 @@ final class PhrictionDocument extends PhrictionDAO
protected $slug;
protected $depth;
protected $contentID;
protected $contentPHID;
protected $status;
protected $mailKey;
protected $viewPolicy;
protected $editPolicy;
protected $spacePHID;
protected $editedEpoch;
private $contentObject = self::ATTACHABLE;
private $ancestors = array();
@ -34,16 +35,11 @@ final class PhrictionDocument extends PhrictionDAO
self::CONFIG_COLUMN_SCHEMA => array(
'slug' => 'sort128',
'depth' => 'uint32',
'contentID' => 'id?',
'status' => 'text32',
'mailKey' => 'bytes20',
'editedEpoch' => 'epoch',
),
self::CONFIG_KEY_SCHEMA => array(
'key_phid' => null,
'phid' => array(
'columns' => array('phid'),
'unique' => true,
),
'slug' => array(
'columns' => array('slug'),
'unique' => true,
@ -56,17 +52,16 @@ final class PhrictionDocument extends PhrictionDAO
) + parent::getConfiguration();
}
public function generatePHID() {
return PhabricatorPHID::generateNewPHID(
PhrictionDocumentPHIDType::TYPECONST);
public function getPHIDType() {
return PhrictionDocumentPHIDType::TYPECONST;
}
public static function initializeNewDocument(PhabricatorUser $actor, $slug) {
$document = new PhrictionDocument();
$document->setSlug($slug);
$document = id(new self())
->setSlug($slug);
$content = new PhrictionContent();
$content->setSlug($slug);
$content = id(new PhrictionContent())
->setSlug($slug);
$default_title = PhabricatorSlug::getDefaultTitle($slug);
$content->setTitle($default_title);
@ -95,6 +90,8 @@ final class PhrictionDocument extends PhrictionDAO
->setSpacePHID($actor->getDefaultSpacePHID());
}
$document->setEditedEpoch(PhabricatorTime::getNow());
return $document;
}

View file

@ -1,7 +1,7 @@
<?php
final class PhrictionDocumentContentTransaction
extends PhrictionDocumentTransactionType {
extends PhrictionDocumentVersionTransaction {
const TRANSACTIONTYPE = 'content';
@ -18,10 +18,9 @@ final class PhrictionDocumentContentTransaction
public function applyInternalEffects($object, $value) {
$object->setStatus(PhrictionDocumentStatus::STATUS_EXISTS);
}
public function applyExternalEffects($object, $value) {
$this->getEditor()->getNewContent()->setContent($value);
$content = $this->getNewDocumentContent($object);
$content->setContent($value);
}
public function shouldHide() {

View file

@ -1,7 +1,7 @@
<?php
final class PhrictionDocumentDeleteTransaction
extends PhrictionDocumentTransactionType {
extends PhrictionDocumentVersionTransaction {
const TRANSACTIONTYPE = 'delete';
@ -11,12 +11,11 @@ final class PhrictionDocumentDeleteTransaction
public function applyInternalEffects($object, $value) {
$object->setStatus(PhrictionDocumentStatus::STATUS_DELETED);
}
public function applyExternalEffects($object, $value) {
$this->getEditor()->getNewContent()->setContent('');
$this->getEditor()->getNewContent()->setChangeType(
PhrictionChangeType::CHANGE_DELETE);
$content = $this->getNewDocumentContent($object);
$content->setContent('');
$content->setChangeType(PhrictionChangeType::CHANGE_DELETE);
}
public function getActionStrength() {

View file

@ -1,7 +1,7 @@
<?php
final class PhrictionDocumentMoveAwayTransaction
extends PhrictionDocumentTransactionType {
extends PhrictionDocumentVersionTransaction {
const TRANSACTIONTYPE = 'move-away';
@ -22,14 +22,12 @@ final class PhrictionDocumentMoveAwayTransaction
public function applyInternalEffects($object, $value) {
$object->setStatus(PhrictionDocumentStatus::STATUS_MOVED);
}
public function applyExternalEffects($object, $value) {
$dict = $value;
$this->getEditor()->getNewContent()->setContent('');
$this->getEditor()->getNewContent()->setChangeType(
PhrictionChangeType::CHANGE_MOVE_AWAY);
$this->getEditor()->getNewContent()->setChangeRef($dict['id']);
$content = $this->getNewDocumentContent($object);
$content->setContent('');
$content->setChangeType(PhrictionChangeType::CHANGE_MOVE_AWAY);
$content->setChangeRef($value['id']);
}
public function getActionName() {

View file

@ -1,7 +1,7 @@
<?php
final class PhrictionDocumentMoveToTransaction
extends PhrictionDocumentTransactionType {
extends PhrictionDocumentVersionTransaction {
const TRANSACTIONTYPE = 'move-to';
@ -11,6 +11,7 @@ final class PhrictionDocumentMoveToTransaction
public function generateNewValue($object, $value) {
$document = $value;
$dict = array(
'id' => $document->getID(),
'phid' => $document->getPHID(),
@ -26,15 +27,13 @@ final class PhrictionDocumentMoveToTransaction
public function applyInternalEffects($object, $value) {
$object->setStatus(PhrictionDocumentStatus::STATUS_EXISTS);
}
public function applyExternalEffects($object, $value) {
$dict = $value;
$this->getEditor()->getNewContent()->setContent($dict['content']);
$this->getEditor()->getNewContent()->setTitle($dict['title']);
$this->getEditor()->getNewContent()->setChangeType(
PhrictionChangeType::CHANGE_MOVE_HERE);
$this->getEditor()->getNewContent()->setChangeRef($dict['id']);
$content = $this->getNewDocumentContent($object);
$content->setContent($value['content']);
$content->setTitle($value['title']);
$content->setChangeType(PhrictionChangeType::CHANGE_MOVE_HERE);
$content->setChangeRef($value['id']);
}
public function getActionStrength() {

View file

@ -1,7 +1,7 @@
<?php
final class PhrictionDocumentTitleTransaction
extends PhrictionDocumentTransactionType {
extends PhrictionDocumentVersionTransaction {
const TRANSACTIONTYPE = 'title';
@ -14,10 +14,10 @@ final class PhrictionDocumentTitleTransaction
public function applyInternalEffects($object, $value) {
$object->setStatus(PhrictionDocumentStatus::STATUS_EXISTS);
}
public function applyExternalEffects($object, $value) {
$this->getEditor()->getNewContent()->setTitle($value);
$content = $this->getNewDocumentContent($object);
$content->setTitle($value);
}
public function getActionStrength() {

View file

@ -0,0 +1,10 @@
<?php
abstract class PhrictionDocumentVersionTransaction
extends PhrictionDocumentTransactionType {
protected function getNewDocumentContent($object) {
return $this->getEditor()->getNewDocumentContent($object);
}
}