Skip to content
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

Update GraphQL mutation compiler to consider arguments with replace_null_with_default #1622

Merged
merged 1 commit into from
Aug 28, 2023

Conversation

dom-binti
Copy link
Contributor

Motivation

With graphql-ruby arguments, you can set required: false, default_value: 'foo', replace_null_with_default: true to fill a null sent by a client with the default value (docs). The current graphql mutation compiler doesn't take this into account, and assumes that all required: false arguments are nilable, when in reality a non-nil default_value plus replace_null_with_default means the resolver will always receive a non-nil value.

Implementation

I updated GraphqlTypeHelper.type_for to take in a GraphQL::Schema::Argument, rather than just the argument type, and check for the other Argument params when deciding if the type is nilable.

Tests

Added a test with both a nil and a non-nil default_value to the existing graphql mutation compiler tests.

@dom-binti
Copy link
Contributor Author

I have signed the CLA!

@dom-binti dom-binti marked this pull request as ready for review August 24, 2023 18:15
@dom-binti dom-binti requested a review from a team as a code owner August 24, 2023 18:15
@dom-binti dom-binti changed the title handle replace_null_with_default arguments Update GraphQL mutation compiler to consider arguments with replace_null_with_default Aug 24, 2023
Copy link
Contributor

@KaanOzkan KaanOzkan left a comment

Choose a reason for hiding this comment

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

LGTM but I'd like to test it with our monolith quickly before shipping when the CI passes.

@dom-binti
Copy link
Contributor Author

@KaanOzkan sgtm. Looks like CI is passing now. Let me know if there's anything else to do on my end!

@KaanOzkan KaanOzkan added the enhancement New feature or request label Aug 28, 2023
@KaanOzkan
Copy link
Contributor

Looks like our monolith doesn't utilize replace_null_with_default. Existing RBIs are unchanged. Thanks for the contribution.

@KaanOzkan KaanOzkan merged commit 6bc9b08 into Shopify:main Aug 28, 2023
15 checks passed
@shopify-shipit shopify-shipit bot temporarily deployed to production September 13, 2023 22:55 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants