-
Notifications
You must be signed in to change notification settings - Fork 301
postgresql: Add foreign key constraints the first time when migrating #1614
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
postgresql: Add foreign key constraints the first time when migrating #1614
Conversation
|
This needs a bit more work to work around #1615 |
|
Ok - confirmed that the tests fail on the new refAdd _ | fmap fieldDB (getEntityIdField edef) == Just (cName newCol) = []and pass with it. |
parsonsmatt
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
hell yeah
| -- This check works around a bug where persistent will sometimes | ||
| -- generate an erroneous ForeignRef for ID fields. | ||
| -- See: https://github.com/yesodweb/persistent/issues/1615 | ||
| refAdd _ | fmap fieldDB (getEntityIdField edef) == Just (cName newCol) = [] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This guard is basically the same as it was before - before, we were checking against the old column's name, and now we're checking against the new column's name, because there won't always be an old column name to check against (eg in the event that the column doesn't yet exist). There's no meaningful difference - previously, we would only ever run this check after verifying that the old column name matched the new column name.
I'm pretty sure there isn't an issue for this already, which is a bit surprising! But in postgresql, when you add a foreign key column to an existing table, you need to generate migrations twice to get it fully set up: the first run only creates the column, and the second creates the FK constraint.
This PR adds a test demonstrating the issue as well as removing a guard which I'm reasonably confident isn't necessary (see #1169 (comment)). The commit titled "refactor: don't match on all of the Column fields" also does a refactor to avoid matching on each of the fields of the
Columnvalues, so that it's obvious which values come from the old vs new column, since I found it quite hard to get my head around what this function was doing to start with, but I'm happy to revert that one if you don't think it's an improvement. I recommend reviewing that commit separately from the others.After submitting your PR:
(unreleased)on the Changelog