-
-
Notifications
You must be signed in to change notification settings - Fork 571
Type-safe "optional-nullable" fields #3779
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
Comments
To clarify, is the following the use case you're interested in? mutation Update {
updatePerson( ## this operation sets phone to null
input: {
id: ...,
name: ...,
phone: null
}
) {
...
}
} vs mutation Update {
updatePerson( ## this operation does not modify phone number
input: {
id: ...,
name: ...
}
) {
...
}
} |
yep |
I think this is a solid proposal. Both For an opt-in approach, I would prefer your solution: strawberry.register_option_type((MyOptionType, NOTHING)) If this Type is meant to be included directly in Strawberry, I would avoid forking the repository. Instead, I’d suggest copying the relevant MIT-licensed code while ensuring proper attribution, such as adding credits in the form of code comments or including a reference to the original license. |
@patrick91 👀 ? |
Code sample in pyright playground from __future__ import annotations
import dataclasses
from typing import Self, reveal_type
@dataclasses.dataclass
class Some[T]:
value: T
def some(self) -> Self:
return self
@dataclasses.dataclass
class Nothing[T]:
def some(self) -> None:
return None
type Maybe[T] = Some[T | None] | Nothing[T]
def some_func() -> Maybe[str]:
return Some("a")
a = some_func()
reveal_type(a)
if phone := a.some():
reveal_type(phone)
reveal_type(phone.value)
else:
reveal_type(phone) |
I have met the exact same problem, but I don't understand why you cannot solve it.
The case where you do not want None but either a string or nothing cannot be handled cleanly due to the GraphQL specification (an optional value is a nullable value**), but if your API is used only internally, you can do the same thing:
(Arguably you could write custom code to ensure that sending a None would return an appropriate error instead of randomly breaking inside your code) I find the proposition slightly contrived, as it turns scalar values into objects. |
phone: str | None | UnsetType can actually solve this, I haven't thought about this really... Though:
phone: str | None = strawberry.UNSET You could say that we can change the signature to the real type but that would type-checkers would lint
We could also think re-introducing the Unset usage which could look like this type Maybe[T] = UnsetType | None | T
def is_unset[T](val: T | UnsetType) -> TypeGuard[UnsetType]:
return isinstance(val, UnsetType)
@strawberry.input
class UpdatePersonInput:
phone: Maybe[str]
def update_person(input_: UpdatePersonInput) -> ...:
if not strawberry.is_unset(input_.phone)
update_phone(input_.phone) I actually have no oppositions to this. |
Could we achieve the same result with the Maybe class & changing the type of the Unset singleton? Then direct identity checks can continue to be supported & there is no need for helper methods? from typing import Final
class UnsetType:
__instance: Optional["UnsetType"] = None
def __new__(cls: type["UnsetType"]) -> "UnsetType":
if cls.__instance is None:
ret = super().__new__(cls)
cls.__instance = ret
return ret
return cls.__instance
def __str__(self) -> str:
return ""
def __repr__(self) -> str:
return "UNSET"
def __bool__(self) -> bool:
return False
~~UNSET: Any = UnsetType()~~
UNSET: Final[UnsetType] = UnsetType() Would we need to remove the falsy behaviour as well? 🤔 Just thinking if 0 is passed & Unset is falsy, then we would miss that 0 value in some contexts? This would return 42 instead of 0? def foo(val: Maybe[int]) -> int:
return val or 42 Existing code should only break if it was already relying on |
Im not sure what is your suggestion. the approach I took is similar to what @Corentin-Bravo suggested.
see the Release notes in #3791
|
I do not understand why helper functions are required if it is possible to use direct identity checks with Python's @strawberry.field
def greet(self, name: Optional[str] = strawberry.UNSET) -> str:
if name is strawberry.UNSET:
return "Name was not set!"
if name is None:
return "Name was null!"
return f"Hello {name}!" Code will need to be changed to allow for type safety with the helper functions. I understand that changing strawberry's Including type For optional nullable fields I would suggest: type Maybe[T] = T | UnsetType
field: Maybe[str] | None This allows type In another issue, we should look at the Falsy behaviour of type |
hmm are you certain about that? @patrick91 WDYT?
AFAICT there is no usecase for |
I think I have expressed all possible cases below In python:
In GraphQL:
The graphqQL schema does not know None nor UNSET, so effectively:
Are all equal for the schema. Therefore, I believe we should not try to be smarter than the underlying GraphQL schema. Maybe[T] should include None, because the schema will allow it. While I agree that it would be better to be able to mark things as optional and nullable separately, GraphQL itself does not allow it. The proposed PR is just syntaxic sugar, and I think it cannot be more than that. |
I understand that GraphQL's schema cannot distinguish between the 3 types above, however, I do not believe that to be a reason to limit Strawberry's type safety when Python can distinguish between the 3. The above examples show the problem & the need for better type safety. I do not believe that limiting a capable type system is the correct approach. A reason for @strawberry.input
class UpdateUserInput:
id: strawberry.ID
name: Maybe[str | None] # Can be UNSET, str or None
bio: Maybe[str] # Can be UNSET, str, but CANNOT be None
@strawberry.mutation
def update_user(self, input: UpdateUserInput) -> User:
user = get_user(input.id)
if input.name is not UNSET:
user.name = input.name # Type checker knows type is str or None
if input.bio is not UNSET:
user.bio = input.bio # Type checker knows type is str & cannot be None
return user A problem with @strawberry.input
class UpdateUserInput:
id: strawberry.ID
name: Maybe[str] # Can be UNSET, str or None
bio: Maybe[str] # Can be UNSET, str or None
@strawberry.mutation
def update_user(self, input: UpdateUserInput) -> User:
user = get_user(input.id)
if input.name is not UNSET:
user.name = input.name # Could be str or None
if input.bio is not UNSET:
user.bio = input.bio # Could be str or None
return user
We can gradually adopt the type annotation for |
The graphql representation of bio will have to be I believe it's better to lose a bit of consistency in the code (using None as "unset" when it cannot be a value, but use UNSET when None is a valid value) rather than having a graphql schema that lies. |
I think this is absolutly valid. This is the reason why type-checkers exist. my2c |
I believe we're seeing this issue appear in issue #3785 where the 'nullability' of
Strawberry's
import strawberry
@strawberry.input(one_of=True)
class SearchBy:
name: str | None = strawberry.UNSET
email: str | None = strawberry.UNSET Which creates the correct GraphQL schema for input SearchBy @oneOf {
name: String
email: String
} However, In this case, type I believe this supports |
* feat: Type-safe "optional-nullable" fields #3779 * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * Rename `isnt_unset` to `not_unset`; inffer use of Maybe and inject UNSET automatically. * add docs * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * feat: enhance Maybe type handling and add typecheck tests * [pre-commit.ci] auto fixes from pre-commit.com hooks for more information, see https://pre-commit.ci * support maybe in field arguments. update docs. * minor doc update * refactor: rename `not_unset` to `exists` * refactor: update type annotations to use Union for optional fields * py 3.9 compat * typing lints * migrate to Maybe & Some * wip: migrate to Maybe and Some * handle maybe arguments on field resolvers properly. (don't use UNSET) * fix mypy * nit * Update release notes, add tests for match * resolve nits * Address review * Remove test for 3.10+ * Add tweet file * Fix typecheckers --------- Co-authored-by: pre-commit-ci[bot] <66853113+pre-commit-ci[bot]@users.noreply.github.com> Co-authored-by: Patrick Arminio <[email protected]>
Hey! I stumbled upon this due to the recent release. I was a bit confused at first as to why the new I think @ctorrington makes a strong argument. @Corentin-Bravo said that:
But in practice, with what tools we have now, I have confusing inconsistency in python code AND I have a schema that lies anyway. I can't speak for everyone, but I come from https://github.com/strawberry-graphql/strawberry-django. For many, it will actually be a very common scenario to require @ctorrington found other examples of lies already built into strawberry. We will not escape from this. In fact, I'd much rather have a built in mechanism and fail early during schema validation or something than have to rely on Django/DB to validate (or having to write custom code myself). So why not give us, python devs, tools to have explicit clarity and consistency? What @ctorrington proposed looks solid. @patrick91 is there any chance to revisit this? |
I'll review this with @bellini666 tomorrow 😊 |
Uh oh!
There was an error while loading. Please reload this page.
Preface
it is a common practice in strawberry that when your data layer have
an optional field i.e
and you want to update it you would use
UNSET
in the mutation inputin order to check whether this field provided by the client or not like so:
Note that this is not an optimization rather a business requirement.
if the user wants to nullify the phone it won't be possible other wise
OTOH you might nullify the phone unintentionally.
This approach can cause lots of bugs since you need to remember that you have
used
UNSET
and to handle this correspondingly.Since strawberry claims to
it is only natural for us to provide a typesafe way to mitigate this.
Proposal
The
Option
type.which will require only this minimal implementationand this is how you'd use it
Currently if you want to know if a field was provided
Backward compat
UNSET
can remain as is for existing codebases.Option
would be handled separately.which
Option
library should we use?The sad truth is that there are no well-maintained libs in the ecosystem.
Never the less it is not hard to maintain something just for strawberry since the implementation
is rather straight forward and not much features are needed. we can fork either
and just forget about it.
then strawberry could use that and you could use whatever you want.
The text was updated successfully, but these errors were encountered: