From 16142ce23ebd884817ab38836f4800dc310d14d7 Mon Sep 17 00:00:00 2001 From: Seamus Lee Date: Fri, 23 Mar 2018 15:38:40 +1100 Subject: [PATCH] Fix Core/Dev 18# where logging fails if the AUTO INCREMENT column is not named id and add tests --- CRM/Logging/Schema.php | 48 ++++++++++++------------ tests/phpunit/CRM/Logging/SchemaTest.php | 43 ++++++++++++++++++++- 2 files changed, 65 insertions(+), 26 deletions(-) diff --git a/CRM/Logging/Schema.php b/CRM/Logging/Schema.php index 3ecdfc01d8..e9ec1b44ba 100644 --- a/CRM/Logging/Schema.php +++ b/CRM/Logging/Schema.php @@ -572,7 +572,7 @@ AND (TABLE_NAME LIKE 'log_civicrm_%' $nonStandardTableNameString ) // NOTE: W.r.t Performance using one query to find all details and storing in static array is much faster // than firing query for every given table. $query = " -SELECT TABLE_SCHEMA, TABLE_NAME, COLUMN_NAME, DATA_TYPE, IS_NULLABLE, COLUMN_DEFAULT, COLUMN_TYPE +SELECT TABLE_SCHEMA, TABLE_NAME, COLUMN_NAME, DATA_TYPE, IS_NULLABLE, COLUMN_DEFAULT, COLUMN_TYPE, EXTRA FROM INFORMATION_SCHEMA.COLUMNS WHERE table_schema IN ('{$this->db}', '{$civiDB}')"; $dao = CRM_Core_DAO::executeQuery($query); @@ -588,6 +588,7 @@ WHERE table_schema IN ('{$this->db}', '{$civiDB}')"; 'DATA_TYPE' => $dao->DATA_TYPE, 'IS_NULLABLE' => $dao->IS_NULLABLE, 'COLUMN_DEFAULT' => $dao->COLUMN_DEFAULT, + 'EXTRA' => $dao->EXTRA, ); if (($first = strpos($dao->COLUMN_TYPE, '(')) != 0) { \Civi::$statics[__CLASS__]['columnSpecs'][$dao->TABLE_NAME][$dao->COLUMN_NAME]['LENGTH'] = substr( @@ -612,10 +613,8 @@ WHERE table_schema IN ('{$this->db}', '{$civiDB}')"; $logTableSpecs = $this->columnSpecsOf($logTable); $diff = array('ADD' => array(), 'MODIFY' => array(), 'OBSOLETE' => array()); - // columns to be added $diff['ADD'] = array_diff(array_keys($civiTableSpecs), array_keys($logTableSpecs)); - // columns to be modified // NOTE: we consider only those columns for modifications where there is a spec change, and that the column definition // wasn't deliberately modified by fixTimeStampAndNotNullSQL() method. @@ -623,28 +622,29 @@ WHERE table_schema IN ('{$this->db}', '{$civiDB}')"; if (!isset($logTableSpecs[$col]) || !is_array($logTableSpecs[$col])) { $logTableSpecs[$col] = array(); } - $specDiff = array_diff($civiTableSpecs[$col], $logTableSpecs[$col]); - if (!empty($specDiff) && $col != 'id' && !array_key_exists($col, $diff['ADD'])) { - // ignore 'id' column for any spec changes, to avoid any auto-increment mysql errors - if ($civiTableSpecs[$col]['DATA_TYPE'] != CRM_Utils_Array::value('DATA_TYPE', $logTableSpecs[$col]) - // We won't alter the log if the length is decreased in case some of the existing data won't fit. - || CRM_Utils_Array::value('LENGTH', $civiTableSpecs[$col]) > CRM_Utils_Array::value('LENGTH', $logTableSpecs[$col]) - ) { - // if data-type is different, surely consider the column - $diff['MODIFY'][] = $col; - } - elseif ($civiTableSpecs[$col]['IS_NULLABLE'] != CRM_Utils_Array::value('IS_NULLABLE', $logTableSpecs[$col]) && - $logTableSpecs[$col]['IS_NULLABLE'] == 'NO' - ) { - // if is-null property is different, and log table's column is NOT-NULL, surely consider the column - $diff['MODIFY'][] = $col; - } - elseif ($civiTableSpecs[$col]['COLUMN_DEFAULT'] != CRM_Utils_Array::value('COLUMN_DEFAULT', $logTableSpecs[$col]) && - !strstr($civiTableSpecs[$col]['COLUMN_DEFAULT'], 'TIMESTAMP') - ) { - // if default property is different, and its not about a timestamp column, consider it - $diff['MODIFY'][] = $col; + if (!empty($specDiff) && $col != 'id' && !in_array($col, $diff['ADD'])) { + if (empty($colSpecs['EXTRA']) || (!empty($colSpecs['EXTRA']) && $colSpecs['EXTRA'] !== 'auto_increment')) { + // ignore 'id' column for any spec changes, to avoid any auto-increment mysql errors + if ($civiTableSpecs[$col]['DATA_TYPE'] != CRM_Utils_Array::value('DATA_TYPE', $logTableSpecs[$col]) + // We won't alter the log if the length is decreased in case some of the existing data won't fit. + || CRM_Utils_Array::value('LENGTH', $civiTableSpecs[$col]) > CRM_Utils_Array::value('LENGTH', $logTableSpecs[$col]) + ) { + // if data-type is different, surely consider the column + $diff['MODIFY'][] = $col; + } + elseif ($civiTableSpecs[$col]['IS_NULLABLE'] != CRM_Utils_Array::value('IS_NULLABLE', $logTableSpecs[$col]) && + $logTableSpecs[$col]['IS_NULLABLE'] == 'NO' + ) { + // if is-null property is different, and log table's column is NOT-NULL, surely consider the column + $diff['MODIFY'][] = $col; + } + elseif ($civiTableSpecs[$col]['COLUMN_DEFAULT'] != CRM_Utils_Array::value('COLUMN_DEFAULT', $logTableSpecs[$col]) && + !strstr($civiTableSpecs[$col]['COLUMN_DEFAULT'], 'TIMESTAMP') + ) { + // if default property is different, and its not about a timestamp column, consider it + $diff['MODIFY'][] = $col; + } } } } diff --git a/tests/phpunit/CRM/Logging/SchemaTest.php b/tests/phpunit/CRM/Logging/SchemaTest.php index 27f72364b9..9c8b7a1a7a 100644 --- a/tests/phpunit/CRM/Logging/SchemaTest.php +++ b/tests/phpunit/CRM/Logging/SchemaTest.php @@ -12,6 +12,10 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase { public function tearDown() { parent::tearDown(); + $schema = new CRM_Logging_Schema(); + $schema->disableLogging(); + $schema->dropAllLogTables(); + CRM_Core_DAO::executeQuery("DROP TABLE IF EXISTS civicrm_test_table"); } public function queryExamples() { @@ -37,8 +41,43 @@ class CRM_Logging_SchemaTest extends CiviUnitTestCase { while ($log_table->fetch()) { $this->assertRegexp('/ENGINE=ARCHIVE/', $log_table->Create_Table); } - $schema->disableLogging(); - $schema->dropAllLogTables(); + } + + public function testAutoIncrementNonIdColumn() { + CRM_Core_DAO::executeQuery("CREATE TABLE `civicrm_test_table` ( + test_id int(10) unsigned NOT NULL AUTO_INCREMENT, + PRIMARY KEY (`test_id`) + ) ENGINE=InnoDB DEFAULT CHARSET=utf8 COLLATE=utf8_unicode_ci"); + $schema = new CRM_Logging_Schema(); + $schema->enableLogging(); + $diffs = $schema->columnsWithDiffSpecs("civicrm_test_table", "log_civicrm_test_table"); + // Test that just havving a non id nanmed column with Auto Increment doesn't create diffs + $this->assertTrue(empty($diffs['MODIFY'])); + $this->assertTrue(empty($diffs['ADD'])); + $this->assertTrue(empty($diffs['OBSOLETE'])); + CRM_Core_DAO::executeQuery("ALTER TABLE civicrm_test_table ADD COLUMN test_varchar varchar(255) DEFAULT NULL"); + \Civi::$statics['CRM_Logging_Schema']['columnSpecs'] = array(); + // Check that it still picks up new columns added. + $diffs = $schema->columnsWithDiffSpecs("civicrm_test_table", "log_civicrm_test_table"); + $this->assertTrue(!empty($diffs['ADD'])); + $this->assertTrue(empty($diffs['MODIFY'])); + $this->assertTrue(empty($diffs['OBSOLETE'])); + $schema->fixSchemaDifferences(); + CRM_Core_DAO::executeQuery("ALTER TABLE civicrm_test_table CHANGE COLUMN test_varchar test_varchar varchar(400) DEFAULT NULL"); + // Check that it properly picks up modifications to columns. + \Civi::$statics['CRM_Logging_Schema']['columnSpecs'] = array(); + $diffs = $schema->columnsWithDiffSpecs("civicrm_test_table", "log_civicrm_test_table"); + $this->assertTrue(!empty($diffs['MODIFY'])); + $this->assertTrue(empty($diffs['ADD'])); + $this->assertTrue(empty($diffs['OBSOLETE'])); + $schema->fixSchemaDifferences(); + CRM_Core_DAO::executeQuery("ALTER TABLE civicrm_test_table CHANGE COLUMN test_varchar test_varchar varchar(300) DEFAULT NULL"); + // Check that when we reduce the size of column that the log table doesn't shrink as well. + \Civi::$statics['CRM_Logging_Schema']['columnSpecs'] = array(); + $diffs = $schema->columnsWithDiffSpecs("civicrm_test_table", "log_civicrm_test_table"); + $this->assertTrue(empty($diffs['MODIFY'])); + $this->assertTrue(empty($diffs['ADD'])); + $this->assertTrue(empty($diffs['OBSOLETE'])); } } -- 2.25.1