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.

Sign up for GitHub

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

Jump to bottom

First round of improvements around non-nullability #110

Merged
merged 1 commit into from
Jan 31, 2024
Merged

First round of improvements around non-nullability #110

merged 1 commit into from
Jan 31, 2024

Conversation

rbino
Copy link
Contributor

@rbino rbino commented Jan 31, 2024 •
edited
Loading

First round of implementation of what was discussed in #105

This PR makes result types non-nullable.

@rbino
improvement: make mutation results non-nullable
67dd52d 
Adjust rool level error tests: since result is now non-nullable, if the content
of the result is nil, the whole result becomes nil. Note that this is not a
breaking change since GraphQL always allows a field nullability to propagate
upwards in case of a field error (see October 2021 spec, section 6.4.4).
@zachdaniel
Copy link
Contributor

For the ids in updates being non nullable, that would be a breaking change for any queries people have, right? I think we'll have to put that behind a configuration for now, and undo it when we do 1.0 for this library.

@rbino
Copy link
Contributor Author

rbino commented Jan 31, 2024

Wouldn't the same thing that was discussed in #105 regarding making the input non-nullable apply (see below)?

It would be a breaking change to require it, even if we only did it in cases where there was at least one required input inside of it, because nullable -> not nullable on an input type is theoretically breaking. Although in those scenarios the action would fail 100% of the time... 🤷 so maybe fine to just make both changes.

Anyway, adding a config works for me too, but which kind of config (i.e. at what level) should it be?

@zachdaniel
Copy link
Contributor

Yeah, but the changes you made in the test made me realize that it's not enough that it wouldn't have worked, because it will break people's type declarations. i.e if they have id: ID GraphQL will yell at them because its not an optional value anymore. We can use application config, there should be some other examples in this repo. I'll cut a 1.0 around the time we do 3.0 of Ash (1-2 months)

@rbino rbino force-pushed the improve-non-nullability branch from 8932ba2 to 67dd52d Compare January 31, 2024 20:56
@rbino
Copy link
Contributor Author

rbino commented Jan 31, 2024

@zachdaniel I've left only the non-breaking result change in this PR and I'll do a separate one with IDs and input objects behind an application config

@zachdaniel zachdaniel merged commit af4193f into ash-project:main Jan 31, 2024
12 checks passed
rbino added a commit to rbino/ash_graphql that referenced this pull request Jan 31, 2024
@rbino
improvement: make mutation arguments non-null
1236da9 
As discussed in ash-project#105 and ash-project#110, put this behind an opt-in configuration to avoid
breaking existing code.
The ID in update mutations is always non-null if non-null mutation arguments are
allowed, while input is non-null if it's allowed _and_ there is at least a
non-null field in the input.

Document the newly added config variable in the getting started guide.
@rbino rbino mentioned this pull request Jan 31, 2024
Merged
1 task
zachdaniel pushed a commit that referenced this pull request Jan 31, 2024
@rbino
improvement: make mutation arguments non-null (#111)
8fff0d3 
* improvement: make mutation arguments non-null

As discussed in #105 and #110, put this behind an opt-in configuration to avoid
breaking existing code.
The ID in update mutations is always non-null if non-null mutation arguments are
allowed, while input is non-null if it's allowed _and_ there is at least a
non-null field in the input.

Document the newly added config variable in the getting started guide.

* chore: enable non-null mutation arguments in tests
rbino added a commit to rbino/ash_graphql that referenced this pull request Feb 9, 2024
@rbino
fix: make results nullable again if root level errors are enabled
c9d9e48 
Building upon ash-project#110, this restores the old behaviour of the result being nullable
when root level errors are present.

While the result is guaranteed to not be nullable in standard conditions (since
either result or errors are always present), when errors are moved to the root
level it could become null, so declaring it non-nullable propagates the null up
to the data field.

This actually causes compatibility problems with some client libraries (e.g.
Relay) that expect the inner result to be null, _not_ data, if there's an error.

This also adds dedicated RootLevelErrors versions of the Api and the Schema
since the configuration is accessed at compile time now, so put_env was not
enough to test them correctly.
@rbino rbino mentioned this pull request Feb 9, 2024
Merged
1 task
rbino added a commit to rbino/ash_graphql that referenced this pull request Feb 9, 2024
@rbino
fix: make results nullable again if root level errors are enabled
988ba62 
Building upon ash-project#110, this restores the old behaviour of the result being nullable
when root level errors are present.

While the result is guaranteed to not be nullable in standard conditions (since
either result or errors are always present), when errors are moved to the root
level it could become null, so declaring it non-nullable propagates the null up
to the data field.

This actually causes compatibility problems with some client libraries (e.g.
Relay) that expect the inner result to be null, _not_ data, if there's an error.

This also adds dedicated RootLevelErrors versions of the Api and the Schema
since the configuration is accessed at compile time now, so put_env was not
enough to test them correctly.
zachdaniel pushed a commit that referenced this pull request Feb 9, 2024
@rbino
fix: make results nullable again if root level errors are enabled (#114)
70eae5f 
Building upon #110, this restores the old behaviour of the result being nullable
when root level errors are present.

While the result is guaranteed to not be nullable in standard conditions (since
either result or errors are always present), when errors are moved to the root
level it could become null, so declaring it non-nullable propagates the null up
to the data field.

This actually causes compatibility problems with some client libraries (e.g.
Relay) that expect the inner result to be null, _not_ data, if there's an error.

This also adds dedicated RootLevelErrors versions of the Api and the Schema
since the configuration is accessed at compile time now, so put_env was not
enough to test them correctly.
@zachdaniel zachdaniel mentioned this pull request Feb 9, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zachdaniel zachdaniel zachdaniel approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@rbino @zachdaniel