Support "void" functions #1648

paulo-raca · 2022-02-16T03:35:25Z

Description

It might not be a best practice, but sometimes a mutation resolver just doesn't need to return anything.

Having a resolver that returns None is currently an error in strawberry.

This PR adds the scalar "Void" and uses it automatically when the type is None (or NoneType, which can be confusing 🙄)

This scalar mimics the Void type provided by GraphQL Scalars

Example:

    @strawberry.type
    class Mutation:
        @strawberry.field
        def do_something(self, arg: int) -> None:
            return

results in this schema:

   type Mutation {
       doSomething(arg: Int!): Void
    }

Changes:

Added a new Void scalar, and registered it to handle None and NoneType
Allow None type annotations
Merge from_optional and from_non_optional into from_maybe_optional
Add a special case for NoneType, to avoid wrapping it into GraphQLNonNull

Types of Changes

Checklist

My code follows the code style of this project.
My change requires a change to the documentation.
I have updated the documentation accordingly.
I have read the CONTRIBUTING document.
I have added tests to cover my changes.
I have tested the changes and verified that they work and don't break anything (as well as I can manage).

botberry · 2022-02-16T03:39:20Z

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:

Support "void" functions

It is now possible to have a resolver that returns "None". Strawberry will automatically assign the new Void scalar in the schema
and will always send null in the response

Exampe

@strawberry.type
class Mutation:
    @strawberry.field
    def do_something(self, arg: int) -> None:
        return

results in this schema:

type Mutation {
    doSomething(arg: Int!): Void
}

Here's the tweet text:

🆕 Release (next) is out! Thanks to Paulo Costa for the PR 👏

Get it here 👉 https://github.com/strawberry-graphql/strawberry/releases/tag/(next)

It might not conform to GraphQL's best practices, but sometimes we just don't need to return anything. This PR adds the scalar "Void" and uses it automatically when the type is `None` (or `NoneType`, which can be confusing) It mimics the `Void` type provided by [GraphQL Scalars](https://www.graphql-scalars.dev/docs/scalars/void)

patrick91

Uhm, I'm not sure we should add a void type, I personally never needed to do something like that (mostly because I use union types for returning errors).

Maybe it would be better do add some docs about this pattern? Have you seen something like this in other libraries?

/cc @jkimbo what do you think?

patrick91 · 2022-02-16T04:43:43Z

strawberry/schema/schema_converter.py

+    def from_maybe_optional(
+        self, type_: Union[StrawberryType, type]
+    ) -> Union[GraphQLNullableType, GraphQLNonNull]:
+        NoneType = type(None)
+        if type_ is None or type_ is NoneType:
+            return self.from_type(type_)
+        elif isinstance(type_, StrawberryOptional):
+            return self.from_type(type_.of_type)
+        else:
+            return GraphQLNonNull(self.from_type(type_))


I like this! /cc @BryceBeagle

codecov · 2022-02-16T09:56:59Z

Codecov Report

Merging #1648 (b3e9986) into main (f25ac4b) will decrease coverage by 0.02%.
The diff coverage is 93.33%.

@@            Coverage Diff             @@
##             main    #1648      +/-   ##
==========================================
- Coverage   98.16%   98.14%   -0.03%     
==========================================
  Files         129      129              
  Lines        4532     4528       -4     
  Branches      781      779       -2     
==========================================
- Hits         4449     4444       -5     
  Misses         43       43              
- Partials       40       41       +1

ThirVondukr · 2022-02-16T10:18:11Z

@patrick91 I think associating entities would be a valid use case for Void type?

type Tag {
  id: String!
  slug: String!
}

type Post {
  id: String!
  tags: [Tag!]!
}

mutation tagPost(tagId: String!, postId: String!): Void

marcoacierno · 2022-02-16T14:53:05Z

@patrick91 I think associating entities would be a valid use case for Void type?

type Tag {
  id: String!
  slug: String!
}

type Post {
  id: String!
  tags: [Tag!]!
}

mutation tagPost(tagId: String!, postId: String!): Void

I think in that case you might want the mutation to return the list of updated tags so that who uses your API (e.g Website) can have the updated list of tags after the mutation without having to re-fetch (e.g update the UI of the site)

paulo-raca · 2022-02-16T15:23:18Z

Uhm, I'm not sure we should add a void type, I personally never needed to do something like that (mostly because I use union types for returning errors).

Yes, there is a consensus that this is not the best practice 😅

However I believe strawberry should still be able to handle it anyway

Maybe it would be better do add some docs about this pattern?

I can add a line about that in the "Mutations" section

Have you seen something like this in other libraries?

TBH I'm quite new to GraphQL and I haven't seen much

However GraphQL Scalars has this and it is used in many github projects

patrick91 · 2022-02-16T15:51:06Z

Uhm, I'm not sure we should add a void type, I personally never needed to do something like that (mostly because I use union types for returning errors).

Yes, there is a consensus that this is not the best practice 😅

However I believe strawberry should still be able to handle it anyway

Maybe it would be better do add some docs about this pattern?

I can add a line about that in the "Mutations" section

Have you seen something like this in other libraries?

TBH I'm quite new to GraphQL and I haven't seen much

However GraphQL Scalars has this and it is used in many github projects

Gotcha, maybe we should add support for it, but not bind it to None 🤔

If more people ask for it to be bound to None we could have a configuration option.

What do you think?

jkimbo · 2022-02-16T15:59:33Z

IMO this seems like a good default. You can override the behaviour of None with the scalar overrides anyway if someone doesn't like it.

…documentation

github-actions · 2022-02-17T00:27:03Z

Hi 👋 You can find a preview of the docs here:

https://strawberry.rocks/docs/pr/1648/types/scalars
https://strawberry.rocks/docs/pr/1648/general/mutations

patrick91

Looks pretty good to me! Thanks for the PR and thanks to everyone for sharing their opinions 😊

I've left a couple of minor comments, then this is ready to be merged :)

RELEASE.md

docs/general/mutations.md

patrick91 · 2022-02-17T00:37:51Z

strawberry/schema/types/base_scalars.py

+    name="Void",
+    serialize=_verify_void,
+    parse_value=_verify_void,
+    description="Represents NULL values",


Suggested change

description="Represents NULL values",

description="Represents None values",

maybe it is better to use None here? or Void? since it will be used in the schema :)

I think NULL makes sense as it will be the returned value on the GraphQL protocol, while None is a Python thing.

However I don't have strong feeling about that 🤷‍♂️

For whatever it is worth, I have defined this description to be the consistent with graphql-scalar's Void

Co-authored-by: Patrick Arminio <[email protected]>

paulo-raca · 2022-02-16T19:37:52Z

strawberry/schema/schema_converter.py

-        else:
-            argument_type = self.from_non_optional(argument.type)
-
+        argument_type = cast(GraphQLInputType, self.from_maybe_optional(argument.type))


I don't know why, but the cast() was necessary in a few places to make mypy happy 🤷‍♂️

paulo-raca · 2022-02-17T01:22:45Z

strawberry/schema/types/base_scalars.py

+    name="Void",
+    serialize=_verify_void,
+    parse_value=_verify_void,
+    description="Represents NULL values",


I think NULL makes sense as it will be the returned value on the GraphQL protocol, while None is a Python thing.

However I don't have strong feeling about that 🤷‍♂️

For whatever it is worth, I have defined this description to be the consistent with graphql-scalar's Void

paulo-raca · 2022-02-17T01:28:56Z

docs/general/mutations.md

+
+<Note>
+
+Mutations with void-result go against [GQL best practices](https://graphql-rules.com/rules/mutation-payload)


🤔 The link is broken in the preview documentation.

Any idea what is wrong? Is there a different syntax inside <Note>?

it needs the spaces, now it looks ok! https://strawberry.rocks/docs/pr/1648/general/mutations

patrick91 · 2022-02-17T02:03:02Z

RELEASE.md

+```grapqhl
+type Mutation {
+    doSomething(arg: Int!): Void
+}


I think this file got messed up because of the suggestions I made 😊 can you fix it? I'm unable to push to your branch :)

Whoops, good catch!

botberry · 2022-02-17T02:43:35Z

Thanks for contributing to Strawberry! 🎉 You've been invited to join
the Strawberry GraphQL organisation 😊

You can also request a free sticker by filling this form: https://forms.gle/dmnfQUPoY5gZbVT67

And don't forget to join our discord server: https://strawberry.rocks/discord 🔥

paulo-raca · 2022-02-17T02:50:44Z

Thanks! ❤️

paulo-raca force-pushed the void branch from 9d39a17 to 0dad31e Compare February 16, 2022 03:43

botberry added bot:has-release-file labels Feb 16, 2022

Update scalars.md

3399916

patrick91 reviewed Feb 16, 2022

View reviewed changes

Add documentation on void functions in the Mutation section of the …

eac3770

…documentation

paulo-raca force-pushed the void branch from 0d443a4 to eac3770 Compare February 16, 2022 19:36

patrick91 added the ok-to-preview label Feb 17, 2022

patrick91 reviewed Feb 17, 2022

View reviewed changes

paulo-raca and others added 3 commits February 16, 2022 22:16

Update docs/general/mutations.md

cd1f3c1

Co-authored-by: Patrick Arminio <[email protected]>

Update RELEASE.md

01014a6

Co-authored-by: Patrick Arminio <[email protected]>

Update RELEASE.md

1286a7b

Co-authored-by: Patrick Arminio <[email protected]>

paulo-raca commented Feb 17, 2022

View reviewed changes

patrick91 reviewed Feb 17, 2022

View reviewed changes

Re-added triple-quotes

b3e9986

patrick91 approved these changes Feb 17, 2022

View reviewed changes

patrick91 merged commit f5a2afd into strawberry-graphql:main Feb 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support "void" functions #1648

Support "void" functions #1648

paulo-raca commented Feb 16, 2022 •

edited

Loading

botberry commented Feb 16, 2022 •

edited

Loading

patrick91 left a comment

patrick91 Feb 16, 2022

codecov bot commented Feb 16, 2022 •

edited

Loading

ThirVondukr commented Feb 16, 2022

marcoacierno commented Feb 16, 2022 •

edited

Loading

paulo-raca commented Feb 16, 2022

patrick91 commented Feb 16, 2022

jkimbo commented Feb 16, 2022

github-actions bot commented Feb 17, 2022

patrick91 left a comment

patrick91 Feb 17, 2022

paulo-raca Feb 17, 2022 •

edited

Loading

paulo-raca Feb 16, 2022

paulo-raca Feb 17, 2022 •

edited

Loading

paulo-raca Feb 17, 2022

patrick91 Feb 17, 2022

paulo-raca Feb 17, 2022

patrick91 Feb 17, 2022

paulo-raca Feb 17, 2022 •

edited

Loading

botberry commented Feb 17, 2022

paulo-raca commented Feb 17, 2022

	description="Represents NULL values",
	description="Represents None values",


		<Note>

		Mutations with void-result go against [GQL best practices](https://graphql-rules.com/rules/mutation-payload)

Support "void" functions #1648

Support "void" functions #1648

Conversation

paulo-raca commented Feb 16, 2022 • edited Loading

Description

Example:

Changes:

Types of Changes

Checklist

botberry commented Feb 16, 2022 • edited Loading

Exampe

patrick91 left a comment

Choose a reason for hiding this comment

patrick91 Feb 16, 2022

Choose a reason for hiding this comment

codecov bot commented Feb 16, 2022 • edited Loading

Codecov Report

ThirVondukr commented Feb 16, 2022

marcoacierno commented Feb 16, 2022 • edited Loading

paulo-raca commented Feb 16, 2022

patrick91 commented Feb 16, 2022

jkimbo commented Feb 16, 2022

github-actions bot commented Feb 17, 2022

patrick91 left a comment

Choose a reason for hiding this comment

patrick91 Feb 17, 2022

Choose a reason for hiding this comment

paulo-raca Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

paulo-raca Feb 16, 2022

Choose a reason for hiding this comment

paulo-raca Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

paulo-raca Feb 17, 2022

Choose a reason for hiding this comment

patrick91 Feb 17, 2022

Choose a reason for hiding this comment

paulo-raca Feb 17, 2022

Choose a reason for hiding this comment

patrick91 Feb 17, 2022

Choose a reason for hiding this comment

paulo-raca Feb 17, 2022 • edited Loading

Choose a reason for hiding this comment

botberry commented Feb 17, 2022

paulo-raca commented Feb 17, 2022

paulo-raca commented Feb 16, 2022 •

edited

Loading

botberry commented Feb 16, 2022 •

edited

Loading

codecov bot commented Feb 16, 2022 •

edited

Loading

marcoacierno commented Feb 16, 2022 •

edited

Loading

paulo-raca Feb 17, 2022 •

edited

Loading

paulo-raca Feb 17, 2022 •

edited

Loading

paulo-raca Feb 17, 2022 •

edited

Loading