Skip to content

Conversation

@hdgarrood
Copy link
Contributor

@hdgarrood hdgarrood commented Dec 11, 2025

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 Column values, 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:

  • Update the Changelog.md file with a link to your PR
  • Bumped the version number if there isn't an (unreleased) on the Changelog
  • Check that CI passes (or if it fails, for reasons unrelated to your change, like CI timeouts)

@hdgarrood hdgarrood marked this pull request as ready for review December 12, 2025 13:06
@hdgarrood
Copy link
Contributor Author

This needs a bit more work to work around #1615

@hdgarrood
Copy link
Contributor Author

hdgarrood commented Dec 12, 2025

Ok - confirmed that the tests fail on the new ExplicitPrimaryKey entity if I get rid of this line:

    refAdd _ | fmap fieldDB (getEntityIdField edef) == Just (cName newCol) = []

and pass with it.

Copy link
Collaborator

@parsonsmatt parsonsmatt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

hell yeah

@parsonsmatt parsonsmatt merged commit bb5b7b5 into yesodweb:master Dec 12, 2025
10 checks passed
-- 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) = []
Copy link
Contributor Author

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.

@hdgarrood hdgarrood deleted the hdgarrood/suggest-foreign-key-constraints-first-time branch December 12, 2025 19:05
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

Successfully merging this pull request may close these issues.

2 participants