cakephp / phinx

PHP Database Migrations for Everyone
https://phinx.org
MIT License
4.46k stars 892 forks source link

Unify multi-column foreign key handling #2212

Closed ndm2 closed 12 months ago

ndm2 commented 1 year ago

Multi-column foreign key support is currently very much broken, and there is no consistency in the behavior of dropForeignKey() and hasForeignKey() accross the various DBMS.

Currently the behavior looks like this:

MySQL Postgres Sqlite SQL Server
Case-sensitive drop no yes yes no¹
Case-sensitive lookup yes yes no yes¹
Multi-column drop no² yes no³ no²
Drop all column matches yes yes no yes
Column order dependent drop no no no no
Column order dependent lookup yes no no no
Silently drop non-existent by columns no yes no yes
Silently drop non-existent by name no no - no

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:

MySQL Postgres Sqlite SQL Server
Case-sensitive drop no yes¹ no yes¹
Case-sensitive lookup no yes¹ no yes¹
Multi-column drop yes yes yes yes
Drop all column matches yes yes yes yes
Column order dependent drop yes yes yes yes
Column order dependent lookup yes yes yes yes
Silently drop non-existent by columns no no no no
Silently drop non-existent by name no no - no

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.

MasterOdin commented 1 year ago

As a bookkeeping question, does this need to land in the 1.x branch, or could we land it in 0.x and have this be put out with a new version there? I'm not sure we've totally established what 1.x is.

ndm2 commented 1 year ago

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 ~seems unused~ doesn't exist anymore.

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.

dereuromark commented 1 year ago

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.

ndm2 commented 1 year ago

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?

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 🤷