Make deprecated reason non-null #3759
Conversation
Test Results 306 files 306 suites 47s ⏱️ Results for commit a95cc02. ♻️ This comment has been updated with latest results. |
| }); | ||
| changeNode(context, newElement); | ||
| return TraversalControl.ABORT; | ||
| } |
There was a problem hiding this comment.
I am guessing the Anonymizer pre-dates the split of GraphQLDirective and GraphQLAppliedDirective
Now that applied directives are separated, there shouldn't be any transformation in this GraphQLDirective visitor method. The deprecated directive as defined in the schema should be unchanged, as any other built-in directive in the next lines
if (DirectiveInfo.isGraphqlSpecifiedDirective(graphQLDirective.getName())) {
return TraversalControl.ABORT;
}
| @@ -285,7 +274,8 @@ public TraversalControl visitGraphQLAppliedDirective(GraphQLAppliedDirective gra | |||
| if (Directives.DEPRECATED_DIRECTIVE_DEFINITION.getName().equals(graphQLDirective.getName())) { | |||
There was a problem hiding this comment.
As I mentioned above, the censoring of @deprecated reasons should actually only be here, in the GraphQLAppliedDirective visitor method only
| @@ -285,7 +274,8 @@ public TraversalControl visitGraphQLAppliedDirective(GraphQLAppliedDirective gra | |||
| if (Directives.DEPRECATED_DIRECTIVE_DEFINITION.getName().equals(graphQLDirective.getName())) { | |||
| GraphQLAppliedDirectiveArgument reason = GraphQLAppliedDirectiveArgument.newArgument().name("reason") | |||
| .type(Scalars.GraphQLString) | |||
There was a problem hiding this comment.
It's ok for an applied directive's argument to be nullable. In this case, the default value ("No longer supported") as defined in the schema will be used
| @@ -968,7 +968,7 @@ type Query { | |||
| "Marks the field, argument, input field or enum value as deprecated" | |||
| directive @deprecated( | |||
| "The reason for the deprecation" | |||
| reason: String = "No longer supported" | |||
There was a problem hiding this comment.
All that follows is a number of tests which need updating
| @@ -63,7 +63,7 @@ public class Directives { | |||
| newInputValueDefinition() | |||
| .name("reason") | |||
| .description(createDescription("The reason for the deprecation")) | |||
| .type(newTypeName().name(STRING).build()) | |||
| .type(newNonNullType(newTypeName().name(STRING).build()).build()) | |||
|
This proposed change has been included in the draft specification today: https://spec.graphql.org/draft/#sec--deprecated |
Closes #3736
An incoming spec change will require the
@deprecatedreason to be non-nullable. graphql/graphql-spec#1040.Whilst this is technically a breaking change, the reason already has a default value, so it is only a breaking change if the reason
nullwas previously used.Update: when I started the PR it was at RFC1 stage, at the time of merging the proposal reached RFC3 (Accepted) stage and is already in the draft spec document: https://spec.graphql.org/draft/#sec--deprecated