From ba3f169d7e0951b9ca051be97be05b0b5fc35cbe Mon Sep 17 00:00:00 2001 From: Tim Otten Date: Mon, 23 May 2022 14:58:18 -0700 Subject: [PATCH] CRM/Upgrade - Snapshots should have name `civicrm_snap_{OWNER}_{VERSION}_{NAME}` --- CRM/Upgrade/Form.php | 2 +- CRM/Upgrade/Incremental/Base.php | 8 +-- CRM/Upgrade/Snapshot.php | 51 ++++++++++++---- tests/phpunit/CRM/Upgrade/SnapshotTest.php | 68 +++++++++++++++------- 4 files changed, 91 insertions(+), 38 deletions(-) diff --git a/CRM/Upgrade/Form.php b/CRM/Upgrade/Form.php index cc8c8d4352..7e9a706024 100644 --- a/CRM/Upgrade/Form.php +++ b/CRM/Upgrade/Form.php @@ -539,7 +539,7 @@ SET version = '$version' if (empty(CRM_Upgrade_Snapshot::getActivationIssues())) { $task = new CRM_Queue_Task( ['CRM_Upgrade_Snapshot', 'cleanupTask'], - [], + ['core'], "Cleanup old upgrade snapshots" ); $queue->createItem($task, ['weight' => 0]); diff --git a/CRM/Upgrade/Incremental/Base.php b/CRM/Upgrade/Incremental/Base.php index b4e3b567ca..f178187125 100644 --- a/CRM/Upgrade/Incremental/Base.php +++ b/CRM/Upgrade/Incremental/Base.php @@ -172,9 +172,9 @@ class CRM_Upgrade_Incremental_Base { * @throws \CRM_Core_Exception */ protected function addSnapshotTask(string $name, CRM_Utils_SQL_Select $select): void { - if (!preg_match(';^[a-z0-9_]+$;', $name)) { - throw new \CRM_Core_Exception("Malformed snapshot name: $name"); - } + CRM_Upgrade_Snapshot::createTableName('core', $this->getMajorMinor(), $name); + // ^^ To simplify QA -- we should always throw an exception for bad snapshot names, even if the local policy doesn't use snapshots. + if (!empty(CRM_Upgrade_Snapshot::getActivationIssues())) { return; } @@ -183,7 +183,7 @@ class CRM_Upgrade_Incremental_Base { 'type' => 'Sql', 'name' => CRM_Upgrade_Form::QUEUE_NAME, ]); - foreach (CRM_Upgrade_Snapshot::createTasks($this->getMajorMinor(), $name, $select) as $task) { + foreach (CRM_Upgrade_Snapshot::createTasks('core', $this->getMajorMinor(), $name, $select) as $task) { $queue->createItem($task, ['weight' => -1]); } } diff --git a/CRM/Upgrade/Snapshot.php b/CRM/Upgrade/Snapshot.php index 442aed6de1..b1c263aa8e 100644 --- a/CRM/Upgrade/Snapshot.php +++ b/CRM/Upgrade/Snapshot.php @@ -79,28 +79,53 @@ class CRM_Upgrade_Snapshot { /** * Create the name of a MySQL snapshot table. * + * @param string $owner + * Name of the component/module/extension that owns the snapshot. + * Ex: 'core', 'sequentialcreditnotes', 'oauth_client' * @param string $version * Ex: '5.50' * @param string $name * Ex: 'dates' * @return string * Ex: 'civicrm_snap_v5_50_dates' + * @throws \CRM_Core_Exception + * If the resulting table name would be invalid, then this throws an exception. */ - public static function createTableName(string $version, string $name): string { - [$major, $minor] = explode('.', $version); - return sprintf('civicrm_snap_v%s_%s_%s', $major, $minor, $name); + public static function createTableName(string $owner, string $version, string $name): string { + $versionParts = explode('.', $version); + if (count($versionParts) !== 2) { + throw new \CRM_Core_Exception("Snapshot support is currently only defined for two-part version (MAJOR.MINOR). Found ($version)."); + // If you change this, be sure to consider `cleanupTask()` as well. + // One reason you might change it -- if you were going to track with the internal schema-numbers from an extension. + // Of course, you could get similar effect with "0.{$schemaNumber}" eg "5002" ==> "0.5002" + } + $versionExpr = ($versionParts[0] . '_' . $versionParts[1]); + + $table = sprintf('civicrm_snap_%s_v%s_%s', $owner, $versionExpr, $name); + if (!preg_match(';^[a-z0-9_]+$;', $table)) { + throw new CRM_Core_Exception("Malformed snapshot name ($table)"); + } + if (strlen($table) > 64) { + throw new CRM_Core_Exception("Snapshot name is too long ($table)"); + } + + return $table; } /** * Build a set of queueable tasks which will store a snapshot. * + * @param string $owner + * Name of the component/module/extension that owns the snapshot. + * Ex: 'core', 'sequentialcreditnotes', 'oauth_client' * @param string $version + * Ex: '5.50' * @param string $name * @param \CRM_Utils_SQL_Select $select * @throws \CRM_Core_Exception */ - public static function createTasks(string $version, string $name, CRM_Utils_SQL_Select $select): iterable { - $destTable = static::createTableName($version, $name); + public static function createTasks(string $owner, string $version, string $name, CRM_Utils_SQL_Select $select): iterable { + $destTable = static::createTableName($owner, $version, $name); $srcTable = \Civi\Test\Invasive::get([$select, 'from']); // Sometimes, backups fail and people rollback and try again. Reset prior snapshots. @@ -144,6 +169,8 @@ class CRM_Upgrade_Snapshot { * Cleanup any old snapshot tables. * * @param CRM_Queue_TaskContext|null $ctx + * @param string $owner + * Ex: 'core', 'sequentialcreditnotes', 'oauth_client' * @param string|null $version * The current version of CiviCRM. * @param int|null $cleanupAfter @@ -151,7 +178,7 @@ class CRM_Upgrade_Snapshot { * Time is measured in terms of MINOR versions - eg "4" means "retain for 4 MINOR versions". * Thus, on v5.60, you could delete any snapshots predating 5.56. */ - public static function cleanupTask(?CRM_Queue_TaskContext $ctx = NULL, ?string $version = NULL, ?int $cleanupAfter = NULL): void { + public static function cleanupTask(?CRM_Queue_TaskContext $ctx = NULL, string $owner = 'core', ?string $version = NULL, ?int $cleanupAfter = NULL): void { $version = $version ?: CRM_Core_BAO_Domain::version(); $cleanupAfter = $cleanupAfter ?: static::$cleanupAfter; @@ -163,13 +190,15 @@ class CRM_Upgrade_Snapshot { SELECT TABLE_NAME as tableName FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_SCHEMA = %1 - AND TABLE_NAME LIKE 'civicrm_snap_v%' + AND TABLE_NAME LIKE %2 "; - $tables = CRM_Core_DAO::executeQuery($query, [1 => [$dao->database(), 'String']]) - ->fetchMap('tableName', 'tableName'); + $tables = CRM_Core_DAO::executeQuery($query, [ + 1 => [$dao->database(), 'String'], + 2 => ["civicrm_snap_{$owner}_v%", 'String'], + ])->fetchMap('tableName', 'tableName'); - $oldTables = array_filter($tables, function($table) use ($cutoff) { - if (preg_match(';^civicrm_snap_v(\d+)_(\d+)_;', $table, $m)) { + $oldTables = array_filter($tables, function($table) use ($owner, $cutoff) { + if (preg_match(";^civicrm_snap_{$owner}_v(\d+)_(\d+)_;", $table, $m)) { $generatedVer = $m[1] . '.' . $m[2]; return (bool) version_compare($generatedVer, $cutoff, '<'); } diff --git a/tests/phpunit/CRM/Upgrade/SnapshotTest.php b/tests/phpunit/CRM/Upgrade/SnapshotTest.php index 744b5f9b70..5a99fa0bb2 100644 --- a/tests/phpunit/CRM/Upgrade/SnapshotTest.php +++ b/tests/phpunit/CRM/Upgrade/SnapshotTest.php @@ -12,6 +12,30 @@ class CRM_Upgrade_SnapshotTest extends CiviUnitTestCase { CRM_Upgrade_Snapshot::$cleanupAfter = 4; } + public function testTableNames_good() { + $this->assertEquals('civicrm_snap_core_v5_45_stuff', CRM_Upgrade_Snapshot::createTableName('core', '5.45', 'stuff')); + $this->assertEquals('civicrm_snap_core_v5_50_stuffy_things', CRM_Upgrade_Snapshot::createTableName('core', '5.50', 'stuffy_things')); + $this->assertEquals('civicrm_snap_oauth_client_v12_34_ext_things', CRM_Upgrade_Snapshot::createTableName('oauth_client', '12.34', 'ext_things')); + $this->assertEquals('civicrm_snap_oauth_client_v0_1234_ext_things', CRM_Upgrade_Snapshot::createTableName('oauth_client', '0.1234', 'ext_things')); + } + + public function testTableNames_bad() { + try { + CRM_Upgrade_Snapshot::createTableName('core', '5.45', 'ab&cd'); + $this->fail('Accepted invalid name'); + } + catch (CRM_Core_Exception $e) { + $this->assertRegExp('/Malformed snapshot name/', $e->getMessage()); + } + try { + CRM_Upgrade_Snapshot::createTableName('core', '5.45', 'loremipsumdolorsitametconsecteturadipiscingelitseddoeiusmod'); + $this->fail('Accepted excessive name'); + } + catch (CRM_Core_Exception $e) { + $this->assertRegExp('/Snapshot name is too long/', $e->getMessage()); + } + } + /** * This example creates a snapshot based on particular sliver of data (ie * the "display_name" and "sort_name" for "Individual" records). It ensures that: @@ -26,25 +50,25 @@ class CRM_Upgrade_SnapshotTest extends CiviUnitTestCase { $this->organizationCreate([], $i); } - $this->runAll(CRM_Upgrade_Snapshot::createTasks('5.45', 'names', CRM_Utils_SQL_Select::from('civicrm_contact') + $this->runAll(CRM_Upgrade_Snapshot::createTasks('core', '5.45', 'names', CRM_Utils_SQL_Select::from('civicrm_contact') ->select('id, display_name, sort_name, modified_date') ->where('contact_type = "Individual"') )); - $this->assertTrue(CRM_Core_DAO::checkTableExists('civicrm_snap_v5_45_names')); - $this->assertSameSchema('civicrm_contact.display_name', 'civicrm_snap_v5_45_names.display_name'); - $this->assertSameSchema('civicrm_contact.sort_name', 'civicrm_snap_v5_45_names.sort_name'); - $this->assertSameSchema('civicrm_contact.modified_date', 'civicrm_snap_v5_45_names.modified_date'); + $this->assertTrue(CRM_Core_DAO::checkTableExists('civicrm_snap_core_v5_45_names')); + $this->assertSameSchema('civicrm_contact.display_name', 'civicrm_snap_core_v5_45_names.display_name'); + $this->assertSameSchema('civicrm_contact.sort_name', 'civicrm_snap_core_v5_45_names.sort_name'); + $this->assertSameSchema('civicrm_contact.modified_date', 'civicrm_snap_core_v5_45_names.modified_date'); $this->assertTrue(CRM_Core_BAO_SchemaHandler::checkIfFieldExists('civicrm_contact', 'legal_name')); - $this->assertFalse(CRM_Core_BAO_SchemaHandler::checkIfFieldExists('civicrm_snap_v5_45_names', 'legal_name')); + $this->assertFalse(CRM_Core_BAO_SchemaHandler::checkIfFieldExists('civicrm_snap_core_v5_45_names', 'legal_name')); $liveContacts = CRM_Core_DAO::singleValueQuery('SELECT count(*) FROM civicrm_contact'); $liveIndividuals = CRM_Core_DAO::singleValueQuery('SELECT count(*) FROM civicrm_contact WHERE contact_type = "Individual"'); - $snapCount = CRM_Core_DAO::singleValueQuery('SELECT count(*) FROM civicrm_snap_v5_45_names'); + $snapCount = CRM_Core_DAO::singleValueQuery('SELECT count(*) FROM civicrm_snap_core_v5_45_names'); $this->assertEquals($liveIndividuals, $snapCount, 'The snapshot should have as many records as live table.'); $this->assertTrue($liveContacts > $liveIndividuals); $this->assertGreaterThan(CRM_Upgrade_Snapshot::$pageSize, $snapCount, "There should be more than 1 page of data in the snapshot. Found $snapCount records."); - CRM_Core_DAO::executeQuery('DROP TABLE civicrm_snap_v5_45_names'); + CRM_Core_DAO::executeQuery('DROP TABLE civicrm_snap_core_v5_45_names'); } /** @@ -59,28 +83,28 @@ class CRM_Upgrade_SnapshotTest extends CiviUnitTestCase { $this->eventCreate([]); $this->eventCreate([]); - $this->runAll(CRM_Upgrade_Snapshot::createTasks('5.45', 'names', CRM_Utils_SQL_Select::from('civicrm_contact') + $this->runAll(CRM_Upgrade_Snapshot::createTasks('core', '5.45', 'names', CRM_Utils_SQL_Select::from('civicrm_contact') ->select('id, display_name, sort_name') ->where('contact_type = "Individual"') )); - $this->assertTrue(CRM_Core_DAO::checkTableExists('civicrm_snap_v5_45_names')); - $this->assertSameSchema('civicrm_contact.display_name', 'civicrm_snap_v5_45_names.display_name'); - $this->assertGreaterThan(1, CRM_Core_DAO::singleValueQuery('SELECT count(*) FROM civicrm_snap_v5_45_names')); + $this->assertTrue(CRM_Core_DAO::checkTableExists('civicrm_snap_core_v5_45_names')); + $this->assertSameSchema('civicrm_contact.display_name', 'civicrm_snap_core_v5_45_names.display_name'); + $this->assertGreaterThan(1, CRM_Core_DAO::singleValueQuery('SELECT count(*) FROM civicrm_snap_core_v5_45_names')); - $this->runAll(CRM_Upgrade_Snapshot::createTasks('5.50', 'dates', CRM_Utils_SQL_Select::from('civicrm_event') + $this->runAll(CRM_Upgrade_Snapshot::createTasks('core', '5.50', 'dates', CRM_Utils_SQL_Select::from('civicrm_event') ->select('id, start_date, end_date, registration_start_date, registration_end_date') )); - $this->assertTrue(CRM_Core_DAO::checkTableExists('civicrm_snap_v5_50_dates')); - $this->assertSameSchema('civicrm_event.start_date', 'civicrm_snap_v5_50_dates.start_date'); - $this->assertGreaterThan(1, CRM_Core_DAO::singleValueQuery('SELECT count(*) FROM civicrm_snap_v5_50_dates')); + $this->assertTrue(CRM_Core_DAO::checkTableExists('civicrm_snap_core_v5_50_dates')); + $this->assertSameSchema('civicrm_event.start_date', 'civicrm_snap_core_v5_50_dates.start_date'); + $this->assertGreaterThan(1, CRM_Core_DAO::singleValueQuery('SELECT count(*) FROM civicrm_snap_core_v5_50_dates')); - CRM_Upgrade_Snapshot::cleanupTask(NULL, '5.52', 6); - $this->assertFalse(CRM_Core_DAO::checkTableExists('civicrm_snap_v5_45_names')); - $this->assertTrue(CRM_Core_DAO::checkTableExists('civicrm_snap_v5_50_dates')); + CRM_Upgrade_Snapshot::cleanupTask(NULL, 'core', '5.52', 6); + $this->assertFalse(CRM_Core_DAO::checkTableExists('civicrm_snap_core_v5_45_names')); + $this->assertTrue(CRM_Core_DAO::checkTableExists('civicrm_snap_core_v5_50_dates')); - CRM_Upgrade_Snapshot::cleanupTask(NULL, '5.58', 6); - $this->assertFalse(CRM_Core_DAO::checkTableExists('civicrm_snap_v5_45_names')); - $this->assertFalse(CRM_Core_DAO::checkTableExists('civicrm_snap_v5_50_dates')); + CRM_Upgrade_Snapshot::cleanupTask(NULL, 'core', '5.58', 6); + $this->assertFalse(CRM_Core_DAO::checkTableExists('civicrm_snap_core_v5_45_names')); + $this->assertFalse(CRM_Core_DAO::checkTableExists('civicrm_snap_core_v5_50_dates')); } /** -- 2.25.1