diff --git a/conf/default.conf.php b/conf/default.conf.php index 612a853411..0cb3f9db72 100644 --- a/conf/default.conf.php +++ b/conf/default.conf.php @@ -856,17 +856,6 @@ return array( 'image/vnd.microsoft.icon' => true, ), - // Phabricator can proxy images from other servers so you can paste the URI - // to a funny picture of a cat into the comment box and have it show up as an - // image. However, this means the webserver Phabricator is running on will - // make HTTP requests to arbitrary URIs. If the server has access to internal - // resources, this could be a security risk. You should only enable it if you - // are installed entirely a VPN and VPN access is required to access - // Phabricator, or if the webserver has no special access to anything. If - // unsure, it is safer to leave this disabled. - 'files.enable-proxy' => false, - - // -- Storage --------------------------------------------------------------- // // Phabricator allows users to upload files, and can keep them in various diff --git a/resources/sql/patches/dropfileproxyimage.sql b/resources/sql/patches/dropfileproxyimage.sql new file mode 100644 index 0000000000..982c0ebf8d --- /dev/null +++ b/resources/sql/patches/dropfileproxyimage.sql @@ -0,0 +1 @@ +DROP TABLE {$NAMESPACE}_file.file_proxyimage; diff --git a/resources/sql/patches/liskcounters.php b/resources/sql/patches/liskcounters.php new file mode 100644 index 0000000000..1cfac2c99c --- /dev/null +++ b/resources/sql/patches/liskcounters.php @@ -0,0 +1,42 @@ +establishConnection('w'); + +$active_auto = head(queryfx_one( + $conn_w, + 'SELECT auto_increment FROM information_schema.tables + WHERE table_name = %s + AND table_schema = DATABASE()', + $active_table->getTableName())); + +$active_max = head(queryfx_one( + $conn_w, + 'SELECT MAX(id) FROM %T', + $active_table->getTableName())); + +$archive_max = head(queryfx_one( + $conn_w, + 'SELECT MAX(id) FROM %T', + $archive_table->getTableName())); + +$initial_counter = max((int)$active_auto, (int)$active_max, (int)$archive_max); + +queryfx( + $conn_w, + 'INSERT IGNORE INTO %T (counterName, counterValue) + VALUES (%s, %d)', + LiskDAO::COUNTER_TABLE_NAME, + $active_table->getTableName(), + $initial_counter + 1); + +// Drop AUTO_INCREMENT from the ID column. +queryfx( + $conn_w, + 'ALTER TABLE %T CHANGE id id INT UNSIGNED NOT NULL', + $active_table->getTableName()); diff --git a/resources/sql/patches/liskcounters.sql b/resources/sql/patches/liskcounters.sql new file mode 100644 index 0000000000..f617a9699f --- /dev/null +++ b/resources/sql/patches/liskcounters.sql @@ -0,0 +1,9 @@ +CREATE TABLE `{$NAMESPACE}_harbormaster`.`lisk_counter` ( + counterName VARCHAR(64) COLLATE utf8_bin PRIMARY KEY, + counterValue BIGINT UNSIGNED NOT NULL +) ENGINE=InnoDB DEFAULT CHARSET=utf8; + +CREATE TABLE `{$NAMESPACE}_worker`.`lisk_counter` ( + counterName VARCHAR(64) COLLATE utf8_bin PRIMARY KEY, + counterValue BIGINT UNSIGNED NOT NULL +) ENGINE=InnoDB DEFAULT CHARSET=utf8; diff --git a/src/__phutil_library_map__.php b/src/__phutil_library_map__.php index 62965e0e03..4bf8b5aa82 100644 --- a/src/__phutil_library_map__.php +++ b/src/__phutil_library_map__.php @@ -608,6 +608,7 @@ phutil_register_library_map(array( 'PhabricatorBarePageView' => 'view/page/PhabricatorBarePageView.php', 'PhabricatorBaseEnglishTranslation' => 'infrastructure/internationalization/PhabricatorBaseEnglishTranslation.php', 'PhabricatorBuiltinPatchList' => 'infrastructure/storage/patch/PhabricatorBuiltinPatchList.php', + 'PhabricatorButtonsExample' => 'applications/uiexample/examples/PhabricatorButtonsExample.php', 'PhabricatorCacheDAO' => 'applications/cache/storage/PhabricatorCacheDAO.php', 'PhabricatorCalendarBrowseController' => 'applications/calendar/controller/PhabricatorCalendarBrowseController.php', 'PhabricatorCalendarController' => 'applications/calendar/controller/PhabricatorCalendarController.php', @@ -744,8 +745,6 @@ phutil_register_library_map(array( 'PhabricatorFileLinkListView' => 'view/layout/PhabricatorFileLinkListView.php', 'PhabricatorFileLinkView' => 'view/layout/PhabricatorFileLinkView.php', 'PhabricatorFileListController' => 'applications/files/controller/PhabricatorFileListController.php', - 'PhabricatorFileProxyController' => 'applications/files/controller/PhabricatorFileProxyController.php', - 'PhabricatorFileProxyImage' => 'applications/files/storage/PhabricatorFileProxyImage.php', 'PhabricatorFileQuery' => 'applications/files/query/PhabricatorFileQuery.php', 'PhabricatorFileShortcutController' => 'applications/files/controller/PhabricatorFileShortcutController.php', 'PhabricatorFileSideNavView' => 'applications/files/view/PhabricatorFileSideNavView.php', @@ -987,7 +986,6 @@ phutil_register_library_map(array( 'PhabricatorRemarkupRuleObjectName' => 'infrastructure/markup/rule/PhabricatorRemarkupRuleObjectName.php', 'PhabricatorRemarkupRulePaste' => 'infrastructure/markup/rule/PhabricatorRemarkupRulePaste.php', 'PhabricatorRemarkupRulePhriction' => 'infrastructure/markup/rule/PhabricatorRemarkupRulePhriction.php', - 'PhabricatorRemarkupRuleProxyImage' => 'infrastructure/markup/rule/PhabricatorRemarkupRuleProxyImage.php', 'PhabricatorRemarkupRuleYoutube' => 'infrastructure/markup/rule/PhabricatorRemarkupRuleYoutube.php', 'PhabricatorRepository' => 'applications/repository/storage/PhabricatorRepository.php', 'PhabricatorRepositoryArcanistProject' => 'applications/repository/storage/PhabricatorRepositoryArcanistProject.php', @@ -1824,6 +1822,7 @@ phutil_register_library_map(array( 'PhabricatorBarePageView' => 'AphrontPageView', 'PhabricatorBaseEnglishTranslation' => 'PhabricatorTranslation', 'PhabricatorBuiltinPatchList' => 'PhabricatorSQLPatchList', + 'PhabricatorButtonsExample' => 'PhabricatorUIExample', 'PhabricatorCacheDAO' => 'PhabricatorLiskDAO', 'PhabricatorCalendarBrowseController' => 'PhabricatorCalendarController', 'PhabricatorCalendarController' => 'PhabricatorController', @@ -1955,8 +1954,6 @@ phutil_register_library_map(array( 'PhabricatorFileLinkListView' => 'AphrontView', 'PhabricatorFileLinkView' => 'AphrontView', 'PhabricatorFileListController' => 'PhabricatorFileController', - 'PhabricatorFileProxyController' => 'PhabricatorFileController', - 'PhabricatorFileProxyImage' => 'PhabricatorFileDAO', 'PhabricatorFileQuery' => 'PhabricatorCursorPagedPolicyAwareQuery', 'PhabricatorFileShortcutController' => 'PhabricatorFileController', 'PhabricatorFileSideNavView' => 'AphrontView', @@ -2172,7 +2169,6 @@ phutil_register_library_map(array( 'PhabricatorRemarkupRuleObjectName' => 'PhutilRemarkupRule', 'PhabricatorRemarkupRulePaste' => 'PhabricatorRemarkupRuleObjectName', 'PhabricatorRemarkupRulePhriction' => 'PhutilRemarkupRule', - 'PhabricatorRemarkupRuleProxyImage' => 'PhutilRemarkupRule', 'PhabricatorRemarkupRuleYoutube' => 'PhutilRemarkupRule', 'PhabricatorRepository' => 'PhabricatorRepositoryDAO', 'PhabricatorRepositoryArcanistProject' => 'PhabricatorRepositoryDAO', diff --git a/src/applications/differential/field/specification/DifferentialFieldSpecification.php b/src/applications/differential/field/specification/DifferentialFieldSpecification.php index d5310530f2..eb64558a94 100644 --- a/src/applications/differential/field/specification/DifferentialFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialFieldSpecification.php @@ -383,6 +383,31 @@ abstract class DifferentialFieldSpecification { return $key; } +/* -( Extending the Search Interface )------------------------------------ */ + + /** + * @task search + */ + public function shouldAddToSearchIndex() { + return false; + } + + /** + * @task search + */ + public function getValueForSearchIndex() { + throw new DifferentialFieldSpecificationIncompleteException($this); + } + + /** + * NOTE: Keys *must be* 4 characters for + * @{class:PhabricatorSearchEngineMySQL}. + * + * @task search + */ + public function getKeyForSearchIndex() { + throw new DifferentialFieldSpecificationIncompleteException($this); + } /* -( Extending Commit Messages )------------------------------------------ */ diff --git a/src/applications/differential/field/specification/DifferentialRevertPlanFieldSpecification.php b/src/applications/differential/field/specification/DifferentialRevertPlanFieldSpecification.php index 4e84e3f492..f23b39c875 100644 --- a/src/applications/differential/field/specification/DifferentialRevertPlanFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialRevertPlanFieldSpecification.php @@ -95,4 +95,16 @@ final class DifferentialRevertPlanFieldSpecification return $value; } + public function shouldAddToSearchIndex() { + return true; + } + + public function getValueForSearchIndex() { + return $this->value; + } + + public function getKeyForSearchIndex() { + return 'rpln'; + } + } diff --git a/src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php b/src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php index e47966cc02..20a11bfef7 100644 --- a/src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialSummaryFieldSpecification.php @@ -70,4 +70,16 @@ final class DifferentialSummaryFieldSpecification return $this->summary; } + public function shouldAddToSearchIndex() { + return true; + } + + public function getValueForSearchIndex() { + return $this->summary; + } + + public function getKeyForSearchIndex() { + return PhabricatorSearchField::FIELD_BODY; + } + } diff --git a/src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php b/src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php index 30238271e2..e73a715f54 100644 --- a/src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php +++ b/src/applications/differential/field/specification/DifferentialTestPlanFieldSpecification.php @@ -103,8 +103,22 @@ final class DifferentialTestPlanFieldSpecification return "TEST PLAN\n".preg_replace('/^/m', ' ', $this->plan); } + public function shouldAddToSearchIndex() { + return true; + } + + public function getValueForSearchIndex() { + return $this->plan; + } + + public function getKeyForSearchIndex() { + return 'tpln'; + } + private function isRequired() { return PhabricatorEnv::getEnvConfig('differential.require-test-plan-field'); } + + } diff --git a/src/applications/files/controller/PhabricatorFileProxyController.php b/src/applications/files/controller/PhabricatorFileProxyController.php deleted file mode 100644 index 51b1e98654..0000000000 --- a/src/applications/files/controller/PhabricatorFileProxyController.php +++ /dev/null @@ -1,54 +0,0 @@ -getRequest(); - $uri = $request->getStr('uri'); - - $proxy = id(new PhabricatorFileProxyImage())->loadOneWhere( - 'uri = %s', - $uri); - - if (!$proxy) { - // This write is fine to skip CSRF checks for, we're just building a - // cache of some remote image. - $unguarded = AphrontWriteGuard::beginScopedUnguardedWrites(); - - $file = PhabricatorFile::newFromFileDownload( - $uri, - nonempty(basename($uri), 'proxied-file')); - if ($file) { - $proxy = new PhabricatorFileProxyImage(); - $proxy->setURI($uri); - $proxy->setFilePHID($file->getPHID()); - $proxy->save(); - } - - unset($unguarded); - } - - if ($proxy) { - $file = id(new PhabricatorFile())->loadOneWhere('phid = %s', - $proxy->getFilePHID()); - if ($file) { - $view_uri = $file->getBestURI(); - } else { - $bad_phid = $proxy->getFilePHID(); - throw new Exception( - "Unable to load file with phid {$bad_phid}." - ); - } - return id(new AphrontRedirectResponse())->setURI($view_uri); - } - - return new Aphront400Response(); - } -} diff --git a/src/applications/files/storage/PhabricatorFileProxyImage.php b/src/applications/files/storage/PhabricatorFileProxyImage.php deleted file mode 100644 index 180bde5fd1..0000000000 --- a/src/applications/files/storage/PhabricatorFileProxyImage.php +++ /dev/null @@ -1,18 +0,0 @@ - false, - ) + parent::getConfiguration(); - } - - static public function getProxyImageURI($uri) { - return '/file/proxy/?uri='.phutil_escape_uri($uri); - } -} - diff --git a/src/applications/maniphest/controller/ManiphestTaskListController.php b/src/applications/maniphest/controller/ManiphestTaskListController.php index 9120dc7dad..56ca9b76f0 100644 --- a/src/applications/maniphest/controller/ManiphestTaskListController.php +++ b/src/applications/maniphest/controller/ManiphestTaskListController.php @@ -543,6 +543,7 @@ final class ManiphestTaskListController extends ManiphestController { $owner_phids, $author_phids, $project_group_phids, + $any_project_phids, array_mergev(mpull($data, 'getProjectPHIDs'))); $handles = id(new PhabricatorObjectHandleData($handle_phids)) ->loadHandles(); diff --git a/src/applications/search/constants/PhabricatorSearchField.php b/src/applications/search/constants/PhabricatorSearchField.php index 3e40debefe..24d28b6769 100644 --- a/src/applications/search/constants/PhabricatorSearchField.php +++ b/src/applications/search/constants/PhabricatorSearchField.php @@ -7,7 +7,6 @@ final class PhabricatorSearchField { const FIELD_TITLE = 'titl'; const FIELD_BODY = 'body'; - const FIELD_TEST_PLAN = 'tpln'; const FIELD_COMMENT = 'cmnt'; } diff --git a/src/applications/search/index/indexer/PhabricatorSearchDifferentialIndexer.php b/src/applications/search/index/indexer/PhabricatorSearchDifferentialIndexer.php index a12b29067a..90c0c86d1e 100644 --- a/src/applications/search/index/indexer/PhabricatorSearchDifferentialIndexer.php +++ b/src/applications/search/index/indexer/PhabricatorSearchDifferentialIndexer.php @@ -14,12 +14,23 @@ final class PhabricatorSearchDifferentialIndexer $doc->setDocumentCreated($rev->getDateCreated()); $doc->setDocumentModified($rev->getDateModified()); - $doc->addField( - PhabricatorSearchField::FIELD_BODY, - $rev->getSummary()); - $doc->addField( - PhabricatorSearchField::FIELD_TEST_PLAN, - $rev->getTestPlan()); + $aux_fields = DifferentialFieldSelector::newSelector() + ->getFieldSpecifications(); + foreach ($aux_fields as $key => $aux_field) { + if (!$aux_field->shouldAddToSearchIndex()) { + unset($aux_fields[$key]); + } + } + + $aux_fields = DifferentialAuxiliaryField::loadFromStorage( + $rev, + $aux_fields); + foreach ($aux_fields as $aux_field) { + $doc->addField( + $aux_field->getKeyForSearchIndex(), + $aux_field->getValueForSearchIndex() + ); + } $doc->addRelationship( PhabricatorSearchRelationship::RELATIONSHIP_AUTHOR, diff --git a/src/applications/uiexample/examples/PhabricatorButtonsExample.php b/src/applications/uiexample/examples/PhabricatorButtonsExample.php new file mode 100644 index 0000000000..03228412db --- /dev/null +++ b/src/applications/uiexample/examples/PhabricatorButtonsExample.php @@ -0,0 +1,45 @@ +<button> to render buttons.'; + } + + public function renderExample() { + $request = $this->getRequest(); + $user = $request->getUser(); + + $colors = array('', 'green', 'grey', 'disabled'); + $sizes = array('', 'small'); + $tags = array('a', 'button'); + + $view = array(); + foreach ($tags as $tag) { + foreach ($colors as $color) { + foreach ($sizes as $size) { + $class = implode(' ', array($color, $size)); + + if ($tag == 'a') { + $class .= ' button'; + } + + $view[] = phutil_render_tag( + $tag, + array( + 'class' => $class, + ), + phutil_escape_html(ucwords($size.' '.$color.' '.$tag))); + + $view[] = '

'; + } + } + } + + return '
'.implode('', $view).'
'; + } +} diff --git a/src/docs/userguide/remarkup.diviner b/src/docs/userguide/remarkup.diviner index 7840a67814..1b680e46af 100644 --- a/src/docs/userguide/remarkup.diviner +++ b/src/docs/userguide/remarkup.diviner @@ -307,16 +307,14 @@ Valid options are: = Embedding Media = -If you set configuration flags, you can embed media directly in text: +If you set a configuration flag, you can embed media directly in text: - - **files.enable-proxy**: allows you to paste in image URLs and have them - render inline. - **remarkup.enable-embedded-youtube**: allows you to paste in YouTube videos and have them render inline. -These options are disabled by default because they have security and/or -silliness implications, read their descriptions in ##default.conf.php## before -enabling them. +This option is disabled by default because it has security and/or +silliness implications. Read the description in ##default.conf.php## before +enabling it. = Image Macros = diff --git a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php index 0364091cb7..37c750f9e8 100644 --- a/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php +++ b/src/infrastructure/daemon/workers/storage/PhabricatorWorkerActiveTask.php @@ -11,6 +11,7 @@ final class PhabricatorWorkerActiveTask extends PhabricatorWorkerTask { public function getConfiguration() { return array( + self::CONFIG_IDS => self::IDS_COUNTER, self::CONFIG_TIMESTAMPS => false, ) + parent::getConfiguration(); } diff --git a/src/infrastructure/markup/PhabricatorMarkupEngine.php b/src/infrastructure/markup/PhabricatorMarkupEngine.php index f8bf913409..3e0273e1aa 100644 --- a/src/infrastructure/markup/PhabricatorMarkupEngine.php +++ b/src/infrastructure/markup/PhabricatorMarkupEngine.php @@ -41,7 +41,7 @@ final class PhabricatorMarkupEngine { private $objects = array(); private $viewer; - private $version = 0; + private $version = 1; /* -( Markup Pipeline )---------------------------------------------------- */ @@ -286,7 +286,6 @@ final class PhabricatorMarkupEngine { return self::newMarkupEngine( array( 'macros' => false, - 'fileproxy' => false, 'youtube' => false, )); @@ -345,7 +344,6 @@ final class PhabricatorMarkupEngine { private static function getMarkupEngineDefaultConfiguration() { return array( 'pygments' => PhabricatorEnv::getEnvConfig('pygments.enabled'), - 'fileproxy' => PhabricatorEnv::getEnvConfig('files.enable-proxy'), 'youtube' => PhabricatorEnv::getEnvConfig( 'remarkup.enable-embedded-youtube'), 'custom-inline' => array(), @@ -394,10 +392,6 @@ final class PhabricatorMarkupEngine { $rules[] = new PhutilRemarkupRuleDocumentLink(); - if ($options['fileproxy']) { - $rules[] = new PhabricatorRemarkupRuleProxyImage(); - } - if ($options['youtube']) { $rules[] = new PhabricatorRemarkupRuleYoutube(); } diff --git a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleProxyImage.php b/src/infrastructure/markup/rule/PhabricatorRemarkupRuleProxyImage.php deleted file mode 100644 index 0083a3bae7..0000000000 --- a/src/infrastructure/markup/rule/PhabricatorRemarkupRuleProxyImage.php +++ /dev/null @@ -1,45 +0,0 @@ -]@', - array($this, 'markupProxyImage'), - $text); - - $text = preg_replace_callback( - '@(?<=^|\s)(\w{3,}://\S+'.$filetypes.')(?=\s|$)@', - array($this, 'markupProxyImage'), - $text); - - return $text; - } - - public function markupProxyImage($matches) { - - $uri = PhabricatorFileProxyImage::getProxyImageURI($matches[1]); - - return $this->getEngine()->storeText( - phutil_render_tag( - 'a', - array( - 'href' => $uri, - 'target' => '_blank', - ), - phutil_render_tag( - 'img', - array( - 'src' => $uri, - 'class' => 'remarkup-proxy-image', - )))); - } - -} diff --git a/src/infrastructure/storage/lisk/LiskDAO.php b/src/infrastructure/storage/lisk/LiskDAO.php index 954191ef25..b78a90ebda 100644 --- a/src/infrastructure/storage/lisk/LiskDAO.php +++ b/src/infrastructure/storage/lisk/LiskDAO.php @@ -177,9 +177,12 @@ abstract class LiskDAO { const SERIALIZATION_PHP = 'php'; const IDS_AUTOINCREMENT = 'ids-auto'; + const IDS_COUNTER = 'ids-counter'; const IDS_PHID = 'ids-phid'; const IDS_MANUAL = 'ids-manual'; + const COUNTER_TABLE_NAME = 'lisk_counter'; + private $__dirtyFields = array(); private $__missingFields = array(); private static $processIsolationLevel = 0; @@ -327,8 +330,23 @@ abstract class LiskDAO { * Lisk objects need to have a unique identifying ID. The three mechanisms * available for generating this ID are IDS_AUTOINCREMENT (default, assumes * the ID column is an autoincrement primary key), IDS_PHID (to generate a - * unique PHID for each object) or IDS_MANUAL (you are taking full - * responsibility for ID management). + * unique PHID for each object), IDS_MANUAL (you are taking full + * responsibility for ID management), or IDS_COUNTER (see below). + * + * InnoDB does not persist the value of `auto_increment` across restarts, + * and instead initializes it to `MAX(id) + 1` during startup. This means it + * may reissue the same autoincrement ID more than once, if the row is deleted + * and then the database is restarted. To avoid this, you can set an object to + * use a counter table with IDS_COUNTER. This will generally behave like + * IDS_AUTOINCREMENT, except that the counter value will persist across + * restarts and inserts will be slightly slower. If a database stores any + * DAOs which use this mechanism, you must create a table there with this + * schema: + * + * CREATE TABLE lisk_counter ( + * counterName VARCHAR(64) COLLATE utf8_bin PRIMARY KEY, + * counterValue BIGINT UNSIGNED NOT NULL + * ) ENGINE=InnoDB DEFAULT CHARSET=utf8; * * CONFIG_TIMESTAMPS * Lisk can automatically handle keeping track of a `dateCreated' and @@ -365,7 +383,6 @@ abstract class LiskDAO { * directly access or assign protected members of your class (use the getters * and setters). * - * * @return dictionary Map of configuration options to values. * * @task config @@ -1181,7 +1198,6 @@ abstract class LiskDAO { return $this; } - /** * Internal implementation of INSERT and REPLACE. * @@ -1193,6 +1209,8 @@ abstract class LiskDAO { $this->willSaveObject(); $data = $this->getPropertyValues(); + $conn = $this->establishConnection('w'); + $id_mechanism = $this->getConfigOption(self::CONFIG_IDS); switch ($id_mechanism) { case self::IDS_AUTOINCREMENT: @@ -1204,6 +1222,17 @@ abstract class LiskDAO { unset($data[$id_key]); } break; + case self::IDS_COUNTER: + // If we are using counter IDs, assign a new ID if we don't already have + // one. + $id_key = $this->getIDKeyForUse(); + if (empty($data[$id_key])) { + $counter_name = $this->getTableName(); + $id = self::loadNextCounterID($conn, $counter_name); + $this->setID($id); + $data[$id_key] = $id; + } + break; case self::IDS_PHID: if (empty($data[$this->getIDKeyForUse()])) { $phid = $this->generatePHID(); @@ -1219,8 +1248,6 @@ abstract class LiskDAO { $this->willWriteData($data); - $conn = $this->establishConnection('w'); - $columns = array_keys($data); foreach ($data as $key => $value) { @@ -1761,4 +1788,37 @@ abstract class LiskDAO { $this->$name = $value; } + /** + * Increments a named counter and returns the next value. + * + * @param AphrontDatabaseConnection Database where the counter resides. + * @param string Counter name to create or increment. + * @return int Next counter value. + * + * @task util + */ + public static function loadNextCounterID( + AphrontDatabaseConnection $conn_w, + $counter_name) { + + // NOTE: If an insert does not touch an autoincrement row or call + // LAST_INSERT_ID(), MySQL normally does not change the value of + // LAST_INSERT_ID(). This can cause a counter's value to leak to a + // new counter if the second counter is created after the first one is + // updated. To avoid this, we insert LAST_INSERT_ID(1), to ensure the + // LAST_INSERT_ID() is always updated and always set correctly after the + // query completes. + + queryfx( + $conn_w, + 'INSERT INTO %T (counterName, counterValue) VALUES + (%s, LAST_INSERT_ID(1)) + ON DUPLICATE KEY UPDATE + counterValue = LAST_INSERT_ID(counterValue + 1)', + self::COUNTER_TABLE_NAME, + $counter_name); + + return $conn_w->getInsertID(); + } + } diff --git a/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php b/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php index 0f3b1db99d..25e0208ebb 100644 --- a/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php +++ b/src/infrastructure/storage/lisk/__tests__/LiskFixtureTestCase.php @@ -95,5 +95,40 @@ final class LiskFixtureTestCase extends PhabricatorTestCase { $this->assertEqual(true, (bool)$load->load((string)$id)); } + public function testCounters() { + $obj = new HarbormasterObject(); + $conn_w = $obj->establishConnection('w'); + + // Test that the counter bascially behaves as expected. + $this->assertEqual(1, LiskDAO::loadNextCounterID($conn_w, 'a')); + $this->assertEqual(2, LiskDAO::loadNextCounterID($conn_w, 'a')); + $this->assertEqual(3, LiskDAO::loadNextCounterID($conn_w, 'a')); + + // This first insert is primarily a test that the previous LAST_INSERT_ID() + // value does not bleed into the creation of a new counter. + $this->assertEqual(1, LiskDAO::loadNextCounterID($conn_w, 'b')); + $this->assertEqual(2, LiskDAO::loadNextCounterID($conn_w, 'b')); + + // These inserts alternate database connections. Since unit tests are + // transactional by default, we need to break out of them or we'll deadlock + // since the transactions don't normally close until we exit the test. + LiskDAO::endIsolateAllLiskEffectsToTransactions(); + try { + + $conn_1 = $obj->establishConnection('w', $force_new = true); + $conn_2 = $obj->establishConnection('w', $force_new = true); + + $this->assertEqual(1, LiskDAO::loadNextCounterID($conn_1, 'z')); + $this->assertEqual(2, LiskDAO::loadNextCounterID($conn_2, 'z')); + $this->assertEqual(3, LiskDAO::loadNextCounterID($conn_1, 'z')); + $this->assertEqual(4, LiskDAO::loadNextCounterID($conn_2, 'z')); + $this->assertEqual(5, LiskDAO::loadNextCounterID($conn_1, 'z')); + + LiskDAO::beginIsolateAllLiskEffectsToTransactions(); + } catch (Exception $ex) { + LiskDAO::beginIsolateAllLiskEffectsToTransactions(); + throw $ex; + } + } } diff --git a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php index 625572b3be..90308fee61 100644 --- a/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php +++ b/src/infrastructure/storage/patch/PhabricatorBuiltinPatchList.php @@ -1012,6 +1012,18 @@ final class PhabricatorBuiltinPatchList extends PhabricatorSQLPatchList { 'type' => 'sql', 'name' => $this->getPatchPath('drydockresourcetype.sql'), ), + 'liskcounters.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('liskcounters.sql'), + ), + 'liskcounters.php' => array( + 'type' => 'php', + 'name' => $this->getPatchPath('liskcounters.php'), + ), + 'dropfileproxyimage.sql' => array( + 'type' => 'sql', + 'name' => $this->getPatchPath('dropfileproxyimage.sql'), + ), ); }