-
-
Notifications
You must be signed in to change notification settings - Fork 511
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
Implement oneOf
#3429
Implement oneOf
#3429
Conversation
We are still missing the equivalent of the check in coerce value
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3429 +/- ##
==========================================
+ Coverage 96.51% 96.53% +0.02%
==========================================
Files 511 516 +5
Lines 32965 33174 +209
Branches 5482 5521 +39
==========================================
+ Hits 31815 32025 +210
+ Misses 917 916 -1
Partials 233 233 |
also, I'm not sure this is best approach for us, I'll think about it in the next few days |
CodSpeed Performance ReportMerging #3429 will not alter performanceComparing Summary
|
so far I've added a But I'm not sure on the check 🤔 At the moment we need to use unset, otherwise we'd get an error since some of the keys won't be populated. Maybe we can allow |
spec is here: graphql/graphql-spec#825 |
I don't want to overcomplicate this, but it would be interesting to investigate if we can use import strawberry
@strawberry.type
class Query:
@strawberry.field
def search(self, query: str) -> list[str]:
...
@strawberry.field
@overload
def search(self, something_else: str) -> list[str]:
... (I'm actually not sure this is valid in python either, but just writing it down in case it might be useful in future) |
I think this is almost ready to go! I just need to test it with codegen 😊 |
actually, maybe we should introduce a new decorator to make this easier to use: @strawberry.one_of_input
class ExampleInputTagged:
a: str | None
b: int | None |
/pre-release |
Pre-release👋 Pre-release 0.230.0.dev.1716318708 [2dc1a07] has been released on PyPi! 🚀 poetry add strawberry-graphql==0.230.0.dev.1716318708 |
Pre-release👋 Pre-release 0.227.0.dev.1713463204 [a9b1edd] has been released on PyPi! 🚀 poetry add strawberry-graphql==0.227.0.dev.1713463204 |
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 coding this up!! Looks great
# We are populating all missing keys with `None`, so users | ||
# don't have to set an explicit default for the input type | ||
# The alternative is to tell them to use `UNSET`, but it looks | ||
# a bit unfriendly to use |
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.
FWIW having to manually default to UNSET for optional input fields is the current behavior/expectation (unless I'm mistaken/misusing), so not requiring the same here is a little inconsistent
i.e. you'd have to add UNSET default on normal inputs but not one-of inputs
@strawberry.input
class FooInput:
a: str | None = UNSET
b: str | None = field(default_value=UNSET, description="foo bar")
@strawberry.input(directives=[OneOf()])
class FooInput:
a: str | None
b: str | None = field(description="foo bar")
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.
would you prefer to use UNSET
to keep the consistency? 😊
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 also think UNSET
would be better, as null
can sometimes have meaning
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.
@bellini666 just to clarify, we don't use null
, as in we, don't show it in the schema, it's just a shortcut to prevent to do this:
@strawberry.input(one_of=True)
class FooInput:
a: str | None = strawberry.UNSET
b: str | None = strawberry.UNSET
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.
@patrick91 ah got it! I think I also misread the original comment here
I agree that not requiring setting this to strawberry.UNSET
is an unexpected behavior, but at the same time I can see how this would be justified by the fact that one_of=True
is somewhat implicitly telling that the input need to receive specific one argument.
I will generally choose the consistency path, so requiring to set this to UNSET
would be my option. But I don't have literally nothing against not doing that.
Also, maybe we should reconsider having to do that in first place for strawberry in general? I mean, GraphQL itself will accept null
and UNSET
for optional fields, so maybe we should default anything that doesn't define a default value to UNSET
as long as it is optional? What do you think?
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.
ok! I'll make the change 😊
Also, maybe we should reconsider having to do that in first place for strawberry in general? I mean, GraphQL itself will accept null and UNSET for optional fields, so maybe we should default anything that doesn't define a default value to UNSET as long as it is optional? What do you think?
mmh, this is probably a big breaking change, maybe we can discuss it on a call? we could make this configurable, but I think I need to think about it more 😊
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 will generally choose the consistency path, so requiring to set this to UNSET would be my option. But I don't have literally nothing against not doing that.
+1 on consistency but I don't feel too strongly about it here because "the fact that one_of=True
is somewhat implicitly telling that the input need to receive specific one argument."
Also, maybe we should reconsider having to do that in first place for strawberry in general? I mean, GraphQL itself will accept null and UNSET for optional fields, so maybe we should default anything that doesn't define a default value to UNSET as long as it is optional? What do you think?
Agreed that if an argument is optional I would expect it automatically to default to UNSET, but I also understand this is a big change...
I don't remember what actually happens if you don't set the default to UNSET... I think it generates the schema fine but requires clients to explicitly send null
? I can't remember.
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.
@bradleyoesch sorry for the late reply
I don't remember what actually happens if you don't set the default to UNSET... I think it generates the schema fine but requires clients to explicitly send null? I can't remember.
you can't send explicit null (it's forbidden by the specification), the issue you get is that the __init__
function will complain because we didn't send all the arguments
Oh woops didn't see this was marked pre-release literally hours before making my comments! |
I've tested this with GraphQL-codegen and it works when using the SDL, but doesn't work when using introspection, but that's not our issue 😊 |
Thanks for adding the Here's a preview of the changelog: This release adds support for import strawberry
@strawberry.input(one_of=True)
class ExampleInputTagged:
a: str | None = strawberry.UNSET
b: int | None = strawberry.UNSET Here's the tweet text:
|
Apollo Federation Subgraph Compatibility Results
Learn more: |
/pre-release |
@bradleyoesch feel free to try the new pre-release 😊 |
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.
Looking great! :)
Left a small nit and I agree with the suggestion of using UNSET
instead of null
# We are populating all missing keys with `None`, so users | ||
# don't have to set an explicit default for the input type | ||
# The alternative is to tell them to use `UNSET`, but it looks | ||
# a bit unfriendly to use |
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 also think UNSET
would be better, as null
can sometimes have meaning
Co-authored-by: Thiago Bellini Ribeiro <thiago@bellini.dev>
for more information, see https://pre-commit.ci
for more information, see https://pre-commit.ci
name: String | ||
email: String | ||
} | ||
``` |
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 think supplying the @strawberry.input(directives=[OneOf()])
directive example too would be nice. I could see some developers wanting to use the directive over the argument to keep the code more similar to the generated schema
(or otherwise document the new OneOf()
directive somewhere
@@ -83,6 +85,9 @@ def _impl_type( | |||
if is_interface_object: | |||
directives.append(InterfaceObject()) | |||
|
|||
if one_of: | |||
directives.append(OneOf()) |
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.
Not as familiar with dynamic imports but should from strawberry.schema_directives import OneOf
go in this conditional so it's only imported when used?
Same question in strawberry/object_type.py
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 think it is ok to have the import there, just to keep the code simple 😊
/pre-release |
We are still missing the equivalent of the check in coerce value
Mostly based on https://github.com/graphql/graphql-js/pull/3513/files
Closes #2421 #2560
TODO: