Skip to content

Bug 3276 - default values not being used correctly during validation #3278

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 6 commits into from
Aug 22, 2023
Merged

Bug 3276 - default values not being used correctly during validation #3278

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
6cdf4a8
Bug 3276 - reproduction of reported bug
bbakerman Aug 1, 2023
3b09851
Bug 3276 - fix for reported bug
bbakerman Aug 1, 2023
5e4e682
Bug 3276 - more repros on the tests
bbakerman Aug 2, 2023
9c99711
issue 3276 - more work on fixing this bug
bbakerman Aug 5, 2023
9df21f1
Tweaked the errors messages
bbakerman Aug 6, 2023
07e8a84
auf Deutsch (courtesy of machine translation)
dondonz Aug 16, 2023
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
52 changes: 39 additions & 13 deletions src/main/java/graphql/validation/TraversalContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@
import graphql.schema.GraphQLType;
import graphql.schema.GraphQLUnionType;
import graphql.schema.GraphQLUnmodifiedType;
import graphql.schema.InputValueWithState;

import java.util.ArrayList;
import java.util.List;
Expand All @@ -47,6 +48,7 @@ public class TraversalContext implements DocumentVisitor {
private final List<GraphQLOutputType> outputTypeStack = new ArrayList<>();
private final List<GraphQLCompositeType> parentTypeStack = new ArrayList<>();
private final List<GraphQLInputType> inputTypeStack = new ArrayList<>();
private final List<InputValueWithState> defaultValueStack = new ArrayList<>();
Copy link
Member Author

@bbakerman bbakerman Aug 5, 2023

Choose a reason for hiding this comment

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

inspiration taken from graphql-js - keep a stack of default values as we traverse the AST

the older existing code was doing exactly this

private final List<GraphQLFieldDefinition> fieldDefStack = new ArrayList<>();
private final List<String> nameStack = new ArrayList<>();
private GraphQLDirective directive;
Expand Down Expand Up @@ -155,6 +157,7 @@ private void enterImpl(Argument argument) {
}

addInputType(argumentType != null ? argumentType.getType() : null);
addDefaultValue(argumentType != null ? argumentType.getArgumentDefaultValue() : null);
this.argument = argumentType;
}

Expand All @@ -165,23 +168,30 @@ private void enterImpl(ArrayValue arrayValue) {
inputType = (GraphQLInputType) unwrapOne(nullableType);
}
addInputType(inputType);
// List positions never have a default value. See graphql-js impl for inspiration
addDefaultValue(null);
Copy link
Member Author

@bbakerman bbakerman Aug 5, 2023

Choose a reason for hiding this comment

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

I did this because graphql-js did this

}

private void enterImpl(ObjectField objectField) {
GraphQLUnmodifiedType objectType = unwrapAll(getInputType());
GraphQLInputType inputType = null;
GraphQLInputObjectField inputField = null;
if (objectType instanceof GraphQLInputObjectType) {
GraphQLInputObjectType inputObjectType = (GraphQLInputObjectType) objectType;
GraphQLInputObjectField inputField = schema.getFieldVisibility().getFieldDefinition(inputObjectType, objectField.getName());
if (inputField != null)
inputField = schema.getFieldVisibility().getFieldDefinition(inputObjectType, objectField.getName());
if (inputField != null) {
inputType = inputField.getType();
}
}
addInputType(inputType);
addDefaultValue(inputField != null ? inputField.getInputFieldDefaultValue() : null);
}

private GraphQLArgument find(List<GraphQLArgument> arguments, String name) {
for (GraphQLArgument argument : arguments) {
if (argument.getName().equals(name)) return argument;
if (argument.getName().equals(name)) {
return argument;
}
}
Copy link
Member Author

@bbakerman bbakerman Aug 5, 2023

Choose a reason for hiding this comment

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

auto format puts in braces (as it should)

return null;
}
Expand All @@ -190,29 +200,32 @@ private GraphQLArgument find(List<GraphQLArgument> arguments, String name) {
@Override
public void leave(Node node, List<Node> ancestors) {
if (node instanceof OperationDefinition) {
outputTypeStack.remove(outputTypeStack.size() - 1);
pop(outputTypeStack);
} else if (node instanceof SelectionSet) {
parentTypeStack.remove(parentTypeStack.size() - 1);
pop(parentTypeStack);
} else if (node instanceof Field) {
leaveName(((Field) node).getName());
fieldDefStack.remove(fieldDefStack.size() - 1);
outputTypeStack.remove(outputTypeStack.size() - 1);
pop(fieldDefStack);
pop(outputTypeStack);
} else if (node instanceof Directive) {
directive = null;
} else if (node instanceof InlineFragment) {
outputTypeStack.remove(outputTypeStack.size() - 1);
pop(outputTypeStack);
} else if (node instanceof FragmentDefinition) {
leaveName(((FragmentDefinition) node).getName());
outputTypeStack.remove(outputTypeStack.size() - 1);
pop(outputTypeStack);
} else if (node instanceof VariableDefinition) {
inputTypeStack.remove(inputTypeStack.size() - 1);
Copy link

@lindaandersson lindaandersson Aug 13, 2023

Choose a reason for hiding this comment

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

Suggested change
inputTypeStack.remove(inputTypeStack.size() - 1);
pop(inputTypeStack);

I just assmed this was a refactoring miss.
Should it also pop the defaultValueStack as the other cases bellow?

Copy link
Member Author

@bbakerman bbakerman Aug 22, 2023

Choose a reason for hiding this comment

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

yes changed

} else if (node instanceof Argument) {
argument = null;
inputTypeStack.remove(inputTypeStack.size() - 1);
pop(inputTypeStack);
pop(defaultValueStack);
} else if (node instanceof ArrayValue) {
inputTypeStack.remove(inputTypeStack.size() - 1);
pop(inputTypeStack);
pop(defaultValueStack);
} else if (node instanceof ObjectField) {
inputTypeStack.remove(inputTypeStack.size() - 1);
pop(inputTypeStack);
pop(defaultValueStack);
}
}

Expand Down Expand Up @@ -249,10 +262,16 @@ private void addOutputType(GraphQLOutputType type) {
}

private <T> T lastElement(List<T> list) {
if (list.size() == 0) return null;
if (list.isEmpty()) {
return null;
}
return list.get(list.size() - 1);
}

private <T> T pop(List<T> list) {
return list.remove(list.size() - 1);
}
Copy link
Member Author

@bbakerman bbakerman Aug 5, 2023

Choose a reason for hiding this comment

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

neater


/**
* @return can be null if the parent is not a CompositeType
*/
Expand All @@ -267,11 +286,18 @@ private void addParentType(GraphQLCompositeType compositeType) {
public GraphQLInputType getInputType() {
return lastElement(inputTypeStack);
}
public InputValueWithState getDefaultValue() {
return lastElement(defaultValueStack);
}

private void addInputType(GraphQLInputType graphQLInputType) {
inputTypeStack.add(graphQLInputType);
}

private void addDefaultValue(InputValueWithState defaultValue) {
defaultValueStack.add(defaultValue);
}

public GraphQLFieldDefinition getFieldDef() {
return lastElement(fieldDefStack);
}
Expand Down
11 changes: 9 additions & 2 deletions src/main/java/graphql/validation/ValidationContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import graphql.schema.GraphQLInputType;
import graphql.schema.GraphQLOutputType;
import graphql.schema.GraphQLSchema;
import graphql.schema.InputValueWithState;

import java.util.LinkedHashMap;
import java.util.List;
Expand Down Expand Up @@ -41,8 +42,10 @@ public ValidationContext(GraphQLSchema schema, Document document, I18n i18n) {
}

private void buildFragmentMap() {
for (Definition definition : document.getDefinitions()) {
if (!(definition instanceof FragmentDefinition)) continue;
for (Definition<?> definition : document.getDefinitions()) {
if (!(definition instanceof FragmentDefinition)) {
continue;
}
Copy link
Member Author

@bbakerman bbakerman Aug 5, 2023

Choose a reason for hiding this comment

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

auto format

FragmentDefinition fragmentDefinition = (FragmentDefinition) definition;
fragmentDefinitionMap.put(fragmentDefinition.getName(), fragmentDefinition);
}
Expand Down Expand Up @@ -72,6 +75,10 @@ public GraphQLInputType getInputType() {
return traversalContext.getInputType();
}

public InputValueWithState getDefaultValue() {
return traversalContext.getDefaultValue();
}

public GraphQLFieldDefinition getFieldDef() {
return traversalContext.getFieldDef();
}
Expand Down
27 changes: 14 additions & 13 deletions src/main/java/graphql/validation/rules/VariableTypesMatch.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -59,24 +59,25 @@ public void checkVariable(VariableReference variableReference) {
if (variableType == null) {
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, getValidationContext().getGraphQLContext(), getValidationContext().getI18n().getLocale());
}
if (expectedType == null) {
// we must have a unknown variable say to not have a known type
GraphQLInputType locationType = getValidationContext().getInputType();
Optional<InputValueWithState> locationDefault = Optional.ofNullable(getValidationContext().getDefaultValue());
if (locationType == null) {
// we must have an unknown variable say to not have a known type
return;
}
if (!variablesTypesMatcher.doesVariableTypesMatch(variableType, variableDefinition.getDefaultValue(), expectedType) &&
!variablesTypesMatcher.doesVariableTypesMatch(variableType, schemaDefaultValue, expectedType)) {
Value<?> locationDefaultValue = null;
if (locationDefault.isPresent() && locationDefault.get().isLiteral()) {
locationDefaultValue = (Value<?>) locationDefault.get().getValue();
} else if (locationDefault.isPresent() && locationDefault.get().isSet()) {
locationDefaultValue = ValuesResolver.valueToLiteral(locationDefault.get(), locationType, getValidationContext().getGraphQLContext(), getValidationContext().getI18n().getLocale());
}
boolean variableDefMatches = variablesTypesMatcher.doesVariableTypesMatch(variableType, variableDefinition.getDefaultValue(), locationType, locationDefaultValue);
if (!variableDefMatches) {
GraphQLType effectiveType = variablesTypesMatcher.effectiveType(variableType, variableDefinition.getDefaultValue());
String message = i18n(VariableTypeMismatch, "VariableTypesMatchRule.unexpectedType",
variableDefinition.getName(),
GraphQLTypeUtil.simplePrint(effectiveType),
GraphQLTypeUtil.simplePrint(expectedType));
GraphQLTypeUtil.simplePrint(locationType));
addError(VariableTypeMismatch, variableReference.getSourceLocation(), message);
}
}
Copy link
Member Author

@bbakerman bbakerman Aug 5, 2023

Choose a reason for hiding this comment

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

The twitter folks dropped of a fix like

 if (!variablesTypesMatcher.doesVariableTypesMatch(variableType, variableDefinition.getDefaultValue(), expectedType) &&
                !variablesTypesMatcher.doesVariableTypesMatch(variableType, schemaDefaultValue, expectedType)) {

but this was kinda wrong. Its not the argument default value but the location default value

I went back to the reference implementation for inspiration and this is MUCH more like that it does

Expand Down
28 changes: 25 additions & 3 deletions src/main/java/graphql/validation/rules/VariablesTypesMatcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,16 +9,38 @@
import static graphql.schema.GraphQLNonNull.nonNull;
import static graphql.schema.GraphQLTypeUtil.isList;
import static graphql.schema.GraphQLTypeUtil.isNonNull;
import static graphql.schema.GraphQLTypeUtil.unwrapNonNull;
import static graphql.schema.GraphQLTypeUtil.unwrapOne;

@Internal
public class VariablesTypesMatcher {

public boolean doesVariableTypesMatch(GraphQLType variableType, Value variableDefaultValue, GraphQLType expectedType) {
return checkType(effectiveType(variableType, variableDefaultValue), expectedType);
/**
* This method and variable naming was inspired from the reference graphql-js implementation
*
* @param varType the variable type
* @param varDefaultValue the default value for the variable
* @param locationType the location type where the variable was encountered
* @param locationDefaultValue the default value for that location
*
* @return true if the variable matches ok
*/
public boolean doesVariableTypesMatch(GraphQLType varType, Value<?> varDefaultValue, GraphQLType locationType, Value<?> locationDefaultValue) {
if (isNonNull(locationType) && !isNonNull(varType)) {
boolean hasNonNullVariableDefaultValue =
varDefaultValue != null && !(varDefaultValue instanceof NullValue);
boolean hasLocationDefaultValue = locationDefaultValue != null;
if (!hasNonNullVariableDefaultValue && !hasLocationDefaultValue) {
return false;
}
GraphQLType nullableLocationType = unwrapNonNull(locationType);
return checkType(varType, nullableLocationType);
}
return checkType(varType, locationType);
Copy link
Member Author

@bbakerman bbakerman Aug 5, 2023

Choose a reason for hiding this comment

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

if you looking in the graphql-js reference implementation - the logic is almost exactly the same

}

public GraphQLType effectiveType(GraphQLType variableType, Value defaultValue) {

public GraphQLType effectiveType(GraphQLType variableType, Value<?> defaultValue) {
if (defaultValue == null || defaultValue instanceof NullValue) {
return variableType;
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/resources/i18n/Validation.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ VariableDefaultValuesOfCorrectType.badDefault=Validation error ({0}) : Bad defau
#
VariablesAreInputTypes.wrongType=Validation error ({0}) : Input variable ''{1}'' type ''{2}'' is not an input type
#
VariableTypesMatchRule.unexpectedType=Validation error ({0}) : Variable type ''{1}'' does not match expected type ''{2}''
VariableTypesMatchRule.unexpectedType=Validation error ({0}) : Variable ''{1}'' of type ''{2}'' used in position expecting type ''{3}''
#
UniqueObjectFieldName.duplicateFieldName=Validation Error ({0}) : There can be only one field named ''{1}''
#
Expand Down
2 changes: 1 addition & 1 deletion src/main/resources/i18n/Validation_de.properties
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ VariableDefaultValuesOfCorrectType.badDefault=Validierungsfehler ({0}) : Ung
#
VariablesAreInputTypes.wrongType=Validierungsfehler ({0}) : Eingabevariable ''{1}'' Typ ''{2}'' ist kein Eingabetyp
#
VariableTypesMatchRule.unexpectedType=Validierungsfehler ({0}) : Der Variablentyp ''{1}'' stimmt nicht mit dem erwarteten Typ ''{2}'' �berein
Copy link
Member

@dondonz dondonz Aug 16, 2023

Choose a reason for hiding this comment

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

FYI: GitHub's diff viewer can't handle non-ASCII characters. It was a lower case u with umlaut

VariableTypesMatchRule.unexpectedType=Validierungsfehler ({0}) : Variable ''{1}'' vom Typ ''{2}'' verwendet in Position, die Typ ''{3}'' erwartet
#
# These are used but IDEA cant find them easily as being called
#
Expand Down
Loading