From 07a460ae8678ec8c422e278bd2158fb4543f1ed1 Mon Sep 17 00:00:00 2001 From: ndm2 Date: Fri, 22 Dec 2023 17:01:14 +0100 Subject: [PATCH] Fix unique constraints getting lost. When using the first column of the constraints and foreign keys to index the rules array, constraints and foreign keys that share the same first column in the composite constraint will overwrite previously set rules. refs #957 --- src/Command/ModelCommand.php | 49 ++++-- templates/bake/Model/table.twig | 6 +- tests/TestCase/Command/ModelCommandTest.php | 140 ++++++++++++++++-- ...iationDetectionCategoriesProductsTable.php | 4 +- ...sociationDetectionProductVersionsTable.php | 2 +- ...ionDetectionProductVersionsTableSigned.php | 2 +- .../comparisons/Model/testBakeTableConfig.php | 2 +- .../Model/testBakeTableWithCounterCache.php | 2 +- .../Model/testBakeUpdateTableNoFile.php | 2 +- 9 files changed, 174 insertions(+), 35 deletions(-) diff --git a/src/Command/ModelCommand.php b/src/Command/ModelCommand.php index 2b006f50..05ebce63 100644 --- a/src/Command/ModelCommand.php +++ b/src/Command/ModelCommand.php @@ -984,22 +984,14 @@ public function getRules(Table $model, array $associations, Arguments $args): ar return []; } $schema = $model->getSchema(); - $fields = $schema->columns(); - if (empty($fields)) { + $schemaFields = $schema->columns(); + if (empty($schemaFields)) { return []; } - $uniqueColumns = ['username', 'login']; - if (in_array($model->getAlias(), ['Users', 'Accounts'])) { - $uniqueColumns[] = 'email'; - } + $uniqueRules = []; + $uniqueConstraintsColumns = []; - $rules = []; - foreach ($fields as $fieldName) { - if (in_array($fieldName, $uniqueColumns, true)) { - $rules[$fieldName] = ['name' => 'isUnique', 'fields' => [$fieldName], 'options' => []]; - } - } foreach ($schema->constraints() as $name) { $constraint = $schema->getConstraint($name); if ($constraint['type'] !== TableSchema::CONSTRAINT_UNIQUE) { @@ -1007,8 +999,11 @@ public function getRules(Table $model, array $associations, Arguments $args): ar } $options = []; - $fields = $constraint['columns']; - foreach ($fields as $field) { + /** @var array $constraintFields */ + $constraintFields = $constraint['columns']; + $uniqueConstraintsColumns = [...$uniqueConstraintsColumns, ...$constraintFields]; + + foreach ($constraintFields as $field) { if ($schema->isNullable($field)) { $allowMultiple = !ConnectionManager::get($this->connection)->getDriver() instanceof Sqlserver; $options['allowMultipleNulls'] = $allowMultiple; @@ -1016,15 +1011,37 @@ public function getRules(Table $model, array $associations, Arguments $args): ar } } - $rules[$constraint['columns'][0]] = ['name' => 'isUnique', 'fields' => $fields, 'options' => $options]; + $uniqueRules[] = ['name' => 'isUnique', 'fields' => $constraintFields, 'options' => $options]; } + $possiblyUniqueColumns = ['username', 'login']; + if (in_array($model->getAlias(), ['Users', 'Accounts'])) { + $possiblyUniqueColumns[] = 'email'; + } + + $possiblyUniqueRules = []; + foreach ($schemaFields as $field) { + if ( + !in_array($field, $uniqueConstraintsColumns, true) && + in_array($field, $possiblyUniqueColumns, true) + ) { + $possiblyUniqueRules[] = ['name' => 'isUnique', 'fields' => [$field], 'options' => []]; + } + } + + $rules = [...$possiblyUniqueRules, ...$uniqueRules]; + if (empty($associations['belongsTo'])) { return $rules; } foreach ($associations['belongsTo'] as $assoc) { - $rules[$assoc['foreignKey']] = ['name' => 'existsIn', 'extra' => $assoc['alias'], 'options' => []]; + $rules[] = [ + 'name' => 'existsIn', + 'fields' => (array)$assoc['foreignKey'], + 'extra' => $assoc['alias'], + 'options' => [], + ]; } return $rules; diff --git a/templates/bake/Model/table.twig b/templates/bake/Model/table.twig index 5a1b9620..41a1eb2e 100644 --- a/templates/bake/Model/table.twig +++ b/templates/bake/Model/table.twig @@ -128,13 +128,13 @@ class {{ name }}Table extends Table{{ fileBuilder.classBuilder.implements ? ' im */ public function buildRules(RulesChecker $rules): RulesChecker { -{% for field, rule in rulesChecker %} -{% set fields = rule.fields is defined ? Bake.exportArray(rule.fields) : Bake.exportVar(field) %} +{% for rule in rulesChecker %} +{% set fields = Bake.exportArray(rule.fields) %} {% set options = '' %} {% for optionName, optionValue in rule.options %} {%~ set options = (loop.first ? '[' : options) ~ "'#{optionName}' => " ~ Bake.exportVar(optionValue) ~ (loop.last ? ']' : ', ') %} {% endfor %} - $rules->add($rules->{{ rule.name }}({{ fields|raw }}{{ (rule.extra|default ? ", '#{rule.extra}'" : '')|raw }}{{ (options ? ', ' ~ options : '')|raw }}), ['errorField' => '{{ field }}']); + $rules->add($rules->{{ rule.name }}({{ fields|raw }}{{ (rule.extra|default ? ", '#{rule.extra}'" : '')|raw }}{{ (options ? ', ' ~ options : '')|raw }}), ['errorField' => '{{ rule.fields[0] }}']); {% endfor %} return $rules; diff --git a/tests/TestCase/Command/ModelCommandTest.php b/tests/TestCase/Command/ModelCommandTest.php index 3e5f8bd9..4a3a8d3b 100644 --- a/tests/TestCase/Command/ModelCommandTest.php +++ b/tests/TestCase/Command/ModelCommandTest.php @@ -1368,7 +1368,7 @@ public function testGetRules() ], [ 'alias' => 'Sites', - 'foreignKey' => 'site_id', + 'foreignKey' => ['site_id1', 'site_id2'], ], ], 'hasMany' => [ @@ -1382,18 +1382,20 @@ public function testGetRules() $args = new Arguments([], [], []); $result = $command->getRules($model, $associations, $args); $expected = [ - 'username' => [ + [ 'name' => 'isUnique', 'fields' => ['username'], 'options' => [], ], - 'country_id' => [ + [ 'name' => 'existsIn', + 'fields' => ['country_id'], 'extra' => 'Countries', 'options' => [], ], - 'site_id' => [ + [ 'name' => 'existsIn', + 'fields' => ['site_id1', 'site_id2'], 'extra' => 'Sites', 'options' => [], ], @@ -1404,9 +1406,6 @@ public function testGetRules() /** * Tests the getRules with unique keys. * - * Multi-column constraints are ignored as they would - * require a break in compatibility. - * * @return void */ public function testGetRulesUniqueKeys() @@ -1416,7 +1415,7 @@ public function testGetRulesUniqueKeys() 'type' => 'unique', 'columns' => ['title'], ]); - $model->getSchema()->addConstraint('ignored_constraint', [ + $model->getSchema()->addConstraint('unique_composite', [ 'type' => 'unique', 'columns' => ['title', 'user_id'], ]); @@ -1425,7 +1424,12 @@ public function testGetRulesUniqueKeys() $args = new Arguments([], [], []); $result = $command->getRules($model, [], $args); $expected = [ - 'title' => [ + [ + 'name' => 'isUnique', + 'fields' => ['title'], + 'options' => [], + ], + [ 'name' => 'isUnique', 'fields' => ['title', 'user_id'], 'options' => [], @@ -1434,6 +1438,124 @@ public function testGetRulesUniqueKeys() $this->assertEquals($expected, $result); } + /** + * Tests that there are no conflicts between neither multiple constraints, + * nor with foreign keys that share one or more identical column. + */ + public function testGetRulesNoColumnNameConflictForUniqueConstraints(): void + { + $model = $this->getTableLocator()->get('Users'); + $model->setSchema([ + 'department_id' => ['type' => 'integer', 'null' => false], + 'username' => ['type' => 'string', 'null' => false], + 'email' => ['type' => 'string', 'null' => false], + ]); + + $model->getSchema()->addConstraint('unique_composite_1', [ + 'type' => 'unique', + 'columns' => ['department_id', 'username'], + ]); + $model->getSchema()->addConstraint('unique_composite_2', [ + 'type' => 'unique', + 'columns' => ['department_id', 'email'], + ]); + + $command = new ModelCommand(); + $args = new Arguments([], [], []); + $associations = [ + 'belongsTo' => [ + ['alias' => 'Departments', 'foreignKey' => 'department_id'], + ], + ]; + + $result = $command->getRules($model, $associations, $args); + $expected = [ + [ + 'name' => 'isUnique', + 'fields' => ['department_id', 'username'], + 'options' => [], + ], + [ + 'name' => 'isUnique', + 'fields' => ['department_id', 'email'], + 'options' => [], + ], + [ + 'name' => 'existsIn', + 'fields' => ['department_id'], + 'extra' => 'Departments', + 'options' => [], + ], + ]; + $this->assertEquals($expected, $result); + } + + /** + * Tests generating unique rules for possibly unique columns based on + * column names instead of on actual unique constraints. + */ + public function testGetRulesForPossiblyUniqueColumns(): void + { + $model = $this->getTableLocator()->get('Users'); + $model->setSchema([ + 'department_id' => ['type' => 'integer', 'null' => false], + 'username' => ['type' => 'string', 'null' => false], + 'login' => ['type' => 'string', 'null' => false], + 'email' => ['type' => 'string', 'null' => false], + ]); + + $command = new ModelCommand(); + $args = new Arguments([], [], []); + $result = $command->getRules($model, [], $args); + $expected = [ + [ + 'name' => 'isUnique', + 'fields' => ['username'], + 'options' => [], + ], + [ + 'name' => 'isUnique', + 'fields' => ['login'], + 'options' => [], + ], + [ + 'name' => 'isUnique', + 'fields' => ['email'], + 'options' => [], + ], + ]; + $this->assertEquals($expected, $result); + + // possibly unique columns should not cause additional rules + // to be generated in case the column is already present in + // an actual unique constraint + + $model->getSchema()->addConstraint('unique_composite', [ + 'type' => 'unique', + 'columns' => ['department_id', 'username'], + ]); + + $result = $command->getRules($model, [], $args); + $expected = [ + [ + 'name' => 'isUnique', + 'fields' => ['login'], + 'options' => [], + ], + [ + 'name' => 'isUnique', + 'fields' => ['email'], + 'options' => [], + ], + [ + 'name' => 'isUnique', + 'fields' => ['department_id', 'username'], + 'options' => [], + ], + ]; + $this->assertEquals($expected, $result); + } + /** * Test that specific behaviors are auto-detected * diff --git a/tests/comparisons/Model/testBakeAssociationDetectionCategoriesProductsTable.php b/tests/comparisons/Model/testBakeAssociationDetectionCategoriesProductsTable.php index d1f7d554..c5fff2bf 100644 --- a/tests/comparisons/Model/testBakeAssociationDetectionCategoriesProductsTable.php +++ b/tests/comparisons/Model/testBakeAssociationDetectionCategoriesProductsTable.php @@ -63,8 +63,8 @@ public function initialize(array $config): void */ public function buildRules(RulesChecker $rules): RulesChecker { - $rules->add($rules->existsIn('category_id', 'Categories'), ['errorField' => 'category_id']); - $rules->add($rules->existsIn('product_id', 'Products'), ['errorField' => 'product_id']); + $rules->add($rules->existsIn(['category_id'], 'Categories'), ['errorField' => 'category_id']); + $rules->add($rules->existsIn(['product_id'], 'Products'), ['errorField' => 'product_id']); return $rules; } diff --git a/tests/comparisons/Model/testBakeAssociationDetectionProductVersionsTable.php b/tests/comparisons/Model/testBakeAssociationDetectionProductVersionsTable.php index c4c27056..c6723816 100644 --- a/tests/comparisons/Model/testBakeAssociationDetectionProductVersionsTable.php +++ b/tests/comparisons/Model/testBakeAssociationDetectionProductVersionsTable.php @@ -78,7 +78,7 @@ public function validationDefault(Validator $validator): Validator */ public function buildRules(RulesChecker $rules): RulesChecker { - $rules->add($rules->existsIn('product_id', 'Products'), ['errorField' => 'product_id']); + $rules->add($rules->existsIn(['product_id'], 'Products'), ['errorField' => 'product_id']); return $rules; } diff --git a/tests/comparisons/Model/testBakeAssociationDetectionProductVersionsTableSigned.php b/tests/comparisons/Model/testBakeAssociationDetectionProductVersionsTableSigned.php index 3e07ce94..9e3a4a62 100644 --- a/tests/comparisons/Model/testBakeAssociationDetectionProductVersionsTableSigned.php +++ b/tests/comparisons/Model/testBakeAssociationDetectionProductVersionsTableSigned.php @@ -78,7 +78,7 @@ public function validationDefault(Validator $validator): Validator */ public function buildRules(RulesChecker $rules): RulesChecker { - $rules->add($rules->existsIn('product_id', 'Products'), ['errorField' => 'product_id']); + $rules->add($rules->existsIn(['product_id'], 'Products'), ['errorField' => 'product_id']); return $rules; } diff --git a/tests/comparisons/Model/testBakeTableConfig.php b/tests/comparisons/Model/testBakeTableConfig.php index a27066c3..16309d95 100644 --- a/tests/comparisons/Model/testBakeTableConfig.php +++ b/tests/comparisons/Model/testBakeTableConfig.php @@ -113,7 +113,7 @@ public function validationDefault(Validator $validator): Validator */ public function buildRules(RulesChecker $rules): RulesChecker { - $rules->add($rules->existsIn('user_id', 'Users'), ['errorField' => 'user_id']); + $rules->add($rules->existsIn(['user_id'], 'Users'), ['errorField' => 'user_id']); return $rules; } diff --git a/tests/comparisons/Model/testBakeTableWithCounterCache.php b/tests/comparisons/Model/testBakeTableWithCounterCache.php index 5ed0a531..7e5369e0 100644 --- a/tests/comparisons/Model/testBakeTableWithCounterCache.php +++ b/tests/comparisons/Model/testBakeTableWithCounterCache.php @@ -66,7 +66,7 @@ public function initialize(array $config): void */ public function buildRules(RulesChecker $rules): RulesChecker { - $rules->add($rules->existsIn('todo_item_id', 'TodoItems'), ['errorField' => 'todo_item_id']); + $rules->add($rules->existsIn(['todo_item_id'], 'TodoItems'), ['errorField' => 'todo_item_id']); return $rules; } diff --git a/tests/comparisons/Model/testBakeUpdateTableNoFile.php b/tests/comparisons/Model/testBakeUpdateTableNoFile.php index 5b037c52..0d262638 100644 --- a/tests/comparisons/Model/testBakeUpdateTableNoFile.php +++ b/tests/comparisons/Model/testBakeUpdateTableNoFile.php @@ -113,7 +113,7 @@ public function validationDefault(Validator $validator): Validator */ public function buildRules(RulesChecker $rules): RulesChecker { - $rules->add($rules->existsIn('user_id', 'Users'), ['errorField' => 'user_id']); + $rules->add($rules->existsIn(['user_id'], 'Users'), ['errorField' => 'user_id']); return $rules; }