Skip to content

Commit 0c8ecd0

Browse files
HavokInspirationHavokInspiration
committed
Add a failing test to show that Schema reflection does not correctly reflects
composite foreign keys and fix it for all the Schema classes
1 parent 23cffae commit 0c8ecd0

File tree

8 files changed

+179
-45
lines changed

8 files changed

+179
-45
lines changed

‎src/Database/Schema/PostgresSchema.php

Lines changed: 40 additions & 32 deletions
Original file line numberDiff line numberDiff line change
@@ -274,35 +274,38 @@ protected function _convertColumnList($columns)
274274
*/
275275
public function describeForeignKeySql($tableName, $config)
276276
{
277-
$sql = "SELECT tc.constraint_name AS name,
277+
$sql = "SELECT
278+
rc.constraint_name AS name,
278279
tc.constraint_type AS type,
279280
kcu.column_name,
280281
rc.match_option AS match_type,
281-
282282
rc.update_rule AS on_update,
283283
rc.delete_rule AS on_delete,
284-
ccu.table_name AS references_table,
285-
ccu.column_name AS references_field
286-
FROM information_schema.table_constraints tc
287284
288-
LEFT JOIN information_schema.key_column_usage kcu
289-
ON tc.constraint_catalog = kcu.constraint_catalog
290-
AND tc.constraint_schema = kcu.constraint_schema
291-
AND tc.constraint_name = kcu.constraint_name
285+
kc.table_name AS references_table,
286+
kc.column_name AS references_field
287+
288+
FROM information_schema.referential_constraints rc
289+
290+
JOIN information_schema.table_constraints tc
291+
ON tc.constraint_name = rc.constraint_name
292+
AND tc.constraint_schema = rc.constraint_schema
293+
AND tc.constraint_name = rc.constraint_name
292294
293-
LEFT JOIN information_schema.referential_constraints rc
294-
ON tc.constraint_catalog = rc.constraint_catalog
295-
AND tc.constraint_schema = rc.constraint_schema
296-
AND tc.constraint_name = rc.constraint_name
295+
JOIN information_schema.key_column_usage kcu
296+
ON kcu.constraint_name = rc.constraint_name
297+
AND kcu.constraint_schema = rc.constraint_schema
298+
AND kcu.constraint_name = rc.constraint_name
297299
298-
LEFT JOIN information_schema.constraint_column_usage ccu
299-
ON rc.unique_constraint_catalog = ccu.constraint_catalog
300-
AND rc.unique_constraint_schema = ccu.constraint_schema
301-
AND rc.unique_constraint_name = ccu.constraint_name
300+
JOIN information_schema.key_column_usage kc
301+
ON kc.ordinal_position = kcu.position_in_unique_constraint
302+
AND kc.constraint_name = rc.unique_constraint_name
302303
303-
WHERE tc.table_name = ?
304-
AND tc.table_schema = ?
305-
AND tc.constraint_type = 'FOREIGN KEY'";
304+
WHERE kcu.table_name = ?
305+
AND kc.table_schema = ?
306+
AND tc.constraint_type = 'FOREIGN KEY'
307+
308+
ORDER BY rc.constraint_name, kcu.ordinal_position";
306309

307310
$schema = empty($config['schema']) ? 'public' : $config['schema'];
308311
return [$sql, [$tableName, $schema]];
@@ -313,17 +316,13 @@ public function describeForeignKeySql($tableName, $config)
313316
*/
314317
public function convertForeignKeyDescription(Table $table, $row)
315318
{
316-
$data = $table->constraint($row['name']);
317-
if (empty($data)) {
318-
$data = [
319-
'type' => Table::CONSTRAINT_FOREIGN,
320-
'columns' => [],
321-
'references' => [$row['references_table'], $row['references_field']],
322-
'update' => $this->_convertOnClause($row['on_update']),
323-
'delete' => $this->_convertOnClause($row['on_delete']),
324-
];
325-
}
326-
$data['columns'][] = $row['column_name'];
319+
$data = [
320+
'type' => Table::CONSTRAINT_FOREIGN,
321+
'columns' => $row['column_name'],
322+
'references' => [$row['references_table'], $row['references_field']],
323+
'update' => $this->_convertOnClause($row['on_update']),
324+
'delete' => $this->_convertOnClause($row['on_delete']),
325+
];
327326
$table->addConstraint($row['name'], $data);
328327
}
329328

@@ -464,11 +463,20 @@ protected function _keySql($prefix, $data)
464463
$data['columns']
465464
);
466465
if ($data['type'] === Table::CONSTRAINT_FOREIGN) {
466+
if (!is_array($data['references'][1])) {
467+
$data['references'][1] = [$data['references'][1]];
468+
}
469+
470+
$columnsReference = array_map(
471+
[$this->_driver, 'quoteIdentifier'],
472+
$data['references'][1]
473+
);
474+
467475
return $prefix . sprintf(
468476
' FOREIGN KEY (%s) REFERENCES %s (%s) ON UPDATE %s ON DELETE %s DEFERRABLE INITIALLY IMMEDIATE',
469477
implode(', ', $columns),
470478
$this->_driver->quoteIdentifier($data['references'][0]),
471-
$this->_driver->quoteIdentifier($data['references'][1]),
479+
implode(', ', $columnsReference),
472480
$this->_foreignOnClause($data['update']),
473481
$this->_foreignOnClause($data['delete'])
474482
);

‎src/Database/Schema/SqliteSchema.php

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,14 @@
2323
class SqliteSchema extends BaseSchema
2424
{
2525

26+
/**
27+
* Array containing the FK names
28+
* Necessary for composite foreign keys to be handled
29+
*
30+
* @var array
31+
*/
32+
protected $_fkIdMap = [];
33+
2634
/**
2735
* Convert a column definition to the abstract types.
2836
*
@@ -207,6 +215,8 @@ public function describeForeignKeySql($tableName, $config)
207215
*/
208216
public function convertForeignKeyDescription(Table $table, $row)
209217
{
218+
$name = $row['from'] . '_fk';
219+
210220
$update = isset($row['on_update']) ? $row['on_update'] : '';
211221
$delete = isset($row['on_delete']) ? $row['on_delete'] : '';
212222
$data = [
@@ -216,7 +226,13 @@ public function convertForeignKeyDescription(Table $table, $row)
216226
'update' => $this->_convertOnClause($update),
217227
'delete' => $this->_convertOnClause($delete),
218228
];
219-
$name = $row['from'] . '_fk';
229+
230+
if (isset($this->_fkIdMap[$table->name()][$row['id']])) {
231+
$name = $this->_fkIdMap[$table->name()][$row['id']];
232+
} else {
233+
$this->_fkIdMap[$table->name()][$row['id']] = $name;
234+
}
235+
220236
$table->addConstraint($name, $data);
221237
}
222238

@@ -315,10 +331,20 @@ public function constraintSql(Table $table, $name)
315331
}
316332
if ($data['type'] === Table::CONSTRAINT_FOREIGN) {
317333
$type = 'FOREIGN KEY';
334+
335+
if (!is_array($data['references'][1])) {
336+
$data['references'][1] = [$data['references'][1]];
337+
}
338+
339+
$columnsReference = array_map(
340+
[$this->_driver, 'quoteIdentifier'],
341+
$data['references'][1]
342+
);
343+
318344
$clause = sprintf(
319345
' REFERENCES %s (%s) ON UPDATE %s ON DELETE %s',
320346
$this->_driver->quoteIdentifier($data['references'][0]),
321-
$this->_driver->quoteIdentifier($data['references'][1]),
347+
implode(', ', $columnsReference),
322348
$this->_foreignOnClause($data['update']),
323349
$this->_foreignOnClause($data['delete'])
324350
);

‎src/Database/Schema/SqlserverSchema.php

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -415,11 +415,20 @@ protected function _keySql($prefix, $data)
415415
$data['columns']
416416
);
417417
if ($data['type'] === Table::CONSTRAINT_FOREIGN) {
418+
if (!is_array($data['references'][1])) {
419+
$data['references'][1] = [$data['references'][1]];
420+
}
421+
422+
$columnsReference = array_map(
423+
[$this->_driver, 'quoteIdentifier'],
424+
$data['references'][1]
425+
);
426+
418427
return $prefix . sprintf(
419428
' FOREIGN KEY (%s) REFERENCES %s (%s) ON UPDATE %s ON DELETE %s',
420429
implode(', ', $columns),
421430
$this->_driver->quoteIdentifier($data['references'][0]),
422-
$this->_driver->quoteIdentifier($data['references'][1]),
431+
implode(', ', $columnsReference),
423432
$this->_foreignOnClause($data['update']),
424433
$this->_foreignOnClause($data['delete'])
425434
);

‎src/Database/Schema/Table.php

Lines changed: 18 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -511,9 +511,27 @@ public function addConstraint($name, $attrs)
511511

512512
if ($attrs['type'] === static::CONSTRAINT_FOREIGN) {
513513
$attrs = $this->_checkForeignKey($attrs);
514+
515+
if (isset($this->_constraints[$name])) {
516+
$this->_constraints[$name]['columns'] = array_merge(
517+
$this->_constraints[$name]['columns'],
518+
$attrs['columns']
519+
);
520+
521+
if (is_string($this->_constraints[$name]['references'][1])) {
522+
$this->_constraints[$name]['references'][1] = [$this->_constraints[$name]['references'][1]];
523+
}
524+
525+
$this->_constraints[$name]['references'][1] = array_merge(
526+
$this->_constraints[$name]['references'][1],
527+
[$attrs['references'][1]]
528+
);
529+
return $this;
530+
}
514531
} else {
515532
unset($attrs['references'], $attrs['update'], $attrs['delete']);
516533
}
534+
517535
$this->_constraints[$name] = $attrs;
518536
return $this;
519537
}

‎tests/Fixture/CustomersFixture.php

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,11 @@
2323
class CustomersFixture extends TestFixture
2424
{
2525

26+
/**
27+
* {@inheritDoc}
28+
*/
29+
public $table = 'customers';
30+
2631
/**
2732
* fields property
2833
*

‎tests/Fixture/ProductOrdersFixture.php renamed to ‎tests/Fixture/OrdersFixture.php

Lines changed: 12 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -17,12 +17,17 @@
1717
use Cake\TestSuite\Fixture\TestFixture;
1818

1919
/**
20-
* Class ProductOrdersFixture
20+
* Class OrdersFixture
2121
*
2222
*/
23-
class ProductOrdersFixture extends TestFixture
23+
class OrdersFixture extends TestFixture
2424
{
2525

26+
/**
27+
* {@inheritDoc}
28+
*/
29+
public $table = 'orders';
30+
2631
/**
2732
* fields property
2833
*
@@ -47,17 +52,17 @@ class ProductOrdersFixture extends TestFixture
4752
'primary' => [
4853
'type' => 'primary', 'columns' => ['id']
4954
],
50-
'product_order_ibfk_1' => [
55+
'product_id_fk' => [
5156
'type' => 'foreign',
52-
'columns' => ['product_category', 'product_id'],
53-
'references' => ['product', ['category', 'id']],
57+
'columns' => ['product_id', 'product_category'],
58+
'references' => ['products', ['id', 'category']],
5459
'update' => 'cascade',
5560
'delete' => 'cascade',
5661
],
57-
'product_order_ibfk_2' => [
62+
'order_ibfk_2' => [
5863
'type' => 'foreign',
5964
'columns' => ['customer_id'],
60-
'references' => ['customer', 'id'],
65+
'references' => ['customers', 'id'],
6166
'update' => 'cascade',
6267
'delete' => 'cascade',
6368
]

‎tests/Fixture/ProductsFixture.php

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,10 @@
2222
*/
2323
class ProductsFixture extends TestFixture
2424
{
25+
/**
26+
* {@inheritDoc}
27+
*/
28+
public $table = 'products';
2529

2630
/**
2731
* fields property
@@ -42,8 +46,8 @@ class ProductsFixture extends TestFixture
4246
* @var array
4347
*/
4448
public $records = [
45-
['category' => 1, 'name' => 'First product', 'price' => 10],
46-
['category' => 2, 'name' => 'Second product', 'price' => 20],
47-
['category' => 3, 'name' => 'Third product', 'price' => 30]
49+
['id' => 1, 'category' => 1, 'name' => 'First product', 'price' => 10],
50+
['id' => 2, 'category' => 2, 'name' => 'Second product', 'price' => 20],
51+
['id' => 3, 'category' => 3, 'name' => 'Third product', 'price' => 30]
4852
];
4953
}

‎tests/TestCase/Database/Schema/TableTest.php

Lines changed: 59 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,14 +15,19 @@
1515
namespace Cake\Test\TestCase\Database\Schema;
1616

1717
use Cake\Database\Schema\Table;
18+
use Cake\Datasource\ConnectionManager;
19+
use Cake\ORM\TableRegistry;
1820
use Cake\TestSuite\TestCase;
21+
use Symfony\Component\Yaml\Exception\RuntimeException;
1922

2023
/**
2124
* Test case for Table
2225
*/
2326
class TableTest extends TestCase
2427
{
2528

29+
public $fixtures = ['core.customers', 'core.products', 'core.orders'];
30+
2631
/**
2732
* Test construction with columns
2833
*
@@ -405,6 +410,39 @@ public function testAddConstraintForeignKey()
405410
$this->assertEquals(['author_id_idx'], $table->constraints());
406411
}
407412

413+
/**
414+
* Test composite foreign keys support
415+
*
416+
* @return void
417+
*/
418+
public function testAddConstraintForeignKeyTwoColumns()
419+
{
420+
$table = TableRegistry::get('Orders');
421+
$compositeConstraint = $table->schema()->constraint('product_id_fk');
422+
$expected = [
423+
'type' => 'foreign',
424+
'columns' => [
425+
'product_id',
426+
'product_category'
427+
],
428+
'references' => [
429+
'products',
430+
['id', 'category']
431+
],
432+
'update' => 'cascade',
433+
'delete' => 'cascade',
434+
'length' => []
435+
];
436+
437+
$this->assertEquals($expected, $compositeConstraint);
438+
439+
$expectedSubstring = '#CONSTRAINT <product_id_fk> FOREIGN KEY \(<product_id>, <product_category>\)' .
440+
' REFERENCES <products> \(<id>, <category>\)#';
441+
$expectedSubstring = str_replace(['<', '>'], ['[`"\[]', '[`"\]]'], $expectedSubstring);
442+
443+
$this->assertRegExp($expectedSubstring, $table->schema()->createSql(ConnectionManager::get('default'))[0]);
444+
}
445+
408446
/**
409447
* Provider for exceptionally bad foreign key data.
410448
*
@@ -462,4 +500,25 @@ public function testTemporary()
462500
$table->temporary(false);
463501
$this->assertFalse($table->temporary());
464502
}
503+
504+
/**
505+
* Assertion for comparing a regex pattern against a query having its identifiers
506+
* quoted. It accepts queries quoted with the characters `<` and `>`. If the third
507+
* parameter is set to true, it will alter the pattern to both accept quoted and
508+
* unquoted queries
509+
*
510+
* @param string $pattern
511+
* @param string $query the result to compare against
512+
* @param bool $optional
513+
* @return void
514+
*/
515+
public function assertQuotedQuery($pattern, $query, $optional = false)
516+
{
517+
if ($optional) {
518+
$optional = '?';
519+
}
520+
$pattern = str_replace('<', '[`"\[]' . $optional, $pattern);
521+
$pattern = str_replace('>', '[`"\]]' . $optional, $pattern);
522+
$this->assertRegExp('#' . $pattern . '#', $query);
523+
}
465524
}

0 commit comments

Comments
 (0)