Implement `oneOf` #3429

patrick91 · 2024-03-31T09:20:52Z

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:

Check print of schema directive
Test with a frontend library
schema codegen support
Introspection support?

We are still missing the equivalent of the check in coerce value

codecov · 2024-03-31T09:25:12Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.53%. Comparing base (fc322a8) to head (a79e961).
Report is 2 commits behind head on main.

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

patrick91 · 2024-03-31T09:25:45Z

also, I'm not sure this is best approach for us, I'll think about it in the next few days

codspeed-hq · 2024-03-31T09:27:47Z

CodSpeed Performance Report

Merging #3429 will not alter performance

_{Comparing feature/one-of (a79e961) with main (a8b108e)}

Summary

✅ 12 untouched benchmarks

patrick91 · 2024-03-31T15:31:37Z

so far I've added a out_type on the GraphQL input which seems to be the best place to check for one of.

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 None and change the check? I need to read the spec 😊

patrick91 · 2024-03-31T15:34:16Z

spec is here: graphql/graphql-spec#825

patrick91 · 2024-03-31T21:42:50Z

I don't want to overcomplicate this, but it would be interesting to investigate if we can use @overload for this too, something like:

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)

patrick91 · 2024-04-17T20:13:54Z

I think this is almost ready to go! I just need to test it with codegen 😊

patrick91 · 2024-04-17T20:18:34Z

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

patrick91 · 2024-04-18T17:59:17Z

/pre-release

botberry · 2024-04-18T18:00:02Z

Pre-release

👋

Pre-release 0.230.0.dev.1716318708 [2dc1a07] has been released on PyPi! 🚀
You can try it by doing:

poetry add strawberry-graphql==0.230.0.dev.1716318708

botberry · 2024-04-18T18:00:27Z

Pre-release

👋

Pre-release 0.227.0.dev.1713463204 [a9b1edd] has been released on PyPi! 🚀
You can try it by doing:

poetry add strawberry-graphql==0.227.0.dev.1713463204

bradleyoesch

Thanks for coding this up!! Looks great

strawberry/object_type.py

strawberry/schema/execute.py

bradleyoesch · 2024-04-18T20:58:39Z

strawberry/schema/schema_converter.py

+            # 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


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")

would you prefer to use UNSET to keep the consistency? 😊

I also think UNSET would be better, as null can sometimes have meaning

@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

@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?

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 😊

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.

@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

bradleyoesch · 2024-04-18T21:02:05Z

/pre-release

Oh woops didn't see this was marked pre-release literally hours before making my comments!

patrick91 · 2024-04-18T21:06:17Z

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 😊

botberry · 2024-04-18T21:19:49Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

This release adds support for @oneOf on input types! 🎉 You can use
one_of=True on input types to create an input type that should only have one
of the fields set.

import strawberry


@strawberry.input(one_of=True)
class ExampleInputTagged:
    a: str | None = strawberry.UNSET
    b: int | None = strawberry.UNSET

Here's the tweet text:

🆕 Release (next) is out! Thanks to @patrick91 for the PR 👏


We now support the `@oneOf` directive on input types, ensuring exactly one
key is specified.

Check out the details and improvements! Get it here 👉 https://strawberry.rocks/release/(next)

botberry · 2024-04-18T21:20:28Z

Apollo Federation Subgraph Compatibility Results

Federation 1 Support Federation 2 Support

`_service`	🟢
`@key (single)`	🟢
`@key (multi)`	🟢
`@key (composite)`	🟢
`repeatable @key`	🟢
`@requires`	🟢
`@provides`	🟢
`federated tracing`	🔲

`@link`	🟢
`@shareable`	🟢
`@tag`	🟢
`@override`	🟢
`@inaccessible`	🟢
`@composeDirective`	🟢
`@interfaceObject`	🟢

Learn more:

patrick91 · 2024-04-18T21:25:42Z

/pre-release

patrick91 · 2024-04-18T21:26:08Z

@bradleyoesch feel free to try the new pre-release 😊

bellini666

Looking great! :)

Left a small nit and I agree with the suggestion of using UNSET instead of null

bellini666 · 2024-04-20T15:29:46Z

strawberry/schema/schema_converter.py

+            # 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


I also think UNSET would be better, as null can sometimes have meaning

strawberry/types/types.py

Co-authored-by: Thiago Bellini Ribeiro <thiago@bellini.dev>

for more information, see https://pre-commit.ci

bradleyoesch · 2024-04-22T14:43:25Z

docs/types/input-types.md

