Handle foreign key constraint names over 63 characters for Postgres#996
Handle foreign key constraint names over 63 characters for Postgres#996parsonsmatt merged 4 commits intomasterfrom
Conversation
I'd prefer to add documentation and Ideally I'd like to factor out the internal stuff into a |
parsonsmatt
left a comment
parsonsmatt
left a comment
There was a problem hiding this comment.
We're missing a changelog entry for persistent itself. I'd appreciate doc comments and since annotations on newly introduced terms as well, even if they're internal.
Great PR, thanks for the work! 😄
| {-# LANGUAGE ScopedTypeVariables #-} | ||
| {-# LANGUAGE TypeFamilies #-} | ||
| {-# LANGUAGE ViewPatterns #-} | ||
| {-# LANGUAGE MultiWayIf #-} |
There was a problem hiding this comment.
I'd rather not introduce this extension - the use-site doesn't require it.
| shortenNames overhead (x, y) = | ||
| if | x + y + overhead <= maximumIdentifierLength -> (x, y) | ||
| | x > y -> shortenNames overhead (x - 1, y) | ||
| | otherwise -> shortenNames overhead (x, y - 1) |
There was a problem hiding this comment.
This can be accomplished with ordinary guards.
shortenNames overhead (x, y)
| x + y + overhead <= maximumIdentifierLenght = (x, y)
| x > y = shortenNames overhead (x - 1, y)
| otherwise = shortenNames overhead (x, y - 1)| ref c fe [] | ||
| | ForeignRef f _ <- fe = | ||
| Just (resolveTableName allDefs f, refName tn c) | ||
| Just (resolveTableName allDefs f, refNameFn tn c) |
There was a problem hiding this comment.
Ah! We have been ignoring the user-provided name of the foreign key reference this entire time?? Dang. Welp. Fixing that is out-of-scope for this PR.
There was a problem hiding this comment.
Oh, hah, didn’t even notice
There was a problem hiding this comment.
maybe the constraint name override can be pushed to master now since that is a backwards compatible workaround. We only implemented it for MySQL since that is the backend we use and were running into the length issue but didn't have the resources to dedicate towards a proper solution.
|
Ok, I think this is good to go (?) |
MaxGabriel
force-pushed
the
fixTooLongRefName2
branch
from
24b6a1c to
d37d4ad
Compare
|
Since this is a breaking change for |
|
Alright, I'm getting |
|
Oh thanks for getting this in! Sorry I often miss github notifications, feel free to ping me on FPChat slack if I don't respond |
3 participants
Currently, persistent-postgresql suggests foreign key constraint names that are over Postgres' length limit of 63 bytes. Postgres helpfully truncates these names, but this causes persistent-postgresql to think the migration to create the constraint has not been applied, and keep recommending that the migration be applied.
To solve this issue, this PR has persistent-postgresql truncate foreign key constraint names using the same algorithm that Postgres itself uses.
MySQL should also have something like this, but for a 64 character limit. It could use the same one Postgres does, or custom logic for itself.
Before submitting your PR, check that you've:
Internalmodule I'm not sure this is necessary? They're only meant to be used by Persistent backends, is my understanding.@sincedeclarations to the Haddock — I did not do this, but because the new APIs are in anInternalmodule I'm not sure this is necessary? They're only meant to be used by Persistent backends, is my understanding.After submitting your PR: