Skip to content

Bugfix: spec-compliant behaviour for non-nullable types with default values. #2573

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
bbakerman merged 2 commits into from
Dec 27, 2021
Merged

Bugfix: spec-compliant behaviour for non-nullable types with default values. #2573

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
4528bb2
Bugfix: don't fail document validation when nullable type is provided…
Oct 5, 2021
e5d0db5
Fix failing tests.
Oct 5, 2021
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
14 changes: 13 additions & 1 deletion src/main/java/graphql/validation/rules/VariableTypesMatchRule.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,19 +3,23 @@

import graphql.Internal;
import graphql.execution.TypeFromAST;
import graphql.execution.ValuesResolver;
import graphql.language.OperationDefinition;
import graphql.language.Value;
import graphql.language.VariableDefinition;
import graphql.language.VariableReference;
import graphql.schema.GraphQLInputType;
import graphql.schema.GraphQLType;
import graphql.schema.GraphQLTypeUtil;
import graphql.schema.InputValueWithState;
import graphql.validation.AbstractRule;
import graphql.validation.ValidationContext;
import graphql.validation.ValidationErrorCollector;
import graphql.validation.ValidationErrorType;

import java.util.LinkedHashMap;
import java.util.Map;
import java.util.Optional;

@Internal
public class VariableTypesMatchRule extends AbstractRule {
Expand Down Expand Up @@ -55,11 +59,19 @@ public void checkVariable(VariableReference variableReference) {
return;
}
GraphQLInputType expectedType = getValidationContext().getInputType();
Optional<InputValueWithState> schemaDefault = Optional.ofNullable(getValidationContext().getArgument()).map(v -> v.getArgumentDefaultValue());
Value schemaDefaultValue = null;
if (schemaDefault.isPresent() && schemaDefault.get().isLiteral()) {
schemaDefaultValue = (Value) schemaDefault.get().getValue();
} else if (schemaDefault.isPresent() && schemaDefault.get().isSet()) {
schemaDefaultValue = ValuesResolver.valueToLiteral(schemaDefault.get(), expectedType);
}
Copy link
Member

@bbakerman bbakerman Oct 9, 2021

Choose a reason for hiding this comment

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

@andimarek - can you please double check that the argument get value calls here are correct in the new regime for reading args values.

Copy link
Member

@andimarek andimarek Dec 20, 2021

Choose a reason for hiding this comment

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

You can simplify the code here: there is a method in ValuesResolver ValuesResolver.valueToLiteral which takes care of converting it fo you.

Copy link
Contributor Author

@folone folone Dec 22, 2021

Choose a reason for hiding this comment

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

Hey @andimarek! I believe that's the exact method I'm using on line 67 there. Am I missing something else?

if (expectedType == null) {
// we must have a unknown variable say to not have a known type
return;
}
if (!variablesTypesMatcher.doesVariableTypesMatch(variableType, variableDefinition.getDefaultValue(), expectedType)) {
if (!variablesTypesMatcher.doesVariableTypesMatch(variableType, variableDefinition.getDefaultValue(), expectedType) &&
!variablesTypesMatcher.doesVariableTypesMatch(variableType, schemaDefaultValue, expectedType)) {
GraphQLType effectiveType = variablesTypesMatcher.effectiveType(variableType, variableDefinition.getDefaultValue());
String message = String.format("Variable type '%s' doesn't match expected type '%s'",
GraphQLTypeUtil.simplePrint(effectiveType),
Expand Down
20 changes: 20 additions & 0 deletions src/test/groovy/graphql/GraphQLTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1129,6 +1129,26 @@ many lines''']

}

def "default value defined in the schema is used when none provided in the query"() {
// Spec (https://spec.graphql.org/June2018/#sec-All-Variable-Usages-are-Allowed): A notable exception to typical variable type compatibility is allowing a variable definition with a nullable type to be provided to a non‐null location as long as either that variable or that location provides a default value.
given:
def spec = """type Query {
sayHello(name: String! = "amigo"): String
}"""
def df = { dfe ->
return dfe.getArgument("name")
} as DataFetcher
def graphQL = TestUtil.graphQL(spec, ["Query": ["sayHello": df]]).build()

when:
def result = graphQL.execute('query($var:String){sayHello(name:$var)}');

then:
result.errors.isEmpty()
result.getData() == [sayHello: "amigo"]

}

def "specified url can be defined and queried via introspection"() {
given:
GraphQLSchema schema = TestUtil.schema('type Query {foo: MyScalar} scalar MyScalar @specifiedBy(url:"myUrl")');
Expand Down
5 changes: 5 additions & 0 deletions src/test/groovy/graphql/validation/rules/VariableTypesMatchRuleTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ import graphql.language.StringValue
import graphql.language.TypeName
import graphql.language.VariableDefinition
import graphql.language.VariableReference
import graphql.schema.GraphQLArgument
import graphql.schema.GraphQLList
import graphql.schema.GraphQLNonNull
import graphql.validation.ValidationContext
Expand All @@ -33,9 +34,11 @@ class VariableTypesMatchRuleTest extends Specification {
def defaultValue = new StringValue("default")
def astType = new TypeName("String")
def expectedType = Scalars.GraphQLBoolean
def argument = GraphQLArgument.newArgument().name("test").type(expectedType).build();

validationContext.getSchema() >> StarWarsSchema.starWarsSchema
validationContext.getInputType() >> expectedType
validationContext.getArgument() >> argument
variablesTypeMatcher.effectiveType(Scalars.GraphQLString, defaultValue) >> Scalars.GraphQLString
variablesTypeMatcher
.doesVariableTypesMatch(Scalars.GraphQLString, defaultValue, expectedType) >> false
Expand All @@ -54,11 +57,13 @@ class VariableTypesMatchRuleTest extends Specification {
def defaultValue = null
def astType = new NonNullType(new ListType(new TypeName("String")))
def expectedType = GraphQLList.list(GraphQLNonNull.nonNull(Scalars.GraphQLString))
def argument = GraphQLArgument.newArgument().name("test").type(expectedType).build();

def mismatchedType = GraphQLNonNull.nonNull(GraphQLList.list(Scalars.GraphQLString))

validationContext.getSchema() >> StarWarsSchema.starWarsSchema
validationContext.getInputType() >> expectedType
validationContext.getArgument() >> argument
variablesTypeMatcher.effectiveType({ mismatchedType.isEqualTo(it) }, defaultValue) >> mismatchedType
variablesTypeMatcher
.doesVariableTypesMatch(expectedType, defaultValue, expectedType) >> false
Expand Down