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
12 changes: 12 additions & 0 deletions src/main/java/graphql/validation/Validator.java
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,9 @@
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;
Expand Down Expand Up @@ -157,6 +160,15 @@ public List<AbstractRule> createRules(ValidationContext validationContext, Valid
UniqueObjectFieldName uniqueObjectFieldName = new UniqueObjectFieldName(validationContext, validationErrorCollector);
rules.add(uniqueObjectFieldName);

DeferDirectiveLabel deferDirectiveLabel = new DeferDirectiveLabel(validationContext, validationErrorCollector);
rules.add(deferDirectiveLabel);

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
return rules;
}
}
41 changes: 41 additions & 0 deletions src/main/java/graphql/validation/rules/DeferDirectiveLabel.java
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package graphql.validation.rules;

import graphql.Internal;
import graphql.language.Argument;
import graphql.language.Directive;
import graphql.language.Node;
import graphql.language.StringValue;
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;

@Internal
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
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> labels = new LinkedHashSet<>();
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
public DeferDirectiveLabel(ValidationContext validationContext, ValidationErrorCollector validationErrorCollector) {
super(validationContext, validationErrorCollector);
}

@Override
public void checkDirective(Directive directive, List<Node> ancestors) {
if (!directive.getName().equals("defer") || directive.getArguments().size() == 0) {
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
return;
}
Argument labelArgument = directive.getArgument("label");
// argument type is validated in DeferDirectiveArgumentType
if (labelArgument != null && labelArgument.getValue() instanceof StringValue) {
if (labels.contains(((StringValue) labelArgument.getValue()).getValue())) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we check the value of if before flagging a label as duplicate?
like, should this be considered valid:

query {
  ... @defer(if: false, label: "a") {
   a
  }
  ... @defer(if: true, label: "a") {
   anotherA
  }
}

Copy link
Member

Choose a reason for hiding this comment

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

if you pull out (StringValue) labelArgument.getValue() to a variable there will be less code later

Copy link
Contributor Author

Choose a reason for hiding this comment

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

should we check the value of if before flagging a label as duplicate? like, should this be considered valid:

query {
  ... @defer(if: false, label: "a") {
   a
  }
  ... @defer(if: true, label: "a") {
   anotherA
  }
}

As far as I know and checked Label must be unique regardless if value
Screenshot 2024-02-22 at 10 46 02 pm

String message = i18n(DuplicateArgumentNames, "UniqueArgumentNames.directiveUniqueArgument", labelArgument.getName(), directive.getName());
addError(DuplicateArgumentNames, directive.getSourceLocation(), message);
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
} else {
labels.add(((StringValue) labelArgument.getValue()).getValue() );
}
}
}

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

import graphql.language.Directive;
import graphql.language.FragmentDefinition;
import graphql.language.InlineFragment;
import graphql.language.Node;
import graphql.language.OperationDefinition;
import graphql.language.SelectionSet;
import graphql.validation.AbstractRule;
import graphql.validation.ValidationContext;
import graphql.validation.ValidationErrorCollector;

import java.util.ArrayList;
import java.util.Collections;
import java.util.List;
import java.util.Optional;

import static graphql.validation.ValidationErrorType.MisplacedDirective;

public class DeferDirectiveOnRootLevel extends AbstractRule {

public DeferDirectiveOnRootLevel(ValidationContext validationContext, ValidationErrorCollector validationErrorCollector) {
super(validationContext, validationErrorCollector);
this.setVisitFragmentSpreads(true);
}

@Override
public void checkDirective(Directive directive, List<Node> ancestors) {
if (directive.getName().equals("defer")){
Optional<Node> fragmentAncestor = getFragmentAncestor(ancestors);
if (fragmentAncestor.isPresent() && fragmentAncestor.get() instanceof OperationDefinition){
OperationDefinition operationDefinition = (OperationDefinition) fragmentAncestor.get();
if (OperationDefinition.Operation.MUTATION.equals(operationDefinition.getOperation()) || OperationDefinition.Operation.SUBSCRIPTION.equals(operationDefinition.getOperation())) {
String message = i18n(MisplacedDirective, "DirectiveMisplaced.notAllowedOperationRootLevel", directive.getName(), operationDefinition.getOperation().name().toLowerCase());
addError(MisplacedDirective, directive.getSourceLocation(), message);
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
}
}

}
}

/**
* Get the first ancestor that is not InlineFragment, SelectionSet or FragmentDefinition
* @param ancestors list of ancestors
* @return Optional of Node parent that is not InlineFragment, SelectionSet or FragmentDefinition.
*/
protected Optional<Node> getFragmentAncestor(List<Node> ancestors){
List<Node> ancestorsCopy = new ArrayList(ancestors);
Collections.reverse(ancestorsCopy);
return ancestorsCopy.stream().filter(node -> !(
node instanceof InlineFragment ||
node instanceof SelectionSet ||
node instanceof FragmentDefinition
)
).findFirst();
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,56 @@
package graphql.validation.rules;

import graphql.language.BooleanValue;
import graphql.language.Directive;
import graphql.language.Document;
import graphql.language.Node;
import graphql.language.OperationDefinition;
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.
*/
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) {
if (!directive.getName().equals("defer") ||
(directive.getArgumentsByName().get("if") != null && !((BooleanValue) directive.getArgumentsByName().get("if").getValue()).isValue() )) {
return;
}
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
// check if the directive is on allowed operation
Optional<OperationDefinition> operationDefinition = getOperation(ancestors);
if (operationDefinition.isPresent() && operationDefinition.get().getOperation() == SUBSCRIPTION) {
String message = i18n(MisplacedDirective, "DirectiveMisplaced.notAllowedOperation", directive.getName(), SUBSCRIPTION.name().toLowerCase());
addError(MisplacedDirective, directive.getSourceLocation(), message);
}
}

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

}

5 changes: 5 additions & 0 deletions src/main/resources/i18n/Validation.properties
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,8 @@ SubscriptionIntrospectionRootField.introspectionRootField=Validation error ({0})
SubscriptionIntrospectionRootField.introspectionRootFieldWithFragment=Validation error ({0}) : Subscription operation ''{1}'' fragment root field ''{2}'' cannot be an introspection field
#
UniqueArgumentNames.uniqueArgument=Validation error ({0}) : There can be only one argument named ''{1}''

UniqueArgumentNames.directiveUniqueArgument=Validation error ({0}) : There can be only one argument named ''{1}'' for directive ''{2}''
#
UniqueDirectiveNamesPerLocation.uniqueDirectives=Validation error ({0}) : Non repeatable directives must be uniquely named within a location. The directive ''{1}'' used on a ''{2}'' is not unique
#
Expand Down Expand Up @@ -98,3 +100,6 @@ 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}''

DirectiveMisplaced.notAllowedOperationRootLevel=Validation error ({0}) : Directive ''{1}'' is not allowed on root of operation ''{2}''
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
DirectiveMisplaced.notAllowedOperation=Validation error ({0}) : Directive ''{1}'' is not on operation ''{2}''
Juliano-Prado marked this conversation as resolved.
Show resolved Hide resolved
24 changes: 24 additions & 0 deletions src/test/groovy/graphql/validation/SpecValidationSchema.java
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,10 @@
import static graphql.introspection.Introspection.DirectiveLocation.QUERY;
import static graphql.schema.GraphQLArgument.newArgument;
import static graphql.schema.GraphQLFieldDefinition.newFieldDefinition;
import static graphql.schema.GraphQLInputObjectField.newInputObjectField;
import static graphql.schema.GraphQLInputObjectType.newInputObject;
import static graphql.schema.GraphQLNonNull.nonNull;
import static graphql.schema.GraphqlTypeComparatorRegistry.BY_NAME_REGISTRY;
import static java.util.Collections.singletonList;

/**
Expand Down Expand Up @@ -215,6 +218,25 @@ public class SpecValidationSchema {
.field(newFieldDefinition().name("cat").type(cat))
.build();

public static GraphQLInputObjectType inputDogType = newInputObject()
.name("DogInput")
.description("Input for A Dog creation.")
.field(newInputObjectField()
.name("id")
.description("The id of the dog.")
.type(nonNull(GraphQLString)))
.build();

public static final GraphQLObjectType petMutationType = GraphQLObjectType.newObject()
.name("PetMutationType")
.field(newFieldDefinition()
.name("createDog")
.type(dog)
.argument(newArgument()
.name("input")
.type(inputDogType)))
.build();

public static final Set<GraphQLType> specValidationDictionary = new HashSet<GraphQLType>() {{
add(dogCommand);
add(catCommand);
Expand Down Expand Up @@ -275,11 +297,13 @@ public class SpecValidationSchema {
.query(queryRoot)
.codeRegistry(codeRegistry)
.subscription(subscriptionRoot)
.mutation(petMutationType)
.additionalDirective(upperDirective)
.additionalDirective(lowerDirective)
.additionalDirective(dogDirective)
.additionalDirective(nonNullDirective)
.additionalDirective(objectArgumentDirective)
.additionalDirective(Directives.DeferDirective)
.additionalTypes(specValidationDictionary)
.build();

Expand Down