beam-automigrate
beam-automigrate copied to clipboard
obsidiansystems
Annotated foreign key constraint names not unique
When annotating foreign key constraints using foreignKeyOnPk or foreignKeyOn, constraint names are automatically created based on the table and columns of the referenced unique constraint. The name is not necessarily unique because many tables may have foreign key constraints that reference the same unique constraint. The constraint name clashes prevent beam-automigrate from being able to model non-trivial databases.
For example, both OrderT and LineItemT have foreign keys to FlowerT. The OrderT foreign key is annotated with a foreign key constraint. If a foreign key constraint was added for the LineItemT foreign key, the constraint names would clash.
Constraint names should instead be based on the table and columns of the foreign key constraint. This is a breaking change; it changes the names of constraints. I will submit a pull request with the fix.
By the way, this is my second of a series of pull requests. I am submitting them sequentially so that each one can be trivially merged, with no merge conflicts, to minimize the work for the project maintainers. If you prefer that I instead combine fixes for multiple issues in a single pull request, however, please let me know. :smile:
Thank you!
Feel free to submit them all at once, and label them with the order in which we should merge.
I created a tc-develop branch with documentation in the README-tc-develop.md file. The plan is to separate commits with actual changes from documentation commits, so that they can be cherry-picked, and relevant parts of the documentation can be copy/pasted to release notes. I can create pull request branches with cherry-picked commits and label them with the order.
Thank you!
A GTableConstraintColumns instance is used to create the foreign key constraint annotations when automatic foreign key discovery is used. The constraint names are already based on the table and columns of the foreign key constraint, not the referenced unique constraint! There are two other issues, however.
First, the constraint name only takes into account the first column. Though overlapping foreign key constraints are not frequently used, they are valid. In such cases, the constraint names will not be unique, which is a problem. This issue is really easy to fix.
Second, the column pairs are created by zipping sorted column names. This works when using Beam naming conventions, but it may be incorrect in cases where custom column names are used. This is not so easy to fix. Personally, I think that matching column order would be better than sorting by column name. This is currently impossible, however, because the order of columns is determined by Map.toList. One of my other planned pull requests changes the design so that columns are ordered (using Vector), which is necessary for type alignment, which is often required in production databases. Any thoughts?
By the way, NO ACTION is the PostgreSQL default for both ON DELETE and ON UPDATE, so I think that the defaults in this instance are appropriate. (I rarely have constraints that use these defaults in an actual database, however, so it is almost always necessary to disable the automatic foreign key discovery and write the annotations explicitly.)
I would like to go ahead and implement the fix for the first issue today, and include it in the pull request. The second issue is unrelated to the constraint name and can be considered later.
I wrote:
I can create pull request branches with cherry-picked commits and label them with the order.
That did not work well, as the new pull request seems to include the commits of the previous pull request (branch) as well.
It is my first time to submit interdependent PRs. Please excuse my naïveté.