+  name: String
+  email: String
+}
+```


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

bradleyoesch · 2024-04-22T14:44:51Z

strawberry/federation/object_type.py

@@ -83,6 +85,9 @@ def _impl_type(
    if is_interface_object:
        directives.append(InterfaceObject())

+    if one_of:
+        directives.append(OneOf())


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

I think it is ok to have the import there, just to keep the code simple 😊

patrick91 · 2024-05-21T19:11:02Z

/pre-release

Initial implementation of supporting oneOf

4225e4f

We are still missing the equivalent of the check in coerce value

patrick91 added 3 commits March 31, 2024 16:53

Validate one of

e9d70c8

Add missing is_one_of

c1bdb99

Lint

a40f9db

Tests based on RFC

3f35667

patrick91 added 9 commits April 1, 2024 12:21

No need to use = UNSET anymore

d707782

Fix camel casing

fe5f8a0

Fix validation rule

03b3f42

Remove unused code

eba231b

Merge branch 'main' into feature/one-of

1ed031a

Add test for directive

82c890a

Extend introspection

6d0cd58

Ignore type

500d1ac

Add missing type annotation

b4fb169

Add release notes

ce0c61f

patrick91 added 3 commits April 18, 2024 19:50

Add one_of_input

2c2fef7

Update release notes

d325cdd

Add docs

a9b1edd

bradleyoesch reviewed Apr 18, 2024

View reviewed changes

patrick91 added 3 commits April 18, 2024 23:06

Safer assign

1a61ad3

Replace new decorator with flag

385edc5

Test for federation

c54fd74

patrick91 marked this pull request as ready for review April 18, 2024 21:18

botberry added bot:has-release-file bot:release-type-minor labels Apr 18, 2024

patrick91 requested a review from bellini666 April 18, 2024 21:22

bellini666 approved these changes Apr 20, 2024

View reviewed changes

patrick91 and others added 5 commits April 20, 2024 18:23

Update strawberry/types/types.py

529a092

Co-authored-by: Thiago Bellini Ribeiro <thiago@bellini.dev>

[pre-commit.ci] auto fixes from pre-commit.com hooks

3466dd8

for more information, see https://pre-commit.ci

Merge branch 'main' into feature/one-of

e80124c

Explicit unset

393b31d

[pre-commit.ci] auto fixes from pre-commit.com hooks

302d38f

for more information, see https://pre-commit.ci

bradleyoesch approved these changes Apr 22, 2024

View reviewed changes

Merge branch 'main' into feature/one-of

2dc1a07

Add tweet file

a79e961

patrick91 merged commit 82eeaa5 into main May 22, 2024
65 checks passed

patrick91 deleted the feature/one-of branch May 22, 2024 17:32

patrick91 mentioned this pull request May 25, 2024

Add oneOf Input Objects directive (#2421) #2560

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `oneOf` #3429

Implement `oneOf` #3429

patrick91 commented Mar 31, 2024 •

edited

codecov bot commented Mar 31, 2024 •

edited

patrick91 commented Mar 31, 2024

codspeed-hq bot commented Mar 31, 2024 •

edited

patrick91 commented Mar 31, 2024

patrick91 commented Mar 31, 2024

patrick91 commented Mar 31, 2024

patrick91 commented Apr 17, 2024

patrick91 commented Apr 17, 2024

patrick91 commented Apr 18, 2024

botberry commented Apr 18, 2024 •

edited

botberry commented Apr 18, 2024

bradleyoesch left a comment

bradleyoesch Apr 18, 2024

patrick91 Apr 18, 2024

bellini666 Apr 20, 2024

patrick91 Apr 20, 2024

bellini666 Apr 20, 2024 •

edited

patrick91 Apr 20, 2024

bradleyoesch Apr 22, 2024

patrick91 May 22, 2024

bradleyoesch commented Apr 18, 2024

patrick91 commented Apr 18, 2024

botberry commented Apr 18, 2024 •

edited

botberry commented Apr 18, 2024 •

edited

patrick91 commented Apr 18, 2024

patrick91 commented Apr 18, 2024

bellini666 left a comment

bellini666 Apr 20, 2024

bradleyoesch Apr 22, 2024

bradleyoesch Apr 22, 2024

patrick91 May 22, 2024

patrick91 commented May 21, 2024

Implement oneOf #3429

Implement oneOf #3429

Conversation

patrick91 commented Mar 31, 2024 • edited

codecov bot commented Mar 31, 2024 • edited

Codecov Report

patrick91 commented Mar 31, 2024

codspeed-hq bot commented Mar 31, 2024 • edited

Merging #3429 will not alter performance

Summary

patrick91 commented Mar 31, 2024

patrick91 commented Mar 31, 2024

patrick91 commented Mar 31, 2024

patrick91 commented Apr 17, 2024

patrick91 commented Apr 17, 2024

patrick91 commented Apr 18, 2024

botberry commented Apr 18, 2024 • edited

Pre-release

botberry commented Apr 18, 2024

Pre-release

bradleyoesch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bellini666 Apr 20, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bradleyoesch commented Apr 18, 2024

patrick91 commented Apr 18, 2024

botberry commented Apr 18, 2024 • edited

botberry commented Apr 18, 2024 • edited

Apollo Federation Subgraph Compatibility Results

patrick91 commented Apr 18, 2024

patrick91 commented Apr 18, 2024

bellini666 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

patrick91 commented May 21, 2024

Implement `oneOf` #3429

Implement `oneOf` #3429

patrick91 commented Mar 31, 2024 •

edited

codecov bot commented Mar 31, 2024 •

edited

codspeed-hq bot commented Mar 31, 2024 •

edited

botberry commented Apr 18, 2024 •

edited

bellini666 Apr 20, 2024 •

edited

botberry commented Apr 18, 2024 •

edited

botberry commented Apr 18, 2024 •

edited