Skip to content

Make deprecated reason non-null #3759

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

Merged
dondonz merged 5 commits into from
Dec 5, 2024
Merged

Make deprecated reason non-null #3759

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
4b4935f
Make deprecated reason non-null
dondonz Dec 1, 2024
b3c99aa
Update schema printer tests
dondonz Dec 1, 2024
017f8ac
Update schema generator test
dondonz Dec 1, 2024
0a1559b
Update anonymizer
dondonz Dec 1, 2024
a95cc02
Tidy up
dondonz Dec 1, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/main/java/graphql/Directives.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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())
Copy link
Member

@bbakerman bbakerman Dec 5, 2024

Choose a reason for hiding this comment

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

what is newNonNullType ???

Copy link
Member

@bbakerman bbakerman Dec 5, 2024

Choose a reason for hiding this comment

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

ahh a builder

.defaultValue(StringValue.newStringValue().value(NO_LONGER_SUPPORTED).build())
.build())
.build();
Expand Down Expand Up @@ -197,7 +197,7 @@ public class Directives {
.description("Marks the field, argument, input field or enum value as deprecated")
.argument(newArgument()
.name("reason")
.type(GraphQLString)
.type(nonNull(GraphQLString))
.defaultValueProgrammatic(NO_LONGER_SUPPORTED)
.description("The reason for the deprecation"))
.validLocations(FIELD_DEFINITION, ENUM_VALUE, ARGUMENT_DEFINITION, INPUT_FIELD_DEFINITION)
Expand Down
18 changes: 4 additions & 14 deletions src/main/java/graphql/util/Anonymizer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -60,7 +60,6 @@
import graphql.schema.GraphQLNamedOutputType;
import graphql.schema.GraphQLNamedSchemaElement;
import graphql.schema.GraphQLNamedType;
import graphql.schema.GraphQLNonNull;
import graphql.schema.GraphQLObjectType;
import graphql.schema.GraphQLScalarType;
import graphql.schema.GraphQLSchema;
Expand Down Expand Up @@ -93,7 +92,7 @@

import static graphql.Assert.assertNotNull;
import static graphql.parser.ParserEnvironment.newParserEnvironment;
import static graphql.schema.GraphQLArgument.newArgument;
import static graphql.schema.GraphQLNonNull.nonNull;
import static graphql.schema.GraphQLTypeUtil.unwrapNonNull;
import static graphql.schema.GraphQLTypeUtil.unwrapNonNullAs;
import static graphql.schema.GraphQLTypeUtil.unwrapOneAs;
Expand Down Expand Up @@ -260,16 +259,6 @@ public TraversalControl visitGraphQLFieldDefinition(GraphQLFieldDefinition graph

@Override
public TraversalControl visitGraphQLDirective(GraphQLDirective graphQLDirective, TraverserContext<GraphQLSchemaElement> context) {
if (Directives.DEPRECATED_DIRECTIVE_DEFINITION.getName().equals(graphQLDirective.getName())) {
GraphQLArgument reason = newArgument().name("reason")
.type(Scalars.GraphQLString)
.clearValue().build();
GraphQLDirective newElement = graphQLDirective.transform(builder -> {
builder.description(null).argument(reason);
});
changeNode(context, newElement);
return TraversalControl.ABORT;
}
Copy link
Member Author

@dondonz dondonz Dec 1, 2024

Choose a reason for hiding this comment

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

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;
}

if (DirectiveInfo.isGraphqlSpecifiedDirective(graphQLDirective.getName())) {
return TraversalControl.ABORT;
}
Expand All @@ -285,7 +274,8 @@ public TraversalControl visitGraphQLAppliedDirective(GraphQLAppliedDirective gra
if (Directives.DEPRECATED_DIRECTIVE_DEFINITION.getName().equals(graphQLDirective.getName())) {
Copy link
Member Author

@dondonz dondonz Dec 1, 2024

Choose a reason for hiding this comment

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

As I mentioned above, the censoring of @deprecated reasons should actually only be here, in the GraphQLAppliedDirective visitor method only

GraphQLAppliedDirectiveArgument reason = GraphQLAppliedDirectiveArgument.newArgument().name("reason")
.type(Scalars.GraphQLString)
Copy link
Member Author

@dondonz dondonz Dec 1, 2024

Choose a reason for hiding this comment

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

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

.clearValue().build();
.clearValue()
.build();
GraphQLAppliedDirective newElement = graphQLDirective.transform(builder -> {
builder.description(null).argument(reason);
});
Expand Down Expand Up @@ -918,7 +908,7 @@ private static GraphQLType fromTypeToGraphQLType(Type type, GraphQLSchema schema
graphql.Assert.assertNotNull(graphQLType, "Schema must contain type %s", typeName);
return graphQLType;
} else if (type instanceof NonNullType) {
return GraphQLNonNull.nonNull(fromTypeToGraphQLType(TypeUtil.unwrapOne(type), schema));
return nonNull(fromTypeToGraphQLType(TypeUtil.unwrapOne(type), schema));
} else if (type instanceof ListType) {
return GraphQLList.list(fromTypeToGraphQLType(TypeUtil.unwrapOne(type), schema));
} else {
Expand Down
2 changes: 1 addition & 1 deletion src/test/groovy/graphql/Issue2141.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -27,7 +27,7 @@ class Issue2141 extends Specification {
"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

"Directs the executor to include this field or fragment only when the `if` argument is true"
Expand Down
6 changes: 4 additions & 2 deletions src/test/groovy/graphql/schema/idl/SchemaGeneratorTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1770,7 +1770,8 @@ class SchemaGeneratorTest extends Specification {

def appliedDirective = f1.getAppliedDirective("deprecated")
appliedDirective.name == "deprecated"
appliedDirective.getArgument("reason").type == GraphQLString
appliedDirective.getArgument("reason").type instanceof GraphQLNonNull
(appliedDirective.getArgument("reason").type as GraphQLNonNull).wrappedType == GraphQLString
printAst(appliedDirective.getArgument("reason").argumentValue.value as Node) == '"No longer supported"'

when:
Expand All @@ -1781,7 +1782,8 @@ class SchemaGeneratorTest extends Specification {

def appliedDirective2 = f2.getAppliedDirective("deprecated")
appliedDirective2.name == "deprecated"
appliedDirective2.getArgument("reason").type == GraphQLString
appliedDirective2.getArgument("reason").type instanceof GraphQLNonNull
(appliedDirective2.getArgument("reason").type as GraphQLNonNull).wrappedType == GraphQLString
printAst(appliedDirective2.getArgument("reason").argumentValue.value as Node) == '"Just because"'
def directive2 = f2.getDirective("deprecated")
printAst(directive2.getArgument("reason").argumentDefaultValue.value as Node) == '"No longer supported"'
Expand Down
26 changes: 13 additions & 13 deletions src/test/groovy/graphql/schema/idl/SchemaPrinterTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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"
Copy link
Member Author

@dondonz dondonz Dec 1, 2024

Choose a reason for hiding this comment

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

All that follows is a number of tests which need updating

reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

directive @enumTypeDirective on ENUM
Expand Down Expand Up @@ -1144,7 +1144,7 @@ input SomeInput {
result == '''"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

"Directs the executor to include this field or fragment only when the `if` argument is true"
Expand Down Expand Up @@ -1240,7 +1240,7 @@ type Query {
resultWithDirectives == '''"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

directive @example on FIELD_DEFINITION
Expand Down Expand Up @@ -1308,7 +1308,7 @@ type Query {
resultWithDirectiveDefinitions == '''"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

directive @example on FIELD_DEFINITION
Expand Down Expand Up @@ -1405,7 +1405,7 @@ extend schema {
"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

"Directs the executor to include this field or fragment only when the `if` argument is true"
Expand Down Expand Up @@ -1491,7 +1491,7 @@ schema @schemaDirective{
"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

"Directs the executor to include this field or fragment only when the `if` argument is true"
Expand Down Expand Up @@ -1631,7 +1631,7 @@ type MyQuery {
result == '''"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

directive @directive1 on SCALAR
Expand Down Expand Up @@ -1873,7 +1873,7 @@ type Query {
result == '''"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

type Query {
Expand Down Expand Up @@ -2167,7 +2167,7 @@ type PrintMeType {
"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

directive @foo on SCHEMA
Expand Down Expand Up @@ -2399,7 +2399,7 @@ directive @include(
"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

union ZUnion = XQuery | Query
Expand Down Expand Up @@ -2515,7 +2515,7 @@ schema {
"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

" custom directive 'example' description 1"
Expand Down Expand Up @@ -2752,7 +2752,7 @@ schema {
"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

" custom directive 'example' description 1"
Expand Down Expand Up @@ -2940,7 +2940,7 @@ input Input {
result == """"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String = "No longer supported"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

"Directs the executor to include this field or fragment only when the `if` argument is true"
Expand Down
8 changes: 6 additions & 2 deletions src/test/groovy/graphql/util/AnonymizerTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -729,8 +729,12 @@ type Object1 {

directive @Directive1(argument1: String! = "stringValue4") repeatable on SCHEMA | SCALAR | OBJECT | FIELD_DEFINITION | ARGUMENT_DEFINITION | INTERFACE | UNION | ENUM | ENUM_VALUE | INPUT_OBJECT | INPUT_FIELD_DEFINITION

directive @deprecated(reason: String) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

interface Interface1 @Directive1(argument1 : "stringValue12") {
field2: String
field3: Enum1
Expand Down
8 changes: 6 additions & 2 deletions src/test/resources/large-schema-1.graphqls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,11 @@ directive @skip(
if: Boolean!
) on FIELD | FRAGMENT_SPREAD | INLINE_FRAGMENT

directive @deprecated(reason: String) on FIELD_DEFINITION | ENUM_VALUE
"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

"Exposes a URL that specifies the behaviour of this scalar."
directive @specifiedBy(
Expand Down Expand Up @@ -544,4 +548,4 @@ input InputObject9 {
inputField32: [ID!]
inputField33: [Scalar2]
inputField34: InputObject8
}
}
6 changes: 5 additions & 1 deletion src/test/resources/many-fragments.graphqls
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,11 @@ directive @Directive8(argument7: Enum3!) on FIELD_DEFINITION

directive @Directive9(argument8: String!) on OBJECT

directive @deprecated(reason: String) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION
"Marks the field, argument, input field or enum value as deprecated"
directive @deprecated(
"The reason for the deprecation"
reason: String! = "No longer supported"
) on FIELD_DEFINITION | ARGUMENT_DEFINITION | ENUM_VALUE | INPUT_FIELD_DEFINITION

"Directs the executor to include this field or fragment only when the `if` argument is true"
directive @include(
Expand Down