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

Add validation for @deprecated on required arguments #917

Merged
merged 1 commit into from
Jan 6, 2022

Conversation

fotoetienne
Copy link
Contributor

The @deprecated directive must not appear on required (non-null without a default)
arguments or input object field definitions. Deprecated arguments and fields are
excluded by default in introspection, and deprecating required arguments or
input fields could create confusion for clients.

The `@deprecated` directive must not appear on required (non-null without a default)
arguments or input object field definitions. Deprecated arguments and fields are
excluded by default in introspection, and deprecating required arguments or
input fields could create confusion for clients.
@linux-foundation-easycla
Copy link

CLA Not Signed

@fotoetienne
Copy link
Contributor Author

In a previous working group meeting, it was discussed that deprecating required inputs could be problematic. There was debate about whether this should be worded in the spec as SHOULD or MUST. In this PR I've added it to the spec as MUST. The reasons are two-fold:

  • The GraphQL spec recommends hiding things that are deprecated. It would make for a confusing experience to have a required field that is hidden.
  • If we change our mind, relaxing the spec from MUST to SHOULD would be a non-breaking change.

@leebyron
Copy link
Collaborator

leebyron commented Jan 6, 2022

@fotoetienne can you double check your CLA status? - merging anyway since this is PR on PR

@leebyron leebyron merged commit d5ed294 into graphql:deprecate-args Jan 6, 2022
@fotoetienne
Copy link
Contributor Author

@fotoetienne can you double check your CLA status? - merging anyway since this is PR on PR

It's fixed now. Thanks!

@glasser
Copy link
Contributor

glasser commented Jan 7, 2022

I'm not sure I understand this validation. I mean, I understand the basic idea that @deprecated on required arguments and fields is problematic.

But isn't the section this is added to a section describing how to validate a particular operation against a schema, and the condition we're concerned about here is about whether a schema is valid itself? It seems like implementing this change would mean not checking for this condition when assembling a schema but instead checking for it as part of every incoming operation that uses the field in question, which seems like an odd place to do this check.

@glasser
Copy link
Contributor

glasser commented Jan 7, 2022

Specifically, it seems like this check should go in http://spec.graphql.org/draft/#sec-Objects.Type-Validation (etc) rather than the validation section.

IvanGoncharov pushed a commit that referenced this pull request Jan 9, 2022
The `@deprecated` directive must not appear on required (non-null without a default)
arguments or input object field definitions. Deprecated arguments and fields are
excluded by default in introspection, and deprecating required arguments or
input fields could create confusion for clients.
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