From 29fd3f136b88f25f4c80414f26ffddfbf20b6881 Mon Sep 17 00:00:00 2001 From: epriestley Date: Thu, 19 Feb 2015 10:37:17 -0800 Subject: [PATCH] Allow columns to be marked as nonmutable (so save() will not change them) Summary: Ref T6840. This feels a little dirty; open to alternate suggestions. We currently have a race condition where multiple daemons may load a commit and then save it at the same time, when processing "reverts X" text. Prior to this feature, two daemons would never load a commit at the same time. The "reverts X" load/save has no effect (doesn't change any object properties), but it will set the state back to the loaded state on save(). This overwrites any flag updates made to the commit in the meantime, and can produce the race in T6840. In other cases (triggers, harbormaster, repositories) we deal with this kind of problem with "append-only-updates + single-consumer", or a bunch of locking. There isn't really a good place to add a single consumer for commits, since a lot of daemons need to access them. We could move the flags column to a separate table, but this feels pretty complicated. And locking is messy, also mostly because we have so many consumers. Just exempting this column (which has unusual behavior) from `save()` feels OK-ish? I don't know if we'll have other use cases for this, and I like it even less if we never do, but this patch is pretty small and feels fairly understandable (that said, I also don't like that it can make some properties just silently not update if you aren't on the lookout). So, this is //a// fix, and feels simplest/least-bad for the moment to me, I thiiink. Test Plan: Added and executed unit tests. Reviewers: btrahan Reviewed By: btrahan Subscribers: epriestley Maniphest Tasks: T6840 Differential Revision: https://secure.phabricator.com/D11822 --- .../20150219.scratch.nonmutable.sql | 2 ++ .../PhabricatorSecurityConfigOptions.php | 2 +- .../storage/HarbormasterScratchTable.php | 5 ++++ src/infrastructure/storage/lisk/LiskDAO.php | 17 ++++++++++++ .../lisk/__tests__/LiskFixtureTestCase.php | 26 +++++++++++++++++++ 5 files changed, 51 insertions(+), 1 deletion(-) create mode 100644 resources/sql/autopatches/20150219.scratch.nonmutable.sql diff --git a/resources/sql/autopatches/20150219.scratch.nonmutable.sql b/resources/sql/autopatches/20150219.scratch.nonmutable.sql new file mode 100644 index 0000000000..96dc08a7ce --- /dev/null +++ b/resources/sql/autopatches/20150219.scratch.nonmutable.sql @@ -0,0 +1,2 @@ +ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_scratchtable + ADD nonmutableData VARCHAR(64) COLLATE {$COLLATE_TEXT}; diff --git a/src/applications/config/option/PhabricatorSecurityConfigOptions.php b/src/applications/config/option/PhabricatorSecurityConfigOptions.php index 7fdb0bbf32..fc707b9b15 100644 --- a/src/applications/config/option/PhabricatorSecurityConfigOptions.php +++ b/src/applications/config/option/PhabricatorSecurityConfigOptions.php @@ -241,7 +241,7 @@ final class PhabricatorSecurityConfigOptions "\n\n". 'Do not enable this option if you serve (or plan to ever serve) '. 'unsecured content over plain HTTP. It is very difficult to '. - 'undo this change once users browsers have accepted the '. + 'undo this change once users\' browsers have accepted the '. 'setting.')), $this->newOption('security.allow-conduit-act-as-user', 'bool', false) ->setBoolOptions( diff --git a/src/applications/harbormaster/storage/HarbormasterScratchTable.php b/src/applications/harbormaster/storage/HarbormasterScratchTable.php index 53a118d828..6fe9dea191 100644 --- a/src/applications/harbormaster/storage/HarbormasterScratchTable.php +++ b/src/applications/harbormaster/storage/HarbormasterScratchTable.php @@ -10,18 +10,23 @@ final class HarbormasterScratchTable extends HarbormasterDAO { protected $data; protected $bigData; + protected $nonmutableData; protected function getConfiguration() { return array( self::CONFIG_COLUMN_SCHEMA => array( 'data' => 'text64', 'bigData' => 'text?', + 'nonmutableData' => 'text64?', ), self::CONFIG_KEY_SCHEMA => array( 'data' => array( 'columns' => array('data'), ), ), + self::CONFIG_NO_MUTATE => array( + 'nonmutableData', + ), ) + parent::getConfiguration(); } diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php index b3c2e351bc..6db6578e1a 100644 --- a/src/infrastructure/storage/lisk/LiskDAO.php +++ b/src/infrastructure/storage/lisk/LiskDAO.php @@ -172,6 +172,7 @@ abstract class LiskDAO { const CONFIG_COLUMN_SCHEMA = 'col-schema'; const CONFIG_KEY_SCHEMA = 'key-schema'; const CONFIG_NO_TABLE = 'no-table'; + const CONFIG_NO_MUTATE = 'no-mutate'; const SERIALIZATION_NONE = 'id'; const SERIALIZATION_JSON = 'json'; @@ -356,6 +357,13 @@ abstract class LiskDAO { * Allows you to specify that this object does not actually have a table in * the database. * + * CONFIG_NO_MUTATE + * Provide a map of columns which should not be included in UPDATE statements. + * If you have some columns which are always written to explicitly and should + * never be overwritten by a save(), you can specify them here. This is an + * advanced, specialized feature and there are usually better approaches for + * most locking/contention problems. + * * @return dictionary Map of configuration options to values. * * @task config @@ -1084,6 +1092,15 @@ abstract class LiskDAO { $this->willSaveObject(); $data = $this->getAllLiskPropertyValues(); + + // Remove colums flagged as nonmutable from the update statement. + $no_mutate = $this->getConfigOption(self::CONFIG_NO_MUTATE); + if ($no_mutate) { + foreach ($no_mutate as $column) { + unset($data[$column]); + } + } + $this->willWriteData($data); $map = array(); diff --git a/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php b/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php index 843ea89d70..c4bd744abc 100644 --- a/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php +++ b/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php @@ -137,4 +137,30 @@ final class LiskFixtureTestCase extends PhabricatorTestCase { } } + public function testNonmutableColumns() { + $object = id(new HarbormasterScratchTable()) + ->setData('val1') + ->setNonmutableData('val1') + ->save(); + + $object->reload(); + + $this->assertEqual('val1', $object->getData()); + $this->assertEqual('val1', $object->getNonmutableData()); + + $object + ->setData('val2') + ->setNonmutableData('val2') + ->save(); + + $object->reload(); + + $this->assertEqual('val2', $object->getData()); + + // NOTE: This is the important test: the nonmutable column should not have + // been affected by the update. + $this->assertEqual('val1', $object->getNonmutableData()); + } + + }