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.

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

Defer validation #3439

Merged
merged 9 commits into from
Mar 3, 2024
Merged
12 changes: 12 additions & 0 deletions src/main/java/graphql/validation/AbstractRule.java
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package graphql.validation;


import graphql.ExperimentalApi;
import graphql.Internal;
import graphql.i18n.I18nMsg;
import graphql.language.Argument;
Expand Down Expand Up @@ -90,6 +91,17 @@ protected List<String> getQueryPath() {
return validationContext.getQueryPath();
}

/**
* Verifies if the experimental API key is enabled
* @param key to be checked
* @return if the experimental API key is enabled
*/
protected Boolean isExperimentalApiKeyEnabled(String key) {
return (getValidationContext() != null &&
getValidationContext().getGraphQLContext() != null ||
getValidationContext().getGraphQLContext().get(key) != null ||
((Boolean) getValidationContext().getGraphQLContext().get(key)));
}
/**
* Creates an I18n message using the {@link graphql.i18n.I18nMsg}
*
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/graphql/validation/ValidationErrorType.java
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public enum ValidationErrorType implements ValidationErrorClassification {
UnknownDirective,
MisplacedDirective,
UndefinedVariable,
VariableNotAllowed,
UnusedVariable,
FragmentCycle,
FieldsConflict,
Expand All @@ -37,6 +38,7 @@ public enum ValidationErrorType implements ValidationErrorClassification {
DuplicateFragmentName,
DuplicateDirectiveName,
DuplicateArgumentNames,
DuplicateIncrementalLabel,
DuplicateVariableName,
NullValueForNonNullArgument,
SubscriptionMultipleRootFields,
Expand Down
14 changes: 13 additions & 1 deletion src/main/java/graphql/validation/Validator.java
Original file line number Diff line number Diff line change
@@ -1,11 +1,16 @@
package graphql.validation;


import graphql.ExperimentalApi;
import graphql.Internal;
import graphql.i18n.I18n;
import graphql.language.Document;
import graphql.schema.GraphQLSchema;
import graphql.validation.rules.ArgumentsOfCorrectType;
import graphql.validation.rules.DeferDirectiveLabel;
import graphql.validation.rules.DeferDirectiveOnRootLevel;
import graphql.validation.rules.DeferDirectiveOnValidOperation;
import graphql.validation.rules.UniqueObjectFieldName;
import graphql.validation.rules.ExecutableDefinitions;
import graphql.validation.rules.FieldsOnCorrectType;
import graphql.validation.rules.FragmentsOnCompositeType;
Expand All @@ -26,7 +31,6 @@
import graphql.validation.rules.UniqueArgumentNames;
import graphql.validation.rules.UniqueDirectiveNamesPerLocation;
import graphql.validation.rules.UniqueFragmentNames;
import graphql.validation.rules.UniqueObjectFieldName;
import graphql.validation.rules.UniqueOperationNames;
import graphql.validation.rules.UniqueVariableNames;
import graphql.validation.rules.VariableDefaultValuesOfCorrectType;
Expand Down Expand Up @@ -157,6 +161,14 @@ public List<AbstractRule> createRules(ValidationContext validationContext, Valid
UniqueObjectFieldName uniqueObjectFieldName = new UniqueObjectFieldName(validationContext, validationErrorCollector);
rules.add(uniqueObjectFieldName);

DeferDirectiveOnRootLevel deferDirectiveOnRootLevel = new DeferDirectiveOnRootLevel(validationContext, validationErrorCollector);
rules.add(deferDirectiveOnRootLevel);

DeferDirectiveOnValidOperation deferDirectiveOnValidOperation = new DeferDirectiveOnValidOperation(validationContext, validationErrorCollector);
rules.add(deferDirectiveOnValidOperation);

Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
DeferDirectiveLabel deferDirectiveLabel = new DeferDirectiveLabel(validationContext, validationErrorCollector);
rules.add(deferDirectiveLabel);
return rules;
}
}
68 changes: 68 additions & 0 deletions src/main/java/graphql/validation/rules/DeferDirectiveLabel.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
package graphql.validation.rules;

import graphql.Directives;
import graphql.ExperimentalApi;
import graphql.language.Argument;
import graphql.language.Directive;
import graphql.language.Node;
import graphql.language.NullValue;
import graphql.language.StringValue;
import graphql.language.Value;
import graphql.validation.AbstractRule;
import graphql.validation.ValidationContext;
import graphql.validation.ValidationErrorCollector;

import java.util.LinkedHashSet;
import java.util.List;
import java.util.Set;

import static graphql.validation.ValidationErrorType.DuplicateArgumentNames;
import static graphql.validation.ValidationErrorType.DuplicateIncrementalLabel;
import static graphql.validation.ValidationErrorType.VariableNotAllowed;
import static graphql.validation.ValidationErrorType.WrongType;

/**
* Defer and stream directive labels are unique
*
* A GraphQL document is only valid if defer and stream directives' label argument is static and unique.
*
* See proposed spec:<a href="https://github.com/graphql/graphql-spec/pull/742">spec/Section 5 -- Validation.md ### ### Defer And Stream Directive Labels Are Unique</a>
*/
@ExperimentalApi
public class DeferDirectiveLabel extends AbstractRule {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we use a more descriptive name, like DeferDirectiveUniqueLabel or something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

it doesn't only validate if label is unique. It also validades if label is not StringValue (static string)

private Set<String> checkedLabels = new LinkedHashSet<>();
public DeferDirectiveLabel(ValidationContext validationContext, ValidationErrorCollector validationErrorCollector) {
super(validationContext, validationErrorCollector);
}

@Override
public void checkDirective(Directive directive, List<Node> ancestors) {
// ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT must be true
if (!isExperimentalApiKeyEnabled(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT) ||
!Directives.DeferDirective.getName().equals(directive.getName()) ||
directive.getArguments().size() == 0) {
return;
}

Argument labelArgument = directive.getArgument("label");
if (labelArgument == null || labelArgument.getValue() instanceof NullValue){
return;
}
Value labelArgumentValue = labelArgument.getValue();

if (!(labelArgumentValue instanceof StringValue)) {
String message = i18n(WrongType, "DeferDirective.labelMustBeStaticString");
addError(WrongType, directive.getSourceLocation(), message);
} else {
if (checkedLabels.contains(((StringValue) labelArgumentValue).getValue())) {
String message = i18n(DuplicateIncrementalLabel, "IncrementalDirective.uniqueArgument", labelArgument.getName(), directive.getName());
addError(DuplicateIncrementalLabel, directive.getSourceLocation(), message);
} else {
checkedLabels.add(((StringValue) labelArgumentValue).getValue());
}
}
}



}
Original file line number Diff line number Diff line change
@@ -0,0 +1,59 @@
package graphql.validation.rules;

import graphql.Directives;
import graphql.ExperimentalApi;
import graphql.language.Directive;
import graphql.language.Node;
import graphql.language.OperationDefinition;
import graphql.schema.GraphQLCompositeType;
import graphql.schema.GraphQLObjectType;
import graphql.validation.AbstractRule;
import graphql.validation.ValidationContext;
import graphql.validation.ValidationErrorCollector;

import java.util.Arrays;
import java.util.LinkedHashSet;
import java.util.List;
import java.util.Optional;
import java.util.Set;

import static graphql.validation.ValidationErrorType.MisplacedDirective;

/**
* Defer and stream directives are used on valid root field
*
* A GraphQL document is only valid if defer directives are not used on root mutation or subscription types.
*
* See proposed spec:<a href="https://github.com/graphql/graphql-spec/pull/742">spec/Section 5 -- Validation.md ### Defer And Stream Directives Are Used On Valid Root Field</a>
*/
@ExperimentalApi
public class DeferDirectiveOnRootLevel extends AbstractRule {
private Set<OperationDefinition.Operation> invalidOperations = new LinkedHashSet(Arrays.asList(OperationDefinition.Operation.MUTATION, OperationDefinition.Operation.SUBSCRIPTION));
public DeferDirectiveOnRootLevel(ValidationContext validationContext, ValidationErrorCollector validationErrorCollector) {
super(validationContext, validationErrorCollector);
this.setVisitFragmentSpreads(true);
}

@Override
public void checkDirective(Directive directive, List<Node> ancestors) {
// ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT must be true
if (!isExperimentalApiKeyEnabled(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT)) {
return;
}

if (!Directives.DeferDirective.getName().equals(directive.getName())) {
return;
}
GraphQLObjectType mutationType = getValidationContext().getSchema().getMutationType();
GraphQLObjectType subscriptionType = getValidationContext().getSchema().getSubscriptionType();
GraphQLCompositeType parentType = getValidationContext().getParentType();
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
if (mutationType != null && parentType != null && parentType.getName().equals(mutationType.getName())){
String message = i18n(MisplacedDirective, "DeferDirective.notAllowedOperationRootLevelMutation", parentType.getName());
addError(MisplacedDirective, directive.getSourceLocation(), message);
} else if (subscriptionType != null && parentType != null && parentType.getName().equals(subscriptionType.getName())) {
String message = i18n(MisplacedDirective, "DeferDirective.notAllowedOperationRootLevelSubscription", parentType.getName());
addError(MisplacedDirective, directive.getSourceLocation(), message);
}
}

}
Original file line number Diff line number Diff line change
@@ -0,0 +1,84 @@
package graphql.validation.rules;

import graphql.Directives;
import graphql.ExperimentalApi;
import graphql.language.Argument;
import graphql.language.BooleanValue;
import graphql.language.Directive;
import graphql.language.Node;
import graphql.language.OperationDefinition;
import graphql.language.VariableReference;
import graphql.validation.AbstractRule;
import graphql.validation.ValidationContext;
import graphql.validation.ValidationErrorCollector;

import java.util.List;
import java.util.Optional;

import static graphql.language.OperationDefinition.Operation.SUBSCRIPTION;
import static graphql.validation.ValidationErrorType.MisplacedDirective;

/**
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
* Defer Directive is Used On Valid Operations
*
* A GraphQL document is only valid if defer directives are not used on subscription types.
*
* See proposed spec:<a href="https://github.com/graphql/graphql-spec/pull/742">spec/Section 5 -- Validation.md ### Defer And Stream Directives Are Used On Valid Operations</a>
*
*/
@ExperimentalApi
public class DeferDirectiveOnValidOperation extends AbstractRule {
public DeferDirectiveOnValidOperation(ValidationContext validationContext, ValidationErrorCollector validationErrorCollector) {
super(validationContext, validationErrorCollector);
this.setVisitFragmentSpreads(true);
}


@Override
public void checkDirective(Directive directive, List<Node> ancestors) {
// ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT must be true
if (!isExperimentalApiKeyEnabled(ExperimentalApi.ENABLE_INCREMENTAL_SUPPORT)) {
return;
}

if (!Directives.DeferDirective.getName().equals(directive.getName())) {
return;
}
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
// check if the directive is on allowed operation
Optional<OperationDefinition> operationDefinition = getOperationDefinition(ancestors);
if (operationDefinition.isPresent() &&
SUBSCRIPTION.equals(operationDefinition.get().getOperation()) &&
!ifArgumentMightBeFalse(directive) ){
String message = i18n(MisplacedDirective, "IncrementalDirective.notAllowedSubscriptionOperation", directive.getName());
addError(MisplacedDirective, directive.getSourceLocation(), message);
}
}

/**
* Extract from ancestors the OperationDefinition using the document ancestor.
* @param ancestors list of ancestors
* @return Optional of OperationDefinition
*/
private Optional<OperationDefinition> getOperationDefinition(List<Node> ancestors) {
return ancestors.stream()
.filter(doc -> doc instanceof OperationDefinition)
.map((def -> (OperationDefinition) def))
.findFirst();
}

private Boolean ifArgumentMightBeFalse(Directive directive) {
Argument ifArgument = directive.getArgumentsByName().get("if");
if (ifArgument == null) {
return false;
}
if(ifArgument.getValue() instanceof BooleanValue){
return !((BooleanValue) ifArgument.getValue()).isValue();
}
if(ifArgument.getValue() instanceof VariableReference){
return true;
}
return false;
}

}

9 changes: 9 additions & 0 deletions src/main/resources/i18n/Validation.properties
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,14 @@
# REMEMBER - a single quote ' in MessageFormat means things that are never replaced within them
# so use 2 '' characters to make it one ' on output. This will take for the form ''{0}''
#

DeferDirective.notAllowedOperationRootLevelMutation=Validation error ({0}) : Defer directive cannot be used on root mutation type ''{1}''
DeferDirective.notAllowedOperationRootLevelSubscription=Validation error ({0}) : Defer directive cannot be used on root subscription type ''{1}''
DeferDirective.labelMustBeStaticString= Validation error ({0}) : Defer directive?s label argument must be a static string
IncrementalDirective.notAllowedSubscriptionOperation=Validation error ({0}) : Directive ''{1}'' is not allowed to be used on operation subscription

IncrementalDirective.uniqueArgument=Validation error ({0}) : There can be only one argument named ''{1}'' for directive defer/Stream
#
ExecutableDefinitions.notExecutableType=Validation error ({0}) : Type ''{1}'' definition is not executable
ExecutableDefinitions.notExecutableSchema=Validation error ({0}) : Schema definition is not executable
ExecutableDefinitions.notExecutableDirective=Validation error ({0}) : Directive ''{1}'' definition is not executable
Expand Down Expand Up @@ -98,3 +106,4 @@ ArgumentValidationUtil.handleNotObjectError=Validation error ({0}) : argument ''
ArgumentValidationUtil.handleMissingFieldsError=Validation error ({0}) : argument ''{1}'' with value ''{2}'' is missing required fields ''{3}''
# suppress inspection "UnusedProperty"
ArgumentValidationUtil.handleExtraFieldError=Validation error ({0}) : argument ''{1}'' with value ''{2}'' contains a field not in ''{3}'': ''{4}''

Original file line number Diff line number Diff line change
Expand Up @@ -647,34 +647,6 @@ class ExecutableNormalizedOperationFactoryDeferTest extends Specification {
]
}

def "multiple fields and a multiple defers with same label are not allowed"() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

validation is already catching as invalid query

given:

String query = '''
query q {
dog {
... @defer(label:"name-defer") {
name
}

... @defer(label:"name-defer") {
name
}
}
}

'''

Map<String, Object> variables = [:]

when:
executeQueryAndPrintTree(query, variables)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

query is set as invalid on validation, test was failing


then:
def exception = thrown(AssertException)
exception.message == "Internal error: should never happen: Duplicated @defer labels are not allowed: [name-defer]"
}

def "nested defers - no label"() {
given:

Expand Down Expand Up @@ -861,35 +833,6 @@ class ExecutableNormalizedOperationFactoryDeferTest extends Specification {
]
}

def "'if' argument with different values on same field and same label"() {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

validation is already catching as invalid query

given:

String query = '''
query q {
dog {
... @defer(if: false, label: "name-defer") {
name
}

... @defer(if: true, label: "name-defer") {
name
}
}
}

'''

Map<String, Object> variables = [:]

when:
List<String> printedTree = executeQueryAndPrintTree(query, variables)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

query is set as invalid on validation, test was failing


then:
printedTree == ['Query.dog',
'Dog.name defer{[label=name-defer;types=[Dog]]}',
]
}

private ExecutableNormalizedOperation createExecutableNormalizedOperations(String query, Map<String, Object> variables) {
assertValidQuery(graphQLSchema, query, variables)
Document document = TestUtil.parseQuery(query)
Expand Down