-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
fix(client): use objects for DbNull, JsonNull, AnyNull #13783
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
SevInf
approved these changes
Jun 13, 2022
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.
I left couple of questions/suggestions, but LGTM overall.
aqrln
commented
Jun 13, 2022
aqrln
force-pushed
the
prisma4/null-symbols
branch
from
June 14, 2022 10:02
66d92f7
to
422351c
Compare
SevInf
reviewed
Jun 14, 2022
2 tasks
aqrln
force-pushed
the
prisma4/null-symbols
branch
from
June 21, 2022 10:52
441fcf8
to
04ee37d
Compare
millsp
reviewed
Jun 22, 2022
millsp
approved these changes
Jun 22, 2022
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.
Looks great, nice work 👏
aqrln
force-pushed
the
prisma4/null-symbols
branch
from
June 22, 2022 11:58
d5bbd3f
to
d63d760
Compare
aqrln
changed the title
fix(client): use symbols for DbNull, JsonNull, AnyNull
fix(client): use objects for DbNull, JsonNull, AnyNull
Jun 22, 2022
Co-authored-by: Sergey Tatarintsev <tatarintsev@prisma.io>
Co-authored-by: Sergey Tatarintsev <tatarintsev@prisma.io>
This reverts commit 59e18c1.
aqrln
force-pushed
the
prisma4/null-symbols
branch
from
June 23, 2022 12:29
7fd08c1
to
7e8a126
Compare
millsp
reviewed
Jun 23, 2022
millsp
reviewed
Jun 23, 2022
SevInf
reviewed
Jun 23, 2022
millsp
reviewed
Jun 23, 2022
millsp
approved these changes
Jun 23, 2022
SevInf
approved these changes
Jun 23, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes https://github.com/prisma/client-planning/issues/18
Fixes #9243
Make
DbNull
,JsonNull
andAnyNull
special objects.Breaking change: when passing
"DbNull"
,"JsonNull"
and"AnyNull"
strings to JSON fields, they will now always be interpreted as strings (while previously they could be interpreted either as strings or as special values)."DbNull"
,"JsonNull"
and"AnyNull"
strings, please update to usePrisma.DbNull
,Prisma.JsonNull
andPrisma.AnyNull
. If you are already using the named constants (which should be the case for most people since that's how it's documented they should be used), no action is required on your side.Prisma.DbNull
as the value of a JSON field, this indicates a bug in your code which wasn't caught by our types previously. The field you are trying to storeDbNull
in is not nullable in the schema, and a literal"DbNull"
string was stored in the database instead ofNULL
."DbNull"
or"JsonNull"
before using it as a value of a JSON column, this is no longer needed, it is safe to pass it as is. Most people would useString
and notJson
columns in the cases which would have been affected by this, but I'm mentioning it anyway because it was raised as one of the concerns in the original issue.First iteration notes
This implementation is fairly generic on the runtime side, but I had to dumb down the generation side compared to what I had initially.
My first approach was to make it possible to mark any enum as symbol enum, generate a registry of all used symbols in
Prisma.Symbols
namespace on-the-fly as such enums are encountered when we traverse the DMMF and then reference them in each specific enum from that registry.It's important that we can't just declare the symbols as
unique symbol
in each specific enum in isolation because that would make, e.g.,JsonNull
fromJsonNullValueInput
andNullableJsonNullValueInput
incompatible between each other on the type level, while we want to preserve the behavior of string literal enums that makes enum variants with identical names interchangeable across different enums — otherwise it won't be possible to use the global helpers in thePrisma
namespace likePrisma.JsonNull
.However, that didn't play well with those helpers in a different aspect: since they are hardcoded and always present, they would need
Prisma.Symbols.JsonNull
,Prisma.Symbols.DbNull
andPrisma.Symbols.AnyNull
to always be present as well, which, as it turned out, is not always the case, since they would only be generated if JSON fields are used in the schema.Thus I decided to declare the top-level helpers as unique symbols and reference them directly from the
Prisma
namespace in the generated enums.If we need any new symbol enums in the future and we don't want to pollute the
Prisma
namespace with their variants, it will still be possible to re-introduce the symbols namespace and generate the top-level helpers conditionally (possibly even defined either as a reference to a symbol from the registry or as aunique symbol
stub for compatibility, so that all global symbols are always present). However, for now it seems like an overkill.Second iteration notes
DbNull
,JsonNull
andAnyNull
are now special objects instead of symbols. This allows for much better type errors when these values are misused.Before:
After: