Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Annotated foreign key constraint names not unique #38

Open
TravisCardwell opened this issue May 25, 2022 · 5 comments
Open

Annotated foreign key constraint names not unique #38

TravisCardwell opened this issue May 25, 2022 · 5 comments

Comments

@TravisCardwell
Copy link
Contributor

TravisCardwell commented May 25, 2022

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.


tc-develop Documentation

TravisCardwell added a commit to TravisCardwell/beam-automigrate that referenced this issue May 25, 2022
This changes foreign key constraint names to be based on the table and
columns of the foreign key, so that they are unique.  This is a breaking
change.
@TravisCardwell
Copy link
Contributor Author

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. 😄

Thank you!

@madeline-os
Copy link
Contributor

Feel free to submit them all at once, and label them with the order in which we should merge.

TravisCardwell added a commit to TravisCardwell/beam-automigrate that referenced this issue May 27, 2022
This changes foreign key constraint names to be based on the table and
columns of the foreign key, so that they are unique.  This is a breaking
change.
@TravisCardwell
Copy link
Contributor Author

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!

@TravisCardwell
Copy link
Contributor Author

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.

TravisCardwell added a commit to TravisCardwell/beam-automigrate that referenced this issue May 27, 2022
This changes the foreign key constraint names created by automatic
foreign key discovery to include all constraint columns instead of just
the first one, so that they are unique.  This is a breaking change.
TravisCardwell added a commit to TravisCardwell/beam-automigrate that referenced this issue May 27, 2022
This changes the foreign key constraint names created by automatic
foreign key discovery to include all constraint columns instead of just
the first one, so that they are unique.  This is a breaking change.
@TravisCardwell
Copy link
Contributor Author

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é.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants