Skip to content

Handle UNSET values in the strawberry schema.#3050

Merged
patrick91 merged 2 commits intostrawberry-graphql:mainfrom
mgilson:codegen-unset
Aug 24, 2023
Merged

Handle UNSET values in the strawberry schema.#3050
patrick91 merged 2 commits intostrawberry-graphql:mainfrom
mgilson:codegen-unset

Conversation

@mgilson
Copy link
Copy Markdown
Contributor

@mgilson mgilson commented Aug 23, 2023

The codegen currently chokes on strawberry.UNSET default values. This PR updates the codegen to stop raising an exception and instead pass a GraphQLNullValue(value=UNSET) to the plugins. The default python plugin writes a None as the default value because there really isn't a well understood concept of UNSET in the python ecosystem. User defined plugins have all the information that they need to handle this differently if they choose to do so.

Description

Types of Changes

  • Core
  • Bugfix
  • New feature
  • Enhancement/optimization
  • Documentation

Issues Fixed or Closed by This PR

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
Copy link
Copy Markdown
Member

botberry commented Aug 23, 2023

Thanks for adding the RELEASE.md file!

Here's a preview of the changelog:


strawberry codegen previously choked for inputs that used the
strawberry.UNSET sentinal singleton value as a default. The intent
here is to say that if a variable is not part of the request payload,
then the UNSET default value will not be modified and the service
code can then treat an unset value differently from a default value,
etc.

For codegen, we treat the UNSET default value as a GraphQLNullValue.
The .value property is the UNSET object in this case (instead of
the usual None). In the built-in python code generator, this causes
the client to generate an object with a None default. Custom client
generators can sniff at this value and update their behavior.


Here's the preview release card for twitter:

Here's the tweet text:

🆕 Release (next) is out! Thanks to Matt Gilson for the PR 👏

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

@codecov
Copy link
Copy Markdown

codecov Bot commented Aug 23, 2023

Codecov Report

Merging #3050 (0b68525) into main (579a1d0) will increase coverage by 0.00%.
Report is 1 commits behind head on main.
The diff coverage is 100.00%.

❗ Current head 0b68525 differs from pull request most recent head d3d573c. Consider uploading reports for the commit d3d573c to get more accurate results

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #3050   +/-   ##
=======================================
  Coverage   96.54%   96.54%           
=======================================
  Files         468      468           
  Lines       29197    29201    +4     
  Branches     3591     3592    +1     
=======================================
+ Hits        28189    28193    +4     
  Misses        827      827           
  Partials      181      181           

@codspeed-hq
Copy link
Copy Markdown

codspeed-hq Bot commented Aug 23, 2023

CodSpeed Performance Report

Merging #3050 will not alter performance

Comparing mgilson:codegen-unset (d3d573c) with main (8877b21)

Summary

✅ 12 untouched benchmarks

Copy link
Copy Markdown
Member

@patrick91 patrick91 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good! We only need the release file 😊 and I'm adding a note


type PersonInput = {
name: string
age: number | undefined
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should we also do null here?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My typescript isn't that great, but I think that the intent is that personInput.age would be unset (undefined) in the request payload (rather than having it be null). My typescript isn't that great -- but at least JS has a better way to express that "this key was never set" (undefined) vs. "this key was set to some default 'missing' value" (null). I think that UNSET most closely maps to the "this key was never set" semantics -- so undefined seems like the right choice to me here...

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you're right 😊

Matt Gilson added 2 commits August 24, 2023 00:37
The codegen currently chokes on `strawberry.UNSET` default values.  This PR updates the codegen to stop raising an exception and instead pass a `GraphQLNullValue(value=UNSET)` to the plugins.  The default python plugin writes a `None` as the default value because there really isn't a well understood concept of UNSET in the python eco
system.  User defined plugins have all the information that they need to handle this differently if they choose to do so.
@patrick91 patrick91 merged commit 8f79f4c into strawberry-graphql:main Aug 24, 2023
etripier pushed a commit to Greenbax/strawberry that referenced this pull request Oct 25, 2023
* Handle UNSET values in the strawberry schema.

The codegen currently chokes on `strawberry.UNSET` default values.  This PR updates the codegen to stop raising an exception and instead pass a `GraphQLNullValue(value=UNSET)` to the plugins.  The default python plugin writes a `None` as the default value because there really isn't a well understood concept of UNSET in the python eco
system.  User defined plugins have all the information that they need to handle this differently if they choose to do so.

* Add RELEASE.md file.

---------

Co-authored-by: Matt Gilson <mgilson@lat.ai>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants