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; }