Skip to content

Refactor GraphQL#execute implementation #509

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 1 commit into from
Jun 21, 2017
Merged

Refactor GraphQL#execute implementation #509

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
7 changes: 5 additions & 2 deletions src/main/java/graphql/ExecutionResultImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@


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

@Internal
Expand All @@ -10,6 +11,10 @@ public class ExecutionResultImpl implements ExecutionResult {
private final List<GraphQLError> errors = new ArrayList<>();
private Object data;

public ExecutionResultImpl(GraphQLError error) {
this(Collections.singletonList(error));
}

public ExecutionResultImpl(List<? extends GraphQLError> errors) {
this(null, errors);
}
Expand All @@ -23,8 +28,6 @@ public ExecutionResultImpl(Object data, List<? extends GraphQLError> errors) {

}



@Override
public <T> T getData() {
//noinspection unchecked
Expand Down
107 changes: 81 additions & 26 deletions src/main/java/graphql/GraphQL.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.util.Collections;
import java.util.List;
import java.util.Map;

Expand Down Expand Up @@ -280,50 +279,106 @@ public ExecutionResult execute(String query, String operationName, Object contex
* @return result including errors
*/
public ExecutionResult execute(ExecutionInput executionInput) {
String query = executionInput.getQuery();
String operationName = executionInput.getOperationName();
Object context = executionInput.getContext();
Object root = executionInput.getRoot();
Map<String, Object> variables = executionInput.getVariables() != null ? executionInput.getVariables() : Collections.emptyMap();
log.debug("Executing request. operation name: {}. query: {} ", executionInput.getOperationName(), executionInput.getQuery());

log.debug("Executing request. operation name: {}. query: {} ", operationName, query);
InstrumentationContext<ExecutionResult> executionInstrumentation = instrumentation.beginExecution(new InstrumentationExecutionParameters(executionInput));
final ExecutionResult executionResult = parseValidateAndExecute(executionInput);
executionInstrumentation.onEnd(executionResult);

return executionResult;
}

private ExecutionResult parseValidateAndExecute(ExecutionInput executionInput) {
ParseResult parseResult = parse(executionInput);
if (parseResult.isFailure()) {
return toParseFailureExecutionResult(parseResult.getException());
}
final Document document = parseResult.getDocument();

final List<ValidationError> errors = validate(executionInput, document);
if (!errors.isEmpty()) {
return new ExecutionResultImpl(errors);
}

InstrumentationContext<ExecutionResult> executionCtx = instrumentation.beginExecution(new InstrumentationExecutionParameters(query, operationName, context, variables));
return execute(executionInput, document);
}

private ParseResult parse(ExecutionInput executionInput) {
InstrumentationContext<Document> parseInstrumentation = instrumentation.beginParse(new InstrumentationExecutionParameters(executionInput));

InstrumentationContext<Document> parseCtx = instrumentation.beginParse(new InstrumentationExecutionParameters(query, operationName, context, variables));
Parser parser = new Parser();
Document document;
try {
document = parser.parseDocument(query);
parseCtx.onEnd(document);
document = parser.parseDocument(executionInput.getQuery());
} catch (ParseCancellationException e) {
RecognitionException recognitionException = (RecognitionException) e.getCause();
SourceLocation sourceLocation = null;
if (recognitionException != null) {
sourceLocation = new SourceLocation(recognitionException.getOffendingToken().getLine(), recognitionException.getOffendingToken().getCharPositionInLine());
}
InvalidSyntaxError invalidSyntaxError = new InvalidSyntaxError(sourceLocation);
return new ExecutionResultImpl(Collections.singletonList(invalidSyntaxError));
parseInstrumentation.onEnd(e);
return ParseResult.ofError((RecognitionException) e.getCause());
}

InstrumentationContext<List<ValidationError>> validationCtx = instrumentation.beginValidation(new InstrumentationValidationParameters(query, operationName, context, variables, document));
parseInstrumentation.onEnd(document);
return ParseResult.of(document);
}

private List<ValidationError> validate(ExecutionInput executionInput, Document document) {
InstrumentationContext<List<ValidationError>> validationCtx = instrumentation.beginValidation(new InstrumentationValidationParameters(executionInput, document));

Validator validator = new Validator();
List<ValidationError> validationErrors = validator.validateDocument(graphQLSchema, document);

validationCtx.onEnd(validationErrors);
return validationErrors;
}

if (validationErrors.size() > 0) {
return new ExecutionResultImpl(validationErrors);
}
ExecutionId executionId = idProvider.provide(query, operationName, context);
private ExecutionResult execute(ExecutionInput executionInput, Document document) {
String query = executionInput.getQuery();
String operationName = executionInput.getOperationName();
Object context = executionInput.getContext();

Execution execution = new Execution(queryStrategy, mutationStrategy, subscriptionStrategy, instrumentation);
ExecutionResult result = execution.execute(executionId, graphQLSchema, context, root, document, operationName, variables);
ExecutionId executionId = idProvider.provide(query, operationName, context);
return execution.execute(document, graphQLSchema, executionId, executionInput);
Copy link
Contributor Author

@tklovett tklovett Jun 19, 2017

Choose a reason for hiding this comment

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

This @Internal API change (and corresponding changes in Execution and ExecutionTest) is new in this amended commit and was not previously reviewed.

}

private ExecutionResult toParseFailureExecutionResult(RecognitionException exception) {
InvalidSyntaxError invalidSyntaxError = toInvalidSyntaxError(exception);
return new ExecutionResultImpl(invalidSyntaxError);
}

private InvalidSyntaxError toInvalidSyntaxError(RecognitionException recognitionException) {
SourceLocation sourceLocation = null;
if (recognitionException != null) {
sourceLocation = new SourceLocation(recognitionException.getOffendingToken().getLine(), recognitionException.getOffendingToken().getCharPositionInLine());
}
return new InvalidSyntaxError(sourceLocation);
}

private static class ParseResult {
private final Document document;
private final RecognitionException exception;

private ParseResult(Document document, RecognitionException exception) {
this.document = document;
this.exception = exception;
}

executionCtx.onEnd(result);
private boolean isFailure() {
return document == null;
}

return result;
private Document getDocument() {
return document;
}

private RecognitionException getException() {
return exception;
}

private static ParseResult of(Document document) {
return new ParseResult(document, null);
}

private static ParseResult ofError(RecognitionException e) {
return new ParseResult(null, e);
}
}
}
8 changes: 7 additions & 1 deletion src/main/java/graphql/execution/Execution.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package graphql.execution;


import graphql.ExecutionInput;
import graphql.ExecutionResult;
import graphql.ExecutionResultImpl;
import graphql.GraphQLException;
Expand Down Expand Up @@ -41,7 +42,12 @@ public Execution(ExecutionStrategy queryStrategy, ExecutionStrategy mutationStra
this.instrumentation = instrumentation;
}

public ExecutionResult execute(ExecutionId executionId, GraphQLSchema graphQLSchema, Object context, Object root, Document document, String operationName, Map<String, Object> variables) {
public ExecutionResult execute(Document document, GraphQLSchema graphQLSchema, ExecutionId executionId, ExecutionInput executionInput) {
final Object context = executionInput.getContext();
final Object root = executionInput.getRoot();
final String operationName = executionInput.getOperationName();
final Map<String, Object> variables = executionInput.getVariables();
Copy link
Contributor Author

@tklovett tklovett Jun 19, 2017

Choose a reason for hiding this comment

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

These can be inlined if you like, but will then cause merge conflicts with #508


ExecutionContextBuilder executionContextBuilder = new ExecutionContextBuilder(new ValuesResolver(), instrumentation);
ExecutionContext executionContext = executionContextBuilder
.executionId(executionId)
Expand Down
11 changes: 11 additions & 0 deletions ...java/graphql/execution/instrumentation/parameters/InstrumentationExecutionParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package graphql.execution.instrumentation.parameters;

import graphql.ExecutionInput;
import graphql.PublicApi;
import graphql.execution.instrumentation.Instrumentation;

import java.util.Collections;
import java.util.Map;

/**
Expand All @@ -15,6 +17,15 @@ public class InstrumentationExecutionParameters {
private final Object context;
private final Map<String, Object> variables;

public InstrumentationExecutionParameters(ExecutionInput executionInput) {
this(
executionInput.getQuery(),
executionInput.getOperationName(),
executionInput.getContext(),
executionInput.getVariables() != null ? executionInput.getVariables() : Collections.emptyMap()
);
}

public InstrumentationExecutionParameters(String query, String operation, Object context, Map<String, Object> variables) {
this.query = query;
this.operation = operation;
Expand Down
7 changes: 3 additions & 4 deletions ...ava/graphql/execution/instrumentation/parameters/InstrumentationValidationParameters.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,18 +1,17 @@
package graphql.execution.instrumentation.parameters;

import graphql.ExecutionInput;
import graphql.execution.instrumentation.Instrumentation;
import graphql.language.Document;

import java.util.Map;

/**
* Parameters sent to {@link Instrumentation} methods
*/
public class InstrumentationValidationParameters extends InstrumentationExecutionParameters {
private final Document document;

public InstrumentationValidationParameters(String query, String operation, Object context, Map<String, Object> arguments, Document document) {
super(query, operation, context, arguments);
public InstrumentationValidationParameters(ExecutionInput executionInput, Document document) {
super(executionInput);
this.document = document;
}

Expand Down
8 changes: 5 additions & 3 deletions src/test/groovy/graphql/execution/ExecutionTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package graphql.execution

import graphql.ExecutionInput
import graphql.MutationSchema
import graphql.execution.instrumentation.NoOpInstrumentation
import graphql.parser.Parser
Expand All @@ -12,6 +13,7 @@ class ExecutionTest extends Specification {
def mutationStrategy = Mock(ExecutionStrategy)
def queryStrategy = Mock(ExecutionStrategy)
def execution = new Execution(queryStrategy, mutationStrategy, subscriptionStrategy, NoOpInstrumentation.INSTANCE)
def emptyExecutionInput = ExecutionInput.newExecutionInput().build()

def "query strategy is used for query requests"() {
given:
Expand All @@ -30,7 +32,7 @@ class ExecutionTest extends Specification {
def document = parser.parseDocument(query)

when:
execution.execute(ExecutionId.generate(), MutationSchema.schema, null, null, document, null, null)
execution.execute(document, MutationSchema.schema, ExecutionId.generate(), emptyExecutionInput)

then:
1 * queryStrategy.execute(*_)
Expand All @@ -50,7 +52,7 @@ class ExecutionTest extends Specification {
def document = parser.parseDocument(query)

when:
execution.execute(ExecutionId.generate(), MutationSchema.schema, null, null, document, null, null)
execution.execute(document, MutationSchema.schema, ExecutionId.generate(), emptyExecutionInput)

then:
0 * queryStrategy.execute(*_)
Expand All @@ -70,7 +72,7 @@ class ExecutionTest extends Specification {
def document = parser.parseDocument(query)

when:
execution.execute(ExecutionId.generate(), MutationSchema.schema, null, null, document, null, null)
execution.execute(document, MutationSchema.schema, ExecutionId.generate(), emptyExecutionInput)

then:
0 * queryStrategy.execute(*_)
Expand Down