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

validate non-nullable directive args #3551

Merged
merged 3 commits into from Apr 16, 2024
Merged

validate non-nullable directive args #3551

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
d843a30
validate non-nullable directive args
jbellenger Mar 31, 2024
0dac307
remove wildcard import
jbellenger Apr 1, 2024
b906880
fix merge conflict
jbellenger Apr 2, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
46 changes: 38 additions & 8 deletions src/main/java/graphql/schema/validation/AppliedDirectiveArgumentsAreValid.java
View file
Open in desktop
Expand Up @@ -6,11 +6,14 @@
import graphql.execution.ValuesResolver;
import graphql.language.Value;
import graphql.schema.CoercingParseValueException;
import graphql.schema.GraphQLAppliedDirective;
import graphql.schema.GraphQLAppliedDirectiveArgument;
import graphql.schema.GraphQLArgument;
import graphql.schema.GraphQLDirective;
import graphql.schema.GraphQLInputType;
import graphql.schema.GraphQLSchema;
import graphql.schema.GraphQLSchemaElement;
import graphql.schema.GraphQLTypeUtil;
import graphql.schema.GraphQLTypeVisitorStub;
import graphql.schema.InputValueWithState;
import graphql.util.TraversalControl;
Expand All @@ -32,29 +35,56 @@ public TraversalControl visitGraphQLDirective(GraphQLDirective directive, Traver
// if there is no parent it means it is just a directive definition and not an applied directive
if (context.getParentNode() != null) {
for (GraphQLArgument graphQLArgument : directive.getArguments()) {
checkArgument(directive, graphQLArgument, context);
checkArgument(
directive.getName(),
graphQLArgument.getName(),
graphQLArgument.getArgumentValue(),
graphQLArgument.getType(),
context
);
}
}
return TraversalControl.CONTINUE;
}

private void checkArgument(GraphQLDirective directive, GraphQLArgument argument, TraverserContext<GraphQLSchemaElement> context) {
if (!argument.hasSetValue()) {
return;
@Override
public TraversalControl visitGraphQLAppliedDirective(GraphQLAppliedDirective directive, TraverserContext<GraphQLSchemaElement> context) {
// if there is no parent it means it is just a directive definition and not an applied directive
if (context.getParentNode() != null) {
for (GraphQLAppliedDirectiveArgument graphQLArgument : directive.getArguments()) {
checkArgument(
directive.getName(),
graphQLArgument.getName(),
graphQLArgument.getArgumentValue(),
graphQLArgument.getType(),
context
);
}
}
return TraversalControl.CONTINUE;
}

private void checkArgument(
String directiveName,
String argumentName,
InputValueWithState argumentValue,
GraphQLInputType argumentType,
TraverserContext<GraphQLSchemaElement> context
) {
GraphQLSchema schema = context.getVarFromParents(GraphQLSchema.class);
SchemaValidationErrorCollector errorCollector = context.getVarFromParents(SchemaValidationErrorCollector.class);
InputValueWithState argumentValue = argument.getArgumentValue();
boolean invalid = false;
if (argumentValue.isLiteral() &&
!validationUtil.isValidLiteralValue((Value<?>) argumentValue.getValue(), argument.getType(), schema, GraphQLContext.getDefault(), Locale.getDefault())) {
!validationUtil.isValidLiteralValue((Value<?>) argumentValue.getValue(), argumentType, schema, GraphQLContext.getDefault(), Locale.getDefault())) {
invalid = true;
} else if (argumentValue.isExternal() &&
!isValidExternalValue(schema, argumentValue.getValue(), argument.getType(), GraphQLContext.getDefault(), Locale.getDefault())) {
!isValidExternalValue(schema, argumentValue.getValue(), argumentType, GraphQLContext.getDefault(), Locale.getDefault())) {
invalid = true;
} else if (argumentValue.isNotSet() && GraphQLTypeUtil.isNonNull(argumentType)) {
Copy link
Member

Choose a reason for hiding this comment

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

I suspect we need to do some more work here on how default values get put into place at runtime. Because if its not null but it has a default than thats ok right.

Copy link
Member

Choose a reason for hiding this comment

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

Actually look at the code - we make an assumption that the default value is transferred into the GraphQLArgument / GraphQLAppliedDirectiveArgument at creation time and hence it should be validated if its a non null type

invalid = true;
}
if (invalid) {
String message = format("Invalid argument '%s' for applied directive of name '%s'", argument.getName(), directive.getName());
String message = format("Invalid argument '%s' for applied directive of name '%s'", argumentName, directiveName);
errorCollector.addError(new SchemaValidationError(SchemaValidationErrorType.InvalidAppliedDirectiveArgument, message));
}
}
Expand Down
52 changes: 41 additions & 11 deletions src/test/groovy/graphql/schema/GraphQLArgumentTest.groovy
View file
Open in desktop
Expand Up @@ -195,23 +195,53 @@ class GraphQLArgumentTest extends Specification {
resolvedDefaultValue == null
}

def "Applied schema directives arguments are validated for programmatic schemas"() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Not certain, but I think "Applied" in the test case name meant something different from a GraphQLAppliedDirective, which this test case doesn't use.

Copy link
Member

Choose a reason for hiding this comment

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

Yeah the reason this is confusing is explained on the main PR comments. Nominally a type should have BOTH a GraphQLAppliedDirective for every GraphQLDirective - but tests allow this not to happen - or more properly programatic building of schemas allow it (since it could also happen in production code if you build your schema directly and not via graphql.schema.idl.SchemaGenerator).

Copy link
Member

Choose a reason for hiding this comment

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

See graphql.schema.idl.SchemaGeneratorAppliedDirectiveHelper

def "schema directive arguments are validated for programmatic schemas"() {
given:
def arg = newArgument().name("arg").type(GraphQLInt).valueProgrammatic(ImmutableKit.emptyMap()).build() // Retain for test coverage
def directive = GraphQLDirective.newDirective().name("cached").argument(arg).build()
def directive = newDirective().name("cached").argument(arg).build()
def field = newFieldDefinition()
.name("hello")
.type(GraphQLString)
.argument(arg)
.withDirective(directive)
.build()
.name("hello")
.type(GraphQLString)
.argument(arg)
.withDirective(directive)
.build()
when:
newSchema().query(
newSchema()
.query(
newObject()
.name("Query")
.field(field)
.build())
.name("Query")
.field(field)
.build()
)
.additionalDirective(directive)
.build()
then:
def e = thrown(InvalidSchemaException)
e.message.contains("Invalid argument 'arg' for applied directive of name 'cached'")
}

def "applied directive arguments are validated for programmatic schemas"() {
given:
def arg = newArgument()
.name("arg")
.type(GraphQLNonNull.nonNull(GraphQLInt))
.build()
def directive = newDirective().name("cached").argument(arg).build()
def field = newFieldDefinition()
.name("hello")
.type(GraphQLString)
.withAppliedDirective(directive.toAppliedDirective())
.build()
when:
newSchema()
.query(
newObject()
.name("Query")
.field(field)
.build()
)
.additionalDirective(directive)
.build()
then:
def e = thrown(InvalidSchemaException)
e.message.contains("Invalid argument 'arg' for applied directive of name 'cached'")
Expand Down