-
Notifications
You must be signed in to change notification settings - Fork 301
Add support for constraint= attribute to override foreign reference constraint names #937
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
Conversation
…quired to support constraint= attribute for foreign key references.
…tation to Persistent-entity-syntax.md
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.
Code looks great! Version bump sounds reasonable.
Could this break anything? You're only adding a bit of functionality. Old versions of persistent will ignore the constraint=. It will generate a migration to rename a constraint if the constraint= is used, I think, but that's expected.
I don't think it can break anything on further thought. I'll merge this in later today. Thanks!
| AnotherVeryLongTableName | ||
| veryLongTableNameId VeryLongTableNameId constraint=short_foreign_key | ||
| ``` | ||
|
|
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.
Thanks for extending the docs!
| pure (DBName x, constraintName) | ||
| | Just x <- T.stripPrefix "constraint=" a = do | ||
| tableName <- fst <$> (ref c fe as) | ||
| pure (tableName, DBName x) |
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.
Oh, I see how this is implemented now! It doesn't require a breaking change addition to a datatype, which is great. It adds a new feature to the QuasiQuoter syntax, but doesn't change the persistent-template library at all.
So if you have a QuasiQuoter block with constraint=fk_name on an old version of persistent, then it'll get ignored. That's okay.
| _ -> [] | ||
| refAdd = case (ref == ref', ref) of | ||
| (False, Just (tname, _cname)) -> [(tname, addReference allDefs (refName tblName name) tname name)] | ||
| (False, Just (tname, cname)) -> [(tname, addReference allDefs cname tname name)] |
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 looks great to me. I think I would like to see it included in the other SQL backends, but that can come in a later PR.
This change adds support for a
constraint=attribute that allows users to override the automatically generated foreign reference constraint names with their own. The motivation for this change is that we had two instances of very long table/column names that led to automatically generated constraint names exceeding 64 characters, which is not allowed in MySQL. Without a change like this we could not use theprintMigrationsoutput.Please let myself (@charukiewicz) or @belevy know if you have suggestions for a better solution or alternative approach.
Before submitting your PR, check that you've:
@sincedeclarations to the HaddockAfter submitting your PR: