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

nullable openapi properties converted to non-nullable gql fields #359

Open
nlundquist opened this issue Feb 17, 2021 · 5 comments · May be fixed by #360
Open

nullable openapi properties converted to non-nullable gql fields #359

nlundquist opened this issue Feb 17, 2021 · 5 comments · May be fixed by #360

Comments

@nlundquist
Copy link

nlundquist commented Feb 17, 2021 •
edited
Loading

OpenAPI properties may be nullable & required. required only indicates that a key must be present in an object to be valid, it makes no restriction on the type of the value corresponding to that key. openapi-to-graphql assumes that required OpenAPI properties are non-nullable:

openapi-to-graphql/packages/openapi-to-graphql/src/schema_builder.ts

Lines 622 to 623 in a06d6f4

const requiredProperty =
typeof def.required === 'object' && def.required.includes(fieldTypeKey)

To Reproduce
Use openapi-to-graphql on a spec with type with a required field that is also nullable. Queries for that type which return a null for that nullable field cause unexpected validation errors.

Expected behavior
No validation errors when returning nulls for explicitly nullable fields.
@Alan-Cha
Copy link
Collaborator

Very interesting! I will review the spec but thank you so much for reporting this issue! This is an important distinction.

@nlundquist nlundquist linked a pull request Feb 18, 2021 that will close this issue
Open
@nlundquist
Copy link
Author

nlundquist commented Feb 18, 2021 •
edited
Loading

Thinking about this a bit more I don't think that required has any relevance to a GraphQL API. Certainly not during querying as it's entirely up to the querier to specify what subset of fields they wish to fetch.

To be useful during mutation, GQL would need to be able to validate the presence of a field independently of it's type. GQL can't do that though- every non-nullable field of an input type is implicitly also required. If openapi-to-graphql tried to use required to imply non-nullability on input types, it would break with any API that has a required nullable field on an input type.

For openapi-to-graphql to validate required precisely it would need to run a JSONSchema validator against the input types since the GQL validator doesn't have the flexibility required. Probably easier in the short term to just document this incompatibility between the specifications.

@Alan-Cha
Copy link
Collaborator

After some more careful review, I believe you are correct! This bug originally stems from a misunderstanding of what required means. We should be checking for nullable instead, so thank you for the clarification.

@Beny406 Beny406 mentioned this issue Aug 12, 2021
Closed
@nicolasparada
Copy link

Any progress on this?
This prevent us from using nullable fields.

@nicolasparada
Copy link

nicolasparada commented Feb 22, 2023 •
edited
Loading

Setting default: null does the trick.
Does not fix this issue, but its a workaround.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging a pull request may close this issue.

use nullable OpenAPI property to determine nullability nlundquist/openapi-to-graphql
3 participants
@nlundquist @nicolasparada @Alan-Cha