recaptime.dev / phorge
@recaptime-dev's working patches + fork for Phorge, a community fork of Phabricator. (Upstream dev and stable branches are at upstream/main and upstream/stable respectively.) hq.recaptime.dev/wiki/Phorge
phorge phabricator
1
fork

Configure Feed

Select the types of activity you want to include in your feed.

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

epriestley 29fd3f13 6a60b8cb

+51 -1
+2
resources/sql/autopatches/20150219.scratch.nonmutable.sql
··· 1 + ALTER TABLE {$NAMESPACE}_harbormaster.harbormaster_scratchtable 2 + ADD nonmutableData VARCHAR(64) COLLATE {$COLLATE_TEXT};
+1 -1
src/applications/config/option/PhabricatorSecurityConfigOptions.php
··· 241 241 "\n\n". 242 242 'Do not enable this option if you serve (or plan to ever serve) '. 243 243 'unsecured content over plain HTTP. It is very difficult to '. 244 - 'undo this change once users browsers have accepted the '. 244 + 'undo this change once users\' browsers have accepted the '. 245 245 'setting.')), 246 246 $this->newOption('security.allow-conduit-act-as-user', 'bool', false) 247 247 ->setBoolOptions(
+5
src/applications/harbormaster/storage/HarbormasterScratchTable.php
··· 10 10 11 11 protected $data; 12 12 protected $bigData; 13 + protected $nonmutableData; 13 14 14 15 protected function getConfiguration() { 15 16 return array( 16 17 self::CONFIG_COLUMN_SCHEMA => array( 17 18 'data' => 'text64', 18 19 'bigData' => 'text?', 20 + 'nonmutableData' => 'text64?', 19 21 ), 20 22 self::CONFIG_KEY_SCHEMA => array( 21 23 'data' => array( 22 24 'columns' => array('data'), 23 25 ), 26 + ), 27 + self::CONFIG_NO_MUTATE => array( 28 + 'nonmutableData', 24 29 ), 25 30 ) + parent::getConfiguration(); 26 31 }
+17
src/infrastructure/storage/lisk/LiskDAO.php
··· 172 172 const CONFIG_COLUMN_SCHEMA = 'col-schema'; 173 173 const CONFIG_KEY_SCHEMA = 'key-schema'; 174 174 const CONFIG_NO_TABLE = 'no-table'; 175 + const CONFIG_NO_MUTATE = 'no-mutate'; 175 176 176 177 const SERIALIZATION_NONE = 'id'; 177 178 const SERIALIZATION_JSON = 'json'; ··· 355 356 * CONFIG_NO_TABLE 356 357 * Allows you to specify that this object does not actually have a table in 357 358 * the database. 359 + * 360 + * CONFIG_NO_MUTATE 361 + * Provide a map of columns which should not be included in UPDATE statements. 362 + * If you have some columns which are always written to explicitly and should 363 + * never be overwritten by a save(), you can specify them here. This is an 364 + * advanced, specialized feature and there are usually better approaches for 365 + * most locking/contention problems. 358 366 * 359 367 * @return dictionary Map of configuration options to values. 360 368 * ··· 1084 1092 1085 1093 $this->willSaveObject(); 1086 1094 $data = $this->getAllLiskPropertyValues(); 1095 + 1096 + // Remove colums flagged as nonmutable from the update statement. 1097 + $no_mutate = $this->getConfigOption(self::CONFIG_NO_MUTATE); 1098 + if ($no_mutate) { 1099 + foreach ($no_mutate as $column) { 1100 + unset($data[$column]); 1101 + } 1102 + } 1103 + 1087 1104 $this->willWriteData($data); 1088 1105 1089 1106 $map = array();
+26
src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php
··· 137 137 } 138 138 } 139 139 140 + public function testNonmutableColumns() { 141 + $object = id(new HarbormasterScratchTable()) 142 + ->setData('val1') 143 + ->setNonmutableData('val1') 144 + ->save(); 145 + 146 + $object->reload(); 147 + 148 + $this->assertEqual('val1', $object->getData()); 149 + $this->assertEqual('val1', $object->getNonmutableData()); 150 + 151 + $object 152 + ->setData('val2') 153 + ->setNonmutableData('val2') 154 + ->save(); 155 + 156 + $object->reload(); 157 + 158 + $this->assertEqual('val2', $object->getData()); 159 + 160 + // NOTE: This is the important test: the nonmutable column should not have 161 + // been affected by the update. 162 + $this->assertEqual('val1', $object->getNonmutableData()); 163 + } 164 + 165 + 140 166 }