Skip to content

Make the graphql-java Asserts truly lazy and hence more efficient #1913

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 5 commits into from
May 21, 2020
Merged

Make the graphql-java Asserts truly lazy and hence more efficient #1913

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8d06b50
Add lazy assertTrue function for performance improvement
Apr 22, 2020
c69e656
Merge branch 'lazyAssert' of https://github.com/klee97/graphql-java i…
bbakerman May 21, 2020
f5b087b
This is a revamp of klee97's PR about lazy asserts.
bbakerman May 21, 2020
0d8cf32
Merge remote-tracking branch 'origin/master' into klee97-lazyAssert
bbakerman May 21, 2020
d5359f4
Fixed up asserts
bbakerman May 21, 2020
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
33 changes: 20 additions & 13 deletions src/main/java/graphql/Assert.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,25 +1,26 @@
package graphql;

@bbakerman bbakerman May 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

This is the KEY file changed and its test.

The others are all knock on effects


import java.util.Collection;
import java.util.function.Supplier;

import static java.lang.String.format;

@SuppressWarnings("TypeParameterUnusedInFormals")
@Internal
public class Assert {

public static <T> T assertNotNull(T object, String format, Object... args) {
public static <T> T assertNotNull(T object, Supplier<String> msg) {
if (object != null) {
return object;
}
throw new AssertException(format(format, args));
throw new AssertException(msg.get());
}

public static <T> T assertNotNullWithNPE(T object, String format, Object... args) {
public static <T> T assertNotNullWithNPE(T object, Supplier<String> msg) {
if (object != null) {
return object;
}
throw new NullPointerException(format(format, args));
throw new NullPointerException(msg.get());
}

public static <T> T assertNotNull(T object) {
Expand All @@ -29,11 +30,11 @@ public static <T> T assertNotNull(T object) {
throw new AssertException("Object required to be not null");
}

public static <T> void assertNull(T object, String format, Object... args) {
public static <T> void assertNull(T object, Supplier<String> msg) {
if (object == null) {
return;
}
throw new AssertException(format(format, args));
throw new AssertException(msg.get());
}

public static <T> void assertNull(T object) {
Expand Down Expand Up @@ -62,18 +63,18 @@ public static <T> Collection<T> assertNotEmpty(Collection<T> collection) {
return collection;
}

public static <T> Collection<T> assertNotEmpty(Collection<T> collection, String format, Object... args) {
public static <T> Collection<T> assertNotEmpty(Collection<T> collection, Supplier<String> msg) {
if (collection == null || collection.isEmpty()) {
throw new AssertException(format(format, args));
throw new AssertException(msg.get());
}
return collection;
}

public static void assertTrue(boolean condition, String format, Object... args) {
public static void assertTrue(boolean condition, Supplier<String> msg) {
if (condition) {
return;
}
throw new AssertException(format(format, args));
throw new AssertException(msg.get());
}

public static void assertTrue(boolean condition) {
Expand All @@ -83,11 +84,18 @@ public static void assertTrue(boolean condition) {
throw new AssertException("condition expected to be true");
}

public static void assertFalse(boolean condition, String format, Object... args) {
public static void assertFalse(boolean condition, Supplier<String> msg) {
if (!condition) {
return;
}
throw new AssertException(format(format, args));
throw new AssertException(msg.get());
}

public static void assertFalse(boolean condition) {
if (!condition) {
return;
}
throw new AssertException("condition expected to be false");
}

@bbakerman bbakerman May 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The above was missing but present for assertTrue so I made it

@bbakerman bbakerman May 21, 2020

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

assertShouldNeverHappen can be as slow as it likes - its ALWAYS going to assert.

I mean we could make it a supplier based on consistency but....I thought its ok to leave it


private static final String invalidNameErrorMessage = "Name must be non-null, non-empty and match [_A-Za-z][_0-9A-Za-z]* - was '%s'";
Expand All @@ -97,7 +105,6 @@ public static void assertFalse(boolean condition, String format, Object... args)
* currently non null, non empty,
*
* @param name - the name to be validated.
*
* @return the name if valid, or AssertException if invalid.
*/
public static String assertValidName(String name) {
Expand Down
6 changes: 3 additions & 3 deletions src/main/java/graphql/ExecutionInput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public class ExecutionInput {

@Internal
private ExecutionInput(String query, String operationName, Object context, Object root, Map<String, Object> variables, DataLoaderRegistry dataLoaderRegistry, CacheControl cacheControl, ExecutionId executionId, Locale locale, Object localContext) {
this.query = assertNotNull(query, "query can't be null");
this.query = assertNotNull(query, () -> "query can't be null");
this.operationName = operationName;
this.context = context;
this.root = root;
Expand Down Expand Up @@ -189,7 +189,7 @@ public static class Builder {
private ExecutionId executionId;

public Builder query(String query) {
this.query = assertNotNull(query, "query can't be null");
this.query = assertNotNull(query, () -> "query can't be null");
return this;
}

Expand Down Expand Up @@ -259,7 +259,7 @@ public Builder root(Object root) {
}

public Builder variables(Map<String, Object> variables) {
this.variables = assertNotNull(variables, "variables map can't be null");
this.variables = assertNotNull(variables, () -> "variables map can't be null");
return this;
}

Expand Down
26 changes: 13 additions & 13 deletions src/main/java/graphql/GraphQL.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -176,13 +176,13 @@ private GraphQL(GraphQLSchema graphQLSchema,
Instrumentation instrumentation,
PreparsedDocumentProvider preparsedDocumentProvider,
ValueUnboxer valueUnboxer) {
this.graphQLSchema = assertNotNull(graphQLSchema, "graphQLSchema must be non null");
this.graphQLSchema = assertNotNull(graphQLSchema, () -> "graphQLSchema must be non null");
this.queryStrategy = queryStrategy != null ? queryStrategy : new AsyncExecutionStrategy();
this.mutationStrategy = mutationStrategy != null ? mutationStrategy : new AsyncSerialExecutionStrategy();
this.subscriptionStrategy = subscriptionStrategy != null ? subscriptionStrategy : new SubscriptionExecutionStrategy();
this.idProvider = assertNotNull(idProvider, "idProvider must be non null");
this.idProvider = assertNotNull(idProvider, () -> "idProvider must be non null");
this.instrumentation = assertNotNull(instrumentation);
this.preparsedDocumentProvider = assertNotNull(preparsedDocumentProvider, "preparsedDocumentProvider must be non null");
this.preparsedDocumentProvider = assertNotNull(preparsedDocumentProvider, () -> "preparsedDocumentProvider must be non null");
this.valueUnboxer = valueUnboxer;
}

Expand Down Expand Up @@ -242,37 +242,37 @@ public Builder(GraphQLSchema graphQLSchema) {
}

public Builder schema(GraphQLSchema graphQLSchema) {
this.graphQLSchema = assertNotNull(graphQLSchema, "GraphQLSchema must be non null");
this.graphQLSchema = assertNotNull(graphQLSchema, () -> "GraphQLSchema must be non null");
return this;
}

public Builder queryExecutionStrategy(ExecutionStrategy executionStrategy) {
this.queryExecutionStrategy = assertNotNull(executionStrategy, "Query ExecutionStrategy must be non null");
this.queryExecutionStrategy = assertNotNull(executionStrategy, () -> "Query ExecutionStrategy must be non null");
return this;
}

public Builder mutationExecutionStrategy(ExecutionStrategy executionStrategy) {
this.mutationExecutionStrategy = assertNotNull(executionStrategy, "Mutation ExecutionStrategy must be non null");
this.mutationExecutionStrategy = assertNotNull(executionStrategy, () -> "Mutation ExecutionStrategy must be non null");
return this;
}

public Builder subscriptionExecutionStrategy(ExecutionStrategy executionStrategy) {
this.subscriptionExecutionStrategy = assertNotNull(executionStrategy, "Subscription ExecutionStrategy must be non null");
this.subscriptionExecutionStrategy = assertNotNull(executionStrategy, () -> "Subscription ExecutionStrategy must be non null");
return this;
}

public Builder instrumentation(Instrumentation instrumentation) {
this.instrumentation = assertNotNull(instrumentation, "Instrumentation must be non null");
this.instrumentation = assertNotNull(instrumentation, () -> "Instrumentation must be non null");
return this;
}

public Builder preparsedDocumentProvider(PreparsedDocumentProvider preparsedDocumentProvider) {
this.preparsedDocumentProvider = assertNotNull(preparsedDocumentProvider, "PreparsedDocumentProvider must be non null");
this.preparsedDocumentProvider = assertNotNull(preparsedDocumentProvider, () -> "PreparsedDocumentProvider must be non null");
return this;
}

public Builder executionIdProvider(ExecutionIdProvider executionIdProvider) {
this.idProvider = assertNotNull(executionIdProvider, "ExecutionIdProvider must be non null");
this.idProvider = assertNotNull(executionIdProvider, () -> "ExecutionIdProvider must be non null");
return this;
}

Expand All @@ -299,9 +299,9 @@ public Builder valueUnboxer(ValueUnboxer valueUnboxer) {
}

public GraphQL build() {
assertNotNull(graphQLSchema, "graphQLSchema must be non null");
assertNotNull(queryExecutionStrategy, "queryStrategy must be non null");
assertNotNull(idProvider, "idProvider must be non null");
assertNotNull(graphQLSchema, () -> "graphQLSchema must be non null");
assertNotNull(queryExecutionStrategy, () -> "queryStrategy must be non null");
assertNotNull(idProvider, () -> "idProvider must be non null");
final Instrumentation augmentedInstrumentation = checkInstrumentationDefaultState(instrumentation, doNotAddDefaultInstrumentations);
return new GraphQL(graphQLSchema, queryExecutionStrategy, mutationExecutionStrategy, subscriptionExecutionStrategy, idProvider, augmentedInstrumentation, preparsedDocumentProvider, valueUnboxer);
}
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/graphql/GraphqlErrorBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -92,7 +92,7 @@ public GraphqlErrorBuilder extensions(Map<String, Object> extensions) {
* @return a newly built GraphqlError
*/
public GraphQLError build() {
assertNotNull(message, "You must provide error message");
assertNotNull(message, () -> "You must provide error message");
return new GraphqlErrorImpl(message, locations, errorType, path, extensions);
}

Expand Down
2 changes: 1 addition & 1 deletion src/main/java/graphql/analysis/MaxQueryComplexityInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -72,7 +72,7 @@ public MaxQueryComplexityInstrumentation(int maxComplexity, FieldComplexityCalcu
public MaxQueryComplexityInstrumentation(int maxComplexity, FieldComplexityCalculator fieldComplexityCalculator,
Function<QueryComplexityInfo, Boolean> maxQueryComplexityExceededFunction) {
this.maxComplexity = maxComplexity;
this.fieldComplexityCalculator = assertNotNull(fieldComplexityCalculator, "calculator can't be null");
this.fieldComplexityCalculator = assertNotNull(fieldComplexityCalculator, () -> "calculator can't be null");
this.maxQueryComplexityExceededFunction = maxQueryComplexityExceededFunction;
}

Expand Down
6 changes: 4 additions & 2 deletions src/main/java/graphql/analysis/NodeVisitorWithTypeTracking.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,7 @@
import static graphql.Assert.assertNotNull;
import static graphql.schema.GraphQLTypeUtil.unwrapAll;
import static graphql.util.TraverserContext.Phase.LEAVE;
import static java.lang.String.format;

/**
* Internally used node visitor which delegates to a {@link QueryVisitor} with type
Expand Down Expand Up @@ -138,8 +139,9 @@ public TraversalControl visitFragmentSpread(FragmentSpread fragmentSpread, Trave
QueryTraversalContext parentEnv = context.getVarFromParents(QueryTraversalContext.class);

GraphQLCompositeType typeCondition = (GraphQLCompositeType) schema.getType(fragmentDefinition.getTypeCondition().getName());
assertNotNull(typeCondition, "Invalid type condition '%s' in fragment '%s'", fragmentDefinition.getTypeCondition().getName(),
fragmentDefinition.getName());
assertNotNull(typeCondition,
() -> format("Invalid type condition '%s' in fragment '%s'", fragmentDefinition.getTypeCondition().getName(),
fragmentDefinition.getName()));
context.setVar(QueryTraversalContext.class, new QueryTraversalContext(typeCondition, parentEnv.getEnvironment(), fragmentDefinition));
return TraversalControl.CONTINUE;
}
Expand Down
8 changes: 4 additions & 4 deletions src/main/java/graphql/analysis/QueryTransformer.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,11 +44,11 @@ private QueryTransformer(GraphQLSchema schema,
GraphQLCompositeType rootParentType,
Map<String, FragmentDefinition> fragmentsByName,
Map<String, Object> variables) {
this.schema = assertNotNull(schema, "schema can't be null");
this.variables = assertNotNull(variables, "variables can't be null");
this.root = assertNotNull(root, "root can't be null");
this.schema = assertNotNull(schema, () -> "schema can't be null");
this.variables = assertNotNull(variables, () -> "variables can't be null");
this.root = assertNotNull(root, () -> "root can't be null");
this.rootParentType = assertNotNull(rootParentType);
this.fragmentsByName = assertNotNull(fragmentsByName, "fragmentsByName can't be null");
this.fragmentsByName = assertNotNull(fragmentsByName, () -> "fragmentsByName can't be null");
}

/**
Expand Down
16 changes: 8 additions & 8 deletions src/main/java/graphql/analysis/QueryTraverser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,10 +48,10 @@ private QueryTraverser(GraphQLSchema schema,
Document document,
String operation,
Map<String, Object> variables) {
assertNotNull(document, "document can't be null");
assertNotNull(document, () -> "document can't be null");
NodeUtil.GetOperationResult getOperationResult = NodeUtil.getOperation(document, operation);
this.schema = assertNotNull(schema, "schema can't be null");
this.variables = assertNotNull(variables, "variables can't be null");
this.schema = assertNotNull(schema, () -> "schema can't be null");
this.variables = assertNotNull(variables, () -> "variables can't be null");
this.fragmentsByName = getOperationResult.fragmentsByName;
this.roots = singletonList(getOperationResult.operationDefinition);
this.rootParentType = getRootTypeFromOperation(getOperationResult.operationDefinition);
Expand All @@ -62,12 +62,12 @@ private QueryTraverser(GraphQLSchema schema,
GraphQLCompositeType rootParentType,
Map<String, FragmentDefinition> fragmentsByName,
Map<String, Object> variables) {
this.schema = assertNotNull(schema, "schema can't be null");
this.variables = assertNotNull(variables, "variables can't be null");
assertNotNull(root, "root can't be null");
this.schema = assertNotNull(schema, () -> "schema can't be null");
this.variables = assertNotNull(variables, () -> "variables can't be null");
assertNotNull(root, () -> "root can't be null");
this.roots = Collections.singleton(root);
this.rootParentType = assertNotNull(rootParentType, "rootParentType can't be null");
this.fragmentsByName = assertNotNull(fragmentsByName, "fragmentsByName can't be null");
this.rootParentType = assertNotNull(rootParentType, () -> "rootParentType can't be null");
this.fragmentsByName = assertNotNull(fragmentsByName, () -> "fragmentsByName can't be null");
}

public Object visitDepthFirst(QueryVisitor queryVisitor) {
Expand Down
4 changes: 2 additions & 2 deletions src/main/java/graphql/execution/Async.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,7 +49,7 @@ public static <T, U> CompletableFuture<List<U>> each(Iterable<T> list, BiFunctio
CompletableFuture<U> cf;
try {
cf = cfFactory.apply(t, index++);
Assert.assertNotNull(cf, "cfFactory must return a non null value");
Assert.assertNotNull(cf, () -> "cfFactory must return a non null value");
} catch (Exception e) {
cf = new CompletableFuture<>();
// Async.each makes sure that it is not a CompletionException inside a CompletionException
Expand All @@ -75,7 +75,7 @@ private static <T, U> void eachSequentiallyImpl(Iterator<T> iterator, CFFactory<
CompletableFuture<U> cf;
try {
cf = cfFactory.apply(iterator.next(), index, tmpResult);
Assert.assertNotNull(cf, "cfFactory must return a non null value");
Assert.assertNotNull(cf, () -> "cfFactory must return a non null value");
} catch (Exception e) {
cf = new CompletableFuture<>();
cf.completeExceptionally(new CompletionException(e));
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/graphql/execution/ExecutionContextBuilder.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ public ExecutionContextBuilder valueUnboxer(ValueUnboxer valueUnboxer) {

public ExecutionContext build() {
// preconditions
assertNotNull(executionId, "You must provide a query identifier");
assertNotNull(executionId, () -> "You must provide a query identifier");

return new ExecutionContext(
instrumentation,
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/graphql/execution/ExecutionId.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ public static ExecutionId from(String id) {
private final String id;

private ExecutionId(String id) {
Assert.assertNotNull(id, "You must provided a non null id");
Assert.assertNotNull(id, () -> "You must provided a non null id");
this.id = id;
}

Expand Down
Loading