From 051f2455190cd43bb530f2bad524552c4041bdbf Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Thu, 5 Nov 2020 09:26:03 -0500 Subject: [PATCH 1/2] Fix crash when changing Country or StateProvince custom fields to single/multi select A single-valued country or state select field is stored as INTEGER with a FK constraint. A multi-valued field is stored as VARCHAR with no constraint. Switching back and forth between the two would cause a crash for 2 reasons: 1. The FK constraint was not being added/dropped. 2. Serialization was hapening prematurely and value separator characters were being inserted into the values while the field was still type INTEGER. --- CRM/Core/BAO/CustomField.php | 69 +++++++++++++------ CRM/Core/BAO/SchemaHandler.php | 31 +++++++-- .../api/v4/Action/CustomFieldAlterTest.php | 39 ++++++++++- 3 files changed, 111 insertions(+), 28 deletions(-) diff --git a/CRM/Core/BAO/CustomField.php b/CRM/Core/BAO/CustomField.php index 4a0ae0fba333..8d1efded39fb 100644 --- a/CRM/Core/BAO/CustomField.php +++ b/CRM/Core/BAO/CustomField.php @@ -85,9 +85,18 @@ public static function dataToType() { * @return CRM_Core_DAO_CustomField */ public static function create($params) { + $changeSerialize = self::getChangeSerialize($params); $customField = self::createCustomFieldRecord($params); + // When deserializing a field, the update needs to run before the schema change + if ($changeSerialize === 0) { + CRM_Core_DAO::singleValueQuery(self::getAlterSerializeSQL($customField)); + } $op = empty($params['id']) ? 'add' : 'modify'; self::createField($customField, $op); + // When serializing a field, the update needs to run after the schema change + if ($changeSerialize === 1) { + CRM_Core_DAO::singleValueQuery(self::getAlterSerializeSQL($customField)); + } CRM_Utils_Hook::post(($op === 'add' ? 'create' : 'edit'), 'CustomField', $customField->id, $customField); @@ -114,10 +123,18 @@ public static function create($params) { * @throws \CiviCRM_API3_Exception */ public static function bulkSave($bulkParams, $defaults = []) { - $addedColumns = $sql = $customFields = []; + $addedColumns = $sql = $customFields = $pre = $post = []; foreach ($bulkParams as $index => $fieldParams) { $params = array_merge($defaults, $fieldParams); + $changeSerialize = self::getChangeSerialize($params); $customField = self::createCustomFieldRecord($params); + // Serialize/deserialize sql must run after/before the table is altered + if ($changeSerialize === 0) { + $pre[] = self::getAlterSerializeSQL($customField); + } + if ($changeSerialize === 1) { + $post[] = self::getAlterSerializeSQL($customField); + } $fieldSQL = self::getAlterFieldSQL($customField, empty($params['id']) ? 'add' : 'modify'); if (!isset($params['custom_group_id'])) { $params['custom_group_id'] = civicrm_api3('CustomField', 'getvalue', ['id' => $customField->id, 'return' => 'custom_group_id']); @@ -128,6 +145,10 @@ public static function bulkSave($bulkParams, $defaults = []) { $customFields[$index] = $customField; } + foreach ($pre as $query) { + CRM_Core_DAO::executeQuery($query); + } + foreach ($sql as $tableName => $statements) { // CRM-7007: do not i18n-rewrite this query CRM_Core_DAO::executeQuery("ALTER TABLE $tableName " . implode(', ', $statements), [], TRUE, NULL, FALSE, FALSE); @@ -139,6 +160,11 @@ public static function bulkSave($bulkParams, $defaults = []) { Civi::service('sql_triggers')->rebuild($tableName, TRUE); } + + foreach ($post as $query) { + CRM_Core_DAO::executeQuery($query); + } + CRM_Utils_System::flushCache(); foreach ($customFields as $index => $customField) { CRM_Utils_Hook::post(empty($bulkParams[$index]['id']) ? 'create' : 'edit', 'CustomField', $customField->id, $customField); @@ -1695,22 +1721,21 @@ public static function getAlterFieldSQL($field, $operation) { } /** - * Reformat existing values for a field when changing its serialize attribute + * Get query to reformat existing values for a field when changing its serialize attribute * * @param CRM_Core_DAO_CustomField $field - * @throws CRM_Core_Exception + * @return string */ - private static function alterSerialize($field) { + private static function getAlterSerializeSQL($field) { $table = CRM_Core_DAO::getFieldValue('CRM_Core_DAO_CustomGroup', $field->custom_group_id, 'table_name'); $col = $field->column_name; $sp = CRM_Core_DAO::VALUE_SEPARATOR; if ($field->serialize) { - $sql = "UPDATE `$table` SET `$col` = CONCAT('$sp', `$col`, '$sp') WHERE `$col` IS NOT NULL AND `$col` NOT LIKE '$sp%' AND `$col` != ''"; + return "UPDATE `$table` SET `$col` = CONCAT('$sp', `$col`, '$sp') WHERE `$col` IS NOT NULL AND `$col` NOT LIKE '$sp%' AND `$col` != ''"; } else { - $sql = "UPDATE `$table` SET `$col` = SUBSTRING_INDEX(SUBSTRING(`$col`, 2), '$sp', 1) WHERE `$col` LIKE '$sp%'"; + return "UPDATE `$table` SET `$col` = SUBSTRING_INDEX(SUBSTRING(`$col`, 2), '$sp', 1) WHERE `$col` LIKE '$sp%'"; } - CRM_Core_DAO::executeQuery($sql); } /** @@ -2006,9 +2031,6 @@ protected static function createCustomFieldRecord($params) { $transaction = new CRM_Core_Transaction(); $params = self::prepareCreate($params); - $alterSerialize = isset($params['serialize']) && !empty($params['id']) - && ($params['serialize'] != CRM_Core_DAO::getFieldValue('CRM_Core_DAO_CustomField', $params['id'], 'serialize')); - $customField = new CRM_Core_DAO_CustomField(); $customField->copyValues($params); $customField->save(); @@ -2029,13 +2051,22 @@ protected static function createCustomFieldRecord($params) { // make sure all values are present in the object for further processing $customField->find(TRUE); - if ($alterSerialize) { - self::alterSerialize($customField); - } - return $customField; } + /** + * @param $params + * @return int|null + */ + protected static function getChangeSerialize($params) { + if (isset($params['serialize']) && !empty($params['id'])) { + if ($params['serialize'] != CRM_Core_DAO::getFieldValue('CRM_Core_DAO_CustomField', $params['id'], 'serialize')) { + return (int) $params['serialize']; + } + } + return NULL; + } + /** * Move custom data from one contact to another. * @@ -2646,13 +2677,9 @@ protected static function prepareCreateParams($field, $operation) { 'searchable' => $field->is_searchable, ]; - if ($operation == 'delete') { - $fkName = "{$tableName}_{$field->column_name}"; - if (strlen($fkName) >= 48) { - $fkName = substr($fkName, 0, 32) . '_' . substr(md5($fkName), 0, 16); - } - $params['fkName'] = $fkName; - } + // For adding/dropping FK constraints + $params['fkName'] = CRM_Core_BAO_SchemaHandler::getIndexName($tableName, $field->column_name); + if ($field->data_type == 'Country' && !self::isSerialized($field)) { $params['fk_table_name'] = 'civicrm_country'; $params['fk_field_name'] = 'id'; diff --git a/CRM/Core/BAO/SchemaHandler.php b/CRM/Core/BAO/SchemaHandler.php index cb56200c4b74..49c7ddb6e869 100644 --- a/CRM/Core/BAO/SchemaHandler.php +++ b/CRM/Core/BAO/SchemaHandler.php @@ -260,13 +260,10 @@ public static function buildForeignKeySQL($params, $separator, $prefix, $tableNa $sql .= $separator; $sql .= str_repeat(' ', 8); $sql .= $prefix; - $fkName = "{$tableName}_{$params['name']}"; - if (strlen($fkName) >= 48) { - $fkName = substr($fkName, 0, 32) . "_" . substr(md5($fkName), 0, 16); - } + $fkName = $params['fkName'] ?? self::getIndexName($tableName, $params['name']); $sql .= "CONSTRAINT FK_$fkName FOREIGN KEY ( `{$params['name']}` ) REFERENCES {$params['fk_table_name']} ( {$params['fk_field_name']} ) "; - $sql .= CRM_Utils_Array::value('fk_attributes', $params); + $sql .= $params['fk_attributes'] ?? ''; } return $sql; } @@ -739,9 +736,18 @@ public static function getFieldAlterSQL($params, $indexExist) { case 'modify': $separator = "\n"; + $fkExists = CRM_Core_DAO::singleValueQuery("SHOW INDEX FROM `{$params['table_name']}` WHERE Key_name = 'FK_{$params['fkName']}'"); + $fkSql = self::buildForeignKeySQL($params, ",\n", "ADD ", $params['table_name']); + if ($fkExists && !$fkSql) { + $sql .= "$separator DROP FOREIGN KEY FK_{$params['fkName']}"; + $separator = ",\n"; + } $sql .= self::buildFieldSQL($params, $separator, "MODIFY "); $separator = ",\n"; $sql .= self::buildSearchIndexSQL($params, $separator, "ADD INDEX ", $indexExist); + if (!$fkExists && $fkSql) { + $sql .= $fkSql; + } break; case 'delete': @@ -757,6 +763,21 @@ public static function getFieldAlterSQL($params, $indexExist) { return $sql; } + /** + * Turns tableName + columnName into a safe & predictable index name + * + * @param $tableName + * @param $columnName + * @return string + */ + public static function getIndexName($tableName, $columnName) { + $indexName = "{$tableName}_{$columnName}"; + if (strlen($indexName) >= 48) { + $indexName = substr($indexName, 0, 32) . "_" . substr(md5($indexName), 0, 16); + } + return $indexName; + } + /** * Performs the utf8mb4 migration. * diff --git a/tests/phpunit/api/v4/Action/CustomFieldAlterTest.php b/tests/phpunit/api/v4/Action/CustomFieldAlterTest.php index 3e44ca82e2d5..689ef37ba556 100644 --- a/tests/phpunit/api/v4/Action/CustomFieldAlterTest.php +++ b/tests/phpunit/api/v4/Action/CustomFieldAlterTest.php @@ -50,6 +50,13 @@ public function testChangeSerialize() { ->addValue('label', 'TestText') ->addValue('html_type', 'Text'), 0 ) + ->addChain('field3', CustomField::create() + ->addValue('custom_group_id', '$id') + ->addValue('serialize', TRUE) + ->addValue('label', 'TestCountry') + ->addValue('data_type', 'Country') + ->addValue('html_type', 'Select'), 0 + ) ->execute() ->first(); @@ -58,7 +65,7 @@ public function testChangeSerialize() { ->addRecord(['subject' => 'A1', 'MyFieldsToAlter.TestText' => 'A1', 'MyFieldsToAlter.TestOptions' => '1']) ->addRecord(['subject' => 'A2', 'MyFieldsToAlter.TestText' => 'A2', 'MyFieldsToAlter.TestOptions' => '2']) ->addRecord(['subject' => 'A3', 'MyFieldsToAlter.TestText' => 'A3', 'MyFieldsToAlter.TestOptions' => '']) - ->addRecord(['subject' => 'A4', 'MyFieldsToAlter.TestText' => 'A4']) + ->addRecord(['subject' => 'A4', 'MyFieldsToAlter.TestText' => 'A4', 'MyFieldsToAlter.TestCountry' => [1228, 1039]]) ->execute(); $result = Activity::get(FALSE) @@ -74,7 +81,7 @@ public function testChangeSerialize() { $this->assertTrue(empty($result['A3']['MyFieldsToAlter.TestOptions'])); $this->assertTrue(empty($result['A4']['MyFieldsToAlter.TestOptions'])); - // Change field to multiselect + // Change options field to multiselect CustomField::update(FALSE) ->addWhere('id', '=', $customGroup['field1']['id']) ->addValue('serialize', TRUE) @@ -109,6 +116,34 @@ public function testChangeSerialize() { $this->assertEquals('Two', $result['A2']['MyFieldsToAlter.TestOptions:label']); $this->assertTrue(empty($result['A3']['MyFieldsToAlter.TestOptions'])); $this->assertTrue(empty($result['A4']['MyFieldsToAlter.TestOptions'])); + + // Change country field from multiselect to single + CustomField::update(FALSE) + ->addWhere('id', '=', $customGroup['field3']['id']) + ->addValue('serialize', FALSE) + ->execute(); + + $result = Activity::get(FALSE) + ->addWhere('MyFieldsToAlter.TestCountry', 'IS NOT NULL') + ->addSelect('custom.*', 'subject') + ->execute()->indexBy('subject'); + $this->assertCount(1, $result); + // The two values originally entered will now be one value + $this->assertEquals(1228, $result['A4']['MyFieldsToAlter.TestCountry']); + + // Change country field from single to multiselect + CustomField::update(FALSE) + ->addWhere('id', '=', $customGroup['field3']['id']) + ->addValue('serialize', TRUE) + ->execute(); + + $result = Activity::get(FALSE) + ->addWhere('MyFieldsToAlter.TestCountry', 'IS NOT NULL') + ->addSelect('custom.*', 'subject') + ->execute()->indexBy('subject'); + $this->assertCount(1, $result); + // The two values originally entered will now be one value + $this->assertEquals([1228], $result['A4']['MyFieldsToAlter.TestCountry']); } } From ba9c74ab3eef50a18495d70e46a2af46cb8b40c6 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Thu, 5 Nov 2020 17:27:18 -0500 Subject: [PATCH 2/2] Custom field - fix creating/updating indexes when modifying fields --- CRM/Core/BAO/CustomField.php | 3 +- CRM/Core/BAO/SchemaHandler.php | 45 ++++++++++--------- .../CRM/Core/BAO/SchemaHandlerTest.php | 9 ++-- 3 files changed, 29 insertions(+), 28 deletions(-) diff --git a/CRM/Core/BAO/CustomField.php b/CRM/Core/BAO/CustomField.php index 8d1efded39fb..f951f3fce329 100644 --- a/CRM/Core/BAO/CustomField.php +++ b/CRM/Core/BAO/CustomField.php @@ -1712,12 +1712,11 @@ public static function createField($field, $operation) { * @return bool */ public static function getAlterFieldSQL($field, $operation) { - $indexExist = $operation === 'add' ? FALSE : CRM_Core_DAO::getFieldValue('CRM_Core_DAO_CustomField', $field->id, 'is_searchable'); $params = self::prepareCreateParams($field, $operation); // Let's suppress the required flag, since that can cause an sql issue... for unknown reasons since we are calling // a function only used by Custom Field creation... $params['required'] = FALSE; - return CRM_Core_BAO_SchemaHandler::getFieldAlterSQL($params, $indexExist); + return CRM_Core_BAO_SchemaHandler::getFieldAlterSQL($params); } /** diff --git a/CRM/Core/BAO/SchemaHandler.php b/CRM/Core/BAO/SchemaHandler.php index 49c7ddb6e869..5f54af1478f1 100644 --- a/CRM/Core/BAO/SchemaHandler.php +++ b/CRM/Core/BAO/SchemaHandler.php @@ -85,7 +85,7 @@ public static function buildTableSQL($params) { $sql .= self::buildPrimaryKeySQL($field, $separator, $prefix); } foreach ($params['fields'] as $field) { - $sql .= self::buildSearchIndexSQL($field, $separator, $prefix); + $sql .= self::buildSearchIndexSQL($field, $separator); } if (isset($params['indexes'])) { foreach ($params['indexes'] as $index) { @@ -155,13 +155,13 @@ public static function buildPrimaryKeySQL($params, $separator, $prefix) { /** * @param array $params - * @param $separator - * @param $prefix - * @param bool $indexExist + * @param string $separator + * @param string $prefix + * @param string|NULL $existingIndex * * @return NULL|string */ - public static function buildSearchIndexSQL($params, $separator, $prefix, $indexExist = FALSE) { + public static function buildSearchIndexSQL($params, $separator, $prefix = '', $existingIndex = NULL) { $sql = NULL; // dont index blob @@ -169,20 +169,19 @@ public static function buildSearchIndexSQL($params, $separator, $prefix, $indexE return $sql; } - //create index only for searchable fields during ADD, - //create index only if field is become searchable during MODIFY, - //drop index only if field is no longer searchable and it does not reference - //a forgein key (and indexExist is true) - if (!empty($params['searchable']) && !$indexExist) { + // Add index if field is searchable if it does not reference a foreign key + // (skip indexing FK fields because it would be redundant to have 2 indexes) + if (!empty($params['searchable']) && empty($params['fk_table_name']) && substr($existingIndex, 0, 5) !== 'INDEX') { $sql .= $separator; $sql .= str_repeat(' ', 8); $sql .= $prefix; $sql .= "INDEX_{$params['name']} ( {$params['name']} )"; } - elseif (empty($params['searchable']) && empty($params['fk_table_name']) && $indexExist) { + // Drop search index if field is no longer searchable + elseif (empty($params['searchable']) && substr($existingIndex, 0, 5) === 'INDEX') { $sql .= $separator; $sql .= str_repeat(' ', 8); - $sql .= "DROP INDEX INDEX_{$params['name']}"; + $sql .= "DROP INDEX $existingIndex"; } return $sql; } @@ -701,14 +700,13 @@ public static function createMissingIndices($missingIndices) { * Build the sql to alter the field. * * @param array $params - * @param bool $indexExist * * @return string */ - public static function buildFieldChangeSql($params, $indexExist) { + public static function buildFieldChangeSql($params) { $sql = str_repeat(' ', 8); $sql .= "ALTER TABLE {$params['table_name']}"; - return $sql . self::getFieldAlterSQL($params, $indexExist); + return $sql . self::getFieldAlterSQL($params); } /** @@ -718,11 +716,10 @@ public static function buildFieldChangeSql($params, $indexExist) { * by individual field we can do one or many. * * @param array $params - * @param bool $indexExist * * @return string */ - public static function getFieldAlterSQL($params, $indexExist) { + public static function getFieldAlterSQL($params) { $sql = ''; switch ($params['operation']) { case 'add': @@ -736,16 +733,20 @@ public static function getFieldAlterSQL($params, $indexExist) { case 'modify': $separator = "\n"; - $fkExists = CRM_Core_DAO::singleValueQuery("SHOW INDEX FROM `{$params['table_name']}` WHERE Key_name = 'FK_{$params['fkName']}'"); + $existingIndex = NULL; + $dao = CRM_Core_DAO::executeQuery("SHOW INDEX FROM `{$params['table_name']}` WHERE Column_name = '{$params['name']}'"); + if ($dao->fetch()) { + $existingIndex = $dao->Key_name; + } $fkSql = self::buildForeignKeySQL($params, ",\n", "ADD ", $params['table_name']); - if ($fkExists && !$fkSql) { - $sql .= "$separator DROP FOREIGN KEY FK_{$params['fkName']}"; + if (substr($existingIndex, 0, 2) === 'FK' && !$fkSql) { + $sql .= "$separator DROP FOREIGN KEY {$existingIndex},\nDROP INDEX {$existingIndex}"; $separator = ",\n"; } $sql .= self::buildFieldSQL($params, $separator, "MODIFY "); $separator = ",\n"; - $sql .= self::buildSearchIndexSQL($params, $separator, "ADD INDEX ", $indexExist); - if (!$fkExists && $fkSql) { + $sql .= self::buildSearchIndexSQL($params, $separator, "ADD INDEX ", $existingIndex); + if (!$existingIndex && $fkSql) { $sql .= $fkSql; } break; diff --git a/tests/phpunit/CRM/Core/BAO/SchemaHandlerTest.php b/tests/phpunit/CRM/Core/BAO/SchemaHandlerTest.php index ae788ba64f72..756ba9bc5bb2 100644 --- a/tests/phpunit/CRM/Core/BAO/SchemaHandlerTest.php +++ b/tests/phpunit/CRM/Core/BAO/SchemaHandlerTest.php @@ -377,24 +377,25 @@ public function testDropColumn() { */ public function testBuildFieldChangeSql() { $params = [ - 'table_name' => 'big_table', + 'table_name' => 'civicrm_contact', 'operation' => 'add', 'name' => 'big_bob', 'type' => 'text', ]; $sql = CRM_Core_BAO_SchemaHandler::buildFieldChangeSql($params, FALSE); - $this->assertEquals('ALTER TABLE big_table + $this->assertEquals('ALTER TABLE civicrm_contact ADD COLUMN `big_bob` text', trim($sql)); $params['operation'] = 'modify'; $params['comment'] = 'super big'; + $params['fkName'] = CRM_Core_BAO_SchemaHandler::getIndexName('civicrm_contact', 'big_bob'); $sql = CRM_Core_BAO_SchemaHandler::buildFieldChangeSql($params, FALSE); - $this->assertEquals("ALTER TABLE big_table + $this->assertEquals("ALTER TABLE civicrm_contact MODIFY `big_bob` text COMMENT 'super big'", trim($sql)); $params['operation'] = 'delete'; $sql = CRM_Core_BAO_SchemaHandler::buildFieldChangeSql($params, FALSE); - $this->assertEquals('ALTER TABLE big_table DROP COLUMN `big_bob`', trim($sql)); + $this->assertEquals('ALTER TABLE civicrm_contact DROP COLUMN `big_bob`', trim($sql)); } }