Conversation
parsonsmatt
left a comment
parsonsmatt
left a comment
There was a problem hiding this comment.
This should occur in two steps.
First, I want to get a patch release out ASAP for 2.11 that modifies the deprecation message. Users should know to continue pattern matching on PersistDbSpecific even if they've added a case for PersistLiteral. I'll write an issue with the relevant information, since it's a pretty specific code path it has to go down. Users that upgrade, see the patch, and include both patterns will work fine.
Second, we can roll this change out. Since it's a breaking change, it'll go out with 2.12. But users that upgrade directly to 2.12 should not observe any problematic behavior.
Why even do this change?
Uhhhh that's a good question. We have deprecated the PersistDbSpecific constructor, so folks want to get rid of it to clean up warnings. But we're now requiring that you keep it around for backwards compatibility on the data loading side! THat's unfortunate.
So this PR can allow you to have a backwards-compatible deserialization logic while avoiding any deprecated terms.
| render (P (PersistLiteralEscaped e)) = MySQL.Escape e | ||
| render (P (PersistLiteral_ DbSpecific s)) = MySQL.Plain $ BBS.fromByteString s | ||
| render (P (PersistLiteral_ Unescaped l)) = MySQL.Plain $ BBS.fromByteString l | ||
| render (P (PersistLiteral_ Escaped e)) = MySQL.Escape e |
There was a problem hiding this comment.
This needed to be expanded out, because PersistDbSpecific will always match on any PersistLiteral_ constructor. Which means the other two patterns would never hit. Expanding it out works like you should expect.
|
|
||
| instance PersistField PgInterval where | ||
| toPersistValue = PersistLiteralEscaped . pgIntervalToBs | ||
| fromPersistValue (PersistDbSpecific bs) = fromPersistValue (PersistLiteralEscaped bs) |
There was a problem hiding this comment.
This line specifically will loop forever at runtime. A pretty nasty bug, fortunately easy to solve.
| PersistLiteralEscaped bs = PersistLiteral_ Escaped bs | ||
|
|
||
| pattern PersistLiteral bs <- PersistLiteral_ _ bs where | ||
| PersistLiteral bs = PersistLiteral_ Unescaped bs |
There was a problem hiding this comment.
This is the meat of the change here. Note that the pattern match ignores the LiteralType argument.
Therefore, a FromJSON instance can return a PersistLiteral_ DbSpecific bs, and pattern matching code using PersistLiteral bs will work (since it expands to PersistLiteral_ _ bs).
|
I've tried this out on the Mercury codebase, and we were able to load all of our models successfully with this branch. I'm going to move forward with the plan. |
1 participant
Before submitting your PR, check that you've:
@sincedeclarations to the HaddockAfter submitting your PR:
(unreleased)on the ChangelogThis PR provides a fix for #1161. Briefly, we replace the various
PeristLiteralconstructors with pattern synonyms that back thePersistLiteral_constructor, which accepts aLiteralKindvalue. The pattern synonyms can be used to construct a value as expected, but in pattern matching, it actually ignores theLiteralKindvalue.This approach has few downsides, documented in the #1161 discussion. To summarize, a pattern match on
PersistDbSpecific bsexpands into a match onPersistLiteral_ _kind bs, which means that it will always succeed. Patterns that attempted to disambiguate and provide difference behavior based onPersistDbSpecificvsPersistLiteralwill always hit the first case that matches instead of differentiating. Specifically, if it hits aPersistDbSpecific bsand delegates toPersistLiteral bs, then it will loop.