Skip to content

Commit

Permalink
Merge pull request #18939 from colemanw/serializeFix
Browse files Browse the repository at this point in the history 
Fix updating custom field schema when toggling search or multiple
  • Loading branch information
@colemanw
colemanw authored Dec 3, 2020
2 parents a11f15b + ba9c74a commit 6874397
Show file tree
Hide file tree
Showing 4 changed files with 136 additions and 52 deletions.
72 changes: 49 additions & 23 deletions CRM/Core/BAO/CustomField.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);

Expand All @@ -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']);
Expand All @@ -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);
Expand All @@ -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);
Expand Down Expand Up @@ -1686,31 +1712,29 @@ 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);
}

/**
* 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);
}

/**
Expand Down Expand Up @@ -2006,9 +2030,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();
Expand All @@ -2029,13 +2050,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.
*
Expand Down Expand Up @@ -2646,13 +2676,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';
Expand Down
68 changes: 45 additions & 23 deletions CRM/Core/BAO/SchemaHandler.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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) {
Expand Down Expand Up @@ -155,34 +155,33 @@ 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
if ($params['type'] == 'text') {
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;
}
Expand Down Expand Up @@ -260,13 +259,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;
}
Expand Down Expand Up @@ -704,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);
}

/**
Expand All @@ -721,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':
Expand All @@ -739,9 +733,22 @@ public static function getFieldAlterSQL($params, $indexExist) {

case 'modify':
$separator = "\n";
$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 (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);
$sql .= self::buildSearchIndexSQL($params, $separator, "ADD INDEX ", $existingIndex);
if (!$existingIndex && $fkSql) {
$sql .= $fkSql;
}
break;

case 'delete':
Expand All @@ -757,6 +764,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.
*
Expand Down
9 changes: 5 additions & 4 deletions tests/phpunit/CRM/Core/BAO/SchemaHandlerTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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));
}

}
39 changes: 37 additions & 2 deletions tests/phpunit/api/v4/Action/CustomFieldAlterTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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();

Expand All @@ -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)
Expand All @@ -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)
Expand Down Expand Up @@ -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']);
}

}

0 comments on commit 6874397

Please sign in to comment.