Skip to content

Commit

Permalink
Merge pull request #967 from cakephp/3.x-fix-unique-constraints-getti…
Browse files Browse the repository at this point in the history 
…ng-lost

3.next - Fix unique constraints getting lost.
  • Loading branch information
@dereuromark
dereuromark authored Dec 28, 2023
2 parents e7e3c43 + 07a460a commit b5f106f
Show file tree
Hide file tree
Showing 9 changed files with 174 additions and 35 deletions.
49 changes: 33 additions & 16 deletions src/Command/ModelCommand.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -984,47 +984,64 @@ 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) {
continue;
}

$options = [];
$fields = $constraint['columns'];
foreach ($fields as $field) {
/** @var array<string> $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;
break;
}
}

$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;
Expand Down
6 changes: 3 additions & 3 deletions templates/bake/Model/table.twig
View file
Original file line number Diff line number Diff line change
Expand Up @@ -139,13 +139,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;
Expand Down
140 changes: 131 additions & 9 deletions tests/TestCase/Command/ModelCommandTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1453,7 +1453,7 @@ public function testGetRules()
],
[
'alias' => 'Sites',
'foreignKey' => 'site_id',
'foreignKey' => ['site_id1', 'site_id2'],
],
],
'hasMany' => [
Expand All @@ -1467,18 +1467,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' => [],
],
Expand All @@ -1489,9 +1491,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()
Expand All @@ -1501,7 +1500,7 @@ public function testGetRulesUniqueKeys()
'type' => 'unique',
'columns' => ['title'],
]);
$model->getSchema()->addConstraint('ignored_constraint', [
$model->getSchema()->addConstraint('unique_composite', [
'type' => 'unique',
'columns' => ['title', 'user_id'],
]);
Expand All @@ -1510,7 +1509,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' => [],
Expand All @@ -1519,6 +1523,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
*
Expand Down
4 changes: 2 additions & 2 deletions tests/comparisons/Model/testBakeAssociationDetectionCategoriesProductsTable.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/comparisons/Model/testBakeAssociationDetectionProductVersionsTable.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/comparisons/Model/testBakeAssociationDetectionProductVersionsTableSigned.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/comparisons/Model/testBakeTableConfig.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/comparisons/Model/testBakeTableWithCounterCache.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
2 changes: 1 addition & 1 deletion tests/comparisons/Model/testBakeUpdateTableNoFile.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down

0 comments on commit b5f106f

Please sign in to comment.