Unify multi-column foreign key handling #2212
Conversation
ndm2
force-pushed
the
1.x-fix-unify-multi-col-fk-handling
branch
from
563266e
to
f2c75ea
Compare
|
As a bookkeeping question, does this need to land in the |
|
Well, I never really understood why Phinx has this 0.x release schema, and I'm not sure when possible breaking changes like this one can be introduced there, especially since 0.next To my understanding 1.x (or whatever it might be released as) is primarily meant to be the CakePHP 5 compatible version, and I stumbled over this multi-column problem while working on the CakePHP 5 compatible version of the Migrations plugin, hence I targeted 1.x here first. If it can go in 0.x too, I could either backport it, or target 0.x and pull it into 1.x via a 0.x -> 1.x merge. |
|
We kept it 0.x to clarify the Api is still somewhat not stable and any major could have some Bagger BC breaking changes than one would expect on stable API. |
There was a problem hiding this comment.
Just some clarifying questions, otherwise I think this code is good to merge, thanks for including comprehensive tests, and that nothing got broken that we already testing for which is nice.
The only potential complaint I could see is that it does potentially make removing all FKs for a given column more annoying, as instead of writing ->dropForeignKey('foo'), you'd maybe have to write several with exact usage. However, I'd argue that's probably better that we force people to be more exact when futzing with their DB, and prevent them from putting themselves into an unexpected state, where they were actually looking to drop the FK on (foo) but not (foo, bar), but not realizing it.
We'll want to figure out what the versioning story for phinx is before we merge this, since I guess we're now on v2, and this'll put us on v3?
| @@ -952,7 +952,7 @@ protected function getForeignKeys(string $tableName): array | |||
| JOIN information_schema.key_column_usage AS kcu ON tc.constraint_name = kcu.constraint_name | |||
| JOIN information_schema.constraint_column_usage AS ccu ON ccu.constraint_name = tc.constraint_name | |||
| WHERE constraint_type = 'FOREIGN KEY' AND tc.table_schema = %s AND tc.table_name = %s | |||
| ORDER BY kcu.position_in_unique_constraint", | |||
| ORDER BY kcu.ordinal_position", | |||
There was a problem hiding this comment.
For my understanding, we don't want to use position_in_unique_constraint as this would give us the order of columns within the index clause on the reference table, while ordinal_position gives us the order within the FK on the table, and that these two orders are not guaranteed to match.
There was a problem hiding this comment.
Correct, the order of columns on the implicitly created index on the target table will always be in the order in which the columns appear in the source table definition, irrespective of the order of columns in the foreign key constraint definition.
| @@ -962,6 +962,10 @@ protected function getForeignKeys(string $tableName): array | |||
| $foreignKeys[$row['constraint_name']]['referenced_table'] = $row['referenced_table_name']; | |||
| $foreignKeys[$row['constraint_name']]['referenced_columns'][] = $row['referenced_column_name']; | |||
| } | |||
| foreach ($foreignKeys as $name => $key) { | |||
| $foreignKeys[$name]['columns'] = array_values(array_unique($key['columns'])); | |||
| $foreignKeys[$name]['referenced_columns'] = array_values(array_unique($key['referenced_columns'])); | |||
There was a problem hiding this comment.
You've added this since you were seeing duplicates from the above query?
There was a problem hiding this comment.
Correct, due to the nature of the joins, every additional column will cause a duplicate, so a foreign key over foo, bar, baz would result in a columns list holding foo, foo, foo, bar, bar, bar, baz, baz, baz.
| @@ -224,7 +224,7 @@ public function hasTable(string $tableName): bool | |||
| } | |||
|
|
|||
| /** @var array<string, mixed> $result */ | |||
| $result = $this->fetchRow(sprintf("SELECT count(*) as [count] FROM information_schema.tables WHERE table_name = '%s';", $tableName)); | |||
| $result = $this->fetchRow(sprintf("SELECT count(*) as [count] FROM INFORMATION_SCHEMA.TABLES WHERE TABLE_NAME = '%s';", $tableName)); | |||
There was a problem hiding this comment.
Capitalizing all these references on the case using a case-sensitive SQL Server installation, which then requires using uppercase?
There was a problem hiding this comment.
Correct, otherwise it will trigger an error. I also tried to make the tests run on a case sensitive database, but somehow in the workflow environment using a case sensitive collation always causes the server to crash, while it's working fine locally. So far I wasn't able to figure out what the problem is.
MasterOdin
changed the title
I don't really know, I was kinda blindsided by the CakePHP 5 release, I must've missed the discussion to change the release date, I was under the impression it would be shortly before/after Cakefest. So I don't really know what do with this now 🤷 |
Multi-column foreign key support is currently very much broken, and there is no consistency in the behavior of
dropForeignKey()andhasForeignKey()accross the various DBMS.Currently the behavior looks like this:
1) In a case sensitive database the query would produce an error.
2) Generates duplicate drop instructions, one for every key that has any of the given columns.
3) For every column, the first key that starts with that column would be deleted.
With these changes, the new behavior would be as follows:
1) The behavior is independent of whether the database/column is actually case-sensitive.
My reasoning for making dropping and looking up foreigny keys by column require to pass the columns in the exact order in which the columns appear in the foreign key definition, is the fact that it is possible to define multiple foreign keys using the same columns. Whether it makes sense to do that, well, I don't really know, but it's very much possible, so I'd rather be strict about it, and have for example
dropForeignKey(['a', 'b'])only drop foreign keys that where defined as(a, b), and not the ones defined as(b, a). In the end consistency might be more important though, so please feel free to disagree, changing the behavior is easy enough (Sqlite might be a bit tricky).Dropping all foreign keys that match the given columns in the correct order might be contentiuous. Not doing so would however mean that one would have to generate one call for each of those keys, and there would not really be any control over which key is being deleted exactly, it would simply be the first match, so it's not like one could drop only a specific key in a group of keys with the same order of columns anyways.
On a related note, the foreign key lookup queries for Postgres and SqlServer produce a lot of duplicates, so those queries might need to be optimized, but I don't really wanted to touch that right now, there's too many pitfalls for my taste, so I've chosen the easy route and just reduced the results, that's good enough to me for now. I only fixed up the ordering for Postgres, as it was sorting by the column order in the related unique constraint, not by the column order in the actual foreign key constraint.
Also this PR relaxes the foreign key SQL parsing for Sqlite, for better compatibility with existing databases that may not adhere to the exact syntax that Phinx creates.