From aaa00839f9b4aafed348620ff10372dbc1bfb65c Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Mon, 4 Mar 2024 10:01:09 +1100 Subject: [PATCH 1/6] Made the field definition lookup more optimised --- .../graphql/execution/ExecutionStrategy.java | 2 +- .../graphql/introspection/Introspection.java | 46 ++++++++++++++++--- 2 files changed, 40 insertions(+), 8 deletions(-) diff --git a/src/main/java/graphql/execution/ExecutionStrategy.java b/src/main/java/graphql/execution/ExecutionStrategy.java index 0d79d326b8..4059aa2d6d 100644 --- a/src/main/java/graphql/execution/ExecutionStrategy.java +++ b/src/main/java/graphql/execution/ExecutionStrategy.java @@ -923,7 +923,7 @@ protected GraphQLFieldDefinition getFieldDef(ExecutionContext executionContext, * @return a {@link GraphQLFieldDefinition} */ protected GraphQLFieldDefinition getFieldDef(GraphQLSchema schema, GraphQLObjectType parentType, Field field) { - return Introspection.getFieldDef(schema, parentType, field.getName()); + return Introspection.getFieldDefinition(schema, parentType, field.getName()); } /** diff --git a/src/main/java/graphql/introspection/Introspection.java b/src/main/java/graphql/introspection/Introspection.java index d496e03702..b884fc84c1 100644 --- a/src/main/java/graphql/introspection/Introspection.java +++ b/src/main/java/graphql/introspection/Introspection.java @@ -702,6 +702,43 @@ public static boolean isIntrospectionTypes(GraphQLNamedType type) { */ public static GraphQLFieldDefinition getFieldDef(GraphQLSchema schema, GraphQLCompositeType parentType, String fieldName) { + GraphQLFieldDefinition fieldDefinition = getSystemFieldDef(schema, parentType, fieldName); + if (fieldDefinition != null) { + return fieldDefinition; + } + + assertTrue(parentType instanceof GraphQLFieldsContainer, () -> String.format("should not happen : parent type must be an object or interface %s", parentType)); + GraphQLFieldsContainer fieldsContainer = (GraphQLFieldsContainer) parentType; + fieldDefinition = schema.getCodeRegistry().getFieldVisibility().getFieldDefinition(fieldsContainer, fieldName); + assertTrue(fieldDefinition != null, () -> String.format("Unknown field '%s' for type %s", fieldName, fieldsContainer.getName())); + return fieldDefinition; + } + + /** + * This will look up a field definition by name, and understand that fields like __typename and __schema are special + * and take precedence in field resolution + * + * @param schema the schema to use + * @param parentType the type of the parent {@link GraphQLFieldsContainer} + * @param fieldName the field to look up + * + * @return a field definition otherwise throws an assertion exception if it's null + */ + public static GraphQLFieldDefinition getFieldDefinition(GraphQLSchema schema, GraphQLFieldsContainer parentType, String fieldName) { + // this method is optimized to look up the most common case first (type for field) and hence suits the hot path of the execution engine + // and as a small benefit does not allocate any assertions unless it completely failed + GraphQLFieldDefinition fieldDefinition = schema.getCodeRegistry().getFieldVisibility().getFieldDefinition(parentType, fieldName); + if (fieldDefinition == null) { + // we look up system fields second because they are less likely to be the field in question + fieldDefinition = getSystemFieldDef(schema, parentType, fieldName); + if (fieldDefinition == null) { + Assert.assertShouldNeverHappen(String.format("Unknown field '%s' for type %s", fieldName, parentType.getName())); + } + } + return fieldDefinition; + } + + private static GraphQLFieldDefinition getSystemFieldDef(GraphQLSchema schema, GraphQLCompositeType parentType, String fieldName) { if (schema.getQueryType() == parentType) { if (fieldName.equals(schema.getIntrospectionSchemaFieldDefinition().getName())) { return schema.getIntrospectionSchemaFieldDefinition(); @@ -713,11 +750,6 @@ public static GraphQLFieldDefinition getFieldDef(GraphQLSchema schema, GraphQLCo if (fieldName.equals(schema.getIntrospectionTypenameFieldDefinition().getName())) { return schema.getIntrospectionTypenameFieldDefinition(); } - - assertTrue(parentType instanceof GraphQLFieldsContainer, () -> String.format("should not happen : parent type must be an object or interface %s", parentType)); - GraphQLFieldsContainer fieldsContainer = (GraphQLFieldsContainer) parentType; - GraphQLFieldDefinition fieldDefinition = schema.getCodeRegistry().getFieldVisibility().getFieldDefinition(fieldsContainer, fieldName); - assertTrue(fieldDefinition != null, () -> String.format("Unknown field '%s' for type %s", fieldName, fieldsContainer.getName())); - return fieldDefinition; + return null; } -} +} \ No newline at end of file From 6b242d4ffb6bea34ba3cfbae3db89a35081bbe10 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 26 Mar 2024 16:09:16 +1100 Subject: [PATCH 2/6] Removed old usages and deprecated the old --- .../java/graphql/analysis/NodeVisitorWithTypeTracking.java | 4 ++-- src/main/java/graphql/introspection/Introspection.java | 2 ++ .../normalized/ExecutableNormalizedOperationFactory.java | 5 +++-- .../ExecutableNormalizedOperationToAstCompiler.java | 4 ++-- 4 files changed, 9 insertions(+), 6 deletions(-) diff --git a/src/main/java/graphql/analysis/NodeVisitorWithTypeTracking.java b/src/main/java/graphql/analysis/NodeVisitorWithTypeTracking.java index 683138e387..fc33070e8c 100644 --- a/src/main/java/graphql/analysis/NodeVisitorWithTypeTracking.java +++ b/src/main/java/graphql/analysis/NodeVisitorWithTypeTracking.java @@ -152,7 +152,7 @@ public TraversalControl visitField(Field field, TraverserContext context) QueryTraversalContext parentEnv = context.getVarFromParents(QueryTraversalContext.class); GraphQLContext graphQLContext = parentEnv.getGraphQLContext(); - GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDef(schema, (GraphQLCompositeType) unwrapAll(parentEnv.getOutputType()), field.getName()); + GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDefinition(schema, (GraphQLFieldsContainer) unwrapAll(parentEnv.getOutputType()), field.getName()); boolean isTypeNameIntrospectionField = fieldDefinition == schema.getIntrospectionTypenameFieldDefinition(); GraphQLFieldsContainer fieldsContainer = !isTypeNameIntrospectionField ? (GraphQLFieldsContainer) unwrapAll(parentEnv.getOutputType()) : null; GraphQLCodeRegistry codeRegistry = schema.getCodeRegistry(); @@ -203,7 +203,7 @@ public TraversalControl visitArgument(Argument argument, TraverserContext QueryVisitorFieldEnvironment fieldEnv = fieldCtx.getEnvironment(); GraphQLFieldsContainer fieldsContainer = fieldEnv.getFieldsContainer(); - GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDef(schema, fieldsContainer, field.getName()); + GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDefinition(schema, fieldsContainer, field.getName()); GraphQLArgument graphQLArgument = fieldDefinition.getArgument(argument.getName()); String argumentName = graphQLArgument.getName(); diff --git a/src/main/java/graphql/introspection/Introspection.java b/src/main/java/graphql/introspection/Introspection.java index 79c2ef3a48..7bf0c70338 100644 --- a/src/main/java/graphql/introspection/Introspection.java +++ b/src/main/java/graphql/introspection/Introspection.java @@ -792,6 +792,7 @@ public static boolean isIntrospectionTypes(String typeName) { * * @return a field definition otherwise throws an assertion exception if it's null */ + @Deprecated(since = "2024-03-24", forRemoval = true) public static GraphQLFieldDefinition getFieldDef(GraphQLSchema schema, GraphQLCompositeType parentType, String fieldName) { GraphQLFieldDefinition fieldDefinition = getSystemFieldDef(schema, parentType, fieldName); @@ -800,6 +801,7 @@ public static GraphQLFieldDefinition getFieldDef(GraphQLSchema schema, GraphQLCo } assertTrue(parentType instanceof GraphQLFieldsContainer, () -> String.format("should not happen : parent type must be an object or interface %s", parentType)); + @SuppressWarnings("DataFlowIssue") GraphQLFieldsContainer fieldsContainer = (GraphQLFieldsContainer) parentType; fieldDefinition = schema.getCodeRegistry().getFieldVisibility().getFieldDefinition(fieldsContainer, fieldName); assertTrue(fieldDefinition != null, () -> String.format("Unknown field '%s' for type %s", fieldName, fieldsContainer.getName())); diff --git a/src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java b/src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java index 735ab899d9..07b449e69a 100644 --- a/src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java +++ b/src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java @@ -34,6 +34,7 @@ import graphql.schema.FieldCoordinates; import graphql.schema.GraphQLCompositeType; import graphql.schema.GraphQLFieldDefinition; +import graphql.schema.GraphQLFieldsContainer; import graphql.schema.GraphQLInterfaceType; import graphql.schema.GraphQLNamedOutputType; import graphql.schema.GraphQLObjectType; @@ -562,7 +563,7 @@ public CollectNFResult collectFromMergedField(ExecutableNormalizedField executab if (fieldAndAstParent.field.getSelectionSet() == null) { continue; } - GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDef(graphQLSchema, fieldAndAstParent.astParentType, fieldAndAstParent.field.getName()); + GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDefinition(graphQLSchema, (GraphQLFieldsContainer) fieldAndAstParent.astParentType, fieldAndAstParent.field.getName()); GraphQLUnmodifiedType astParentType = unwrapAll(fieldDefinition.getType()); this.collectFromSelectionSet(fieldAndAstParent.field.getSelectionSet(), collectedFields, @@ -644,7 +645,7 @@ private ExecutableNormalizedField createNF(CollectedFieldGroup collectedFieldGro Set objectTypes = collectedFieldGroup.objectTypes; field = collectedFieldGroup.fields.iterator().next().field; String fieldName = field.getName(); - GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDef(graphQLSchema, objectTypes.iterator().next(), fieldName); + GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDefinition(graphQLSchema, objectTypes.iterator().next(), fieldName); Map argumentValues = ValuesResolver.getArgumentValues(fieldDefinition.getArguments(), field.getArguments(), CoercedVariables.of(this.coercedVariableValues.toMap()), this.options.graphQLContext, this.options.locale); Map normalizedArgumentValues = null; diff --git a/src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java b/src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java index 877decb767..daa70ecc0d 100644 --- a/src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java +++ b/src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java @@ -24,8 +24,8 @@ import graphql.language.TypeName; import graphql.language.Value; import graphql.normalized.incremental.NormalizedDeferredExecution; -import graphql.schema.GraphQLCompositeType; import graphql.schema.GraphQLFieldDefinition; +import graphql.schema.GraphQLFieldsContainer; import graphql.schema.GraphQLObjectType; import graphql.schema.GraphQLSchema; import graphql.schema.GraphQLUnmodifiedType; @@ -465,7 +465,7 @@ private static Value argValue(ExecutableNormalizedField executableNormalizedF private static GraphQLFieldDefinition getFieldDefinition(GraphQLSchema schema, String parentType, ExecutableNormalizedField nf) { - return Introspection.getFieldDef(schema, (GraphQLCompositeType) schema.getType(parentType), nf.getName()); + return Introspection.getFieldDefinition(schema, (GraphQLFieldsContainer) schema.getType(parentType), nf.getName()); } From a27be65e53075c28375718b528aefb6632569a2a Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 26 Mar 2024 16:42:04 +1100 Subject: [PATCH 3/6] No longer deprecated --- .../graphql/analysis/NodeVisitorWithTypeTracking.java | 2 +- src/main/java/graphql/introspection/Introspection.java | 9 ++++----- 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/src/main/java/graphql/analysis/NodeVisitorWithTypeTracking.java b/src/main/java/graphql/analysis/NodeVisitorWithTypeTracking.java index fc33070e8c..2a2b237aeb 100644 --- a/src/main/java/graphql/analysis/NodeVisitorWithTypeTracking.java +++ b/src/main/java/graphql/analysis/NodeVisitorWithTypeTracking.java @@ -152,7 +152,7 @@ public TraversalControl visitField(Field field, TraverserContext context) QueryTraversalContext parentEnv = context.getVarFromParents(QueryTraversalContext.class); GraphQLContext graphQLContext = parentEnv.getGraphQLContext(); - GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDefinition(schema, (GraphQLFieldsContainer) unwrapAll(parentEnv.getOutputType()), field.getName()); + GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDef(schema, (GraphQLCompositeType) unwrapAll(parentEnv.getOutputType()), field.getName()); boolean isTypeNameIntrospectionField = fieldDefinition == schema.getIntrospectionTypenameFieldDefinition(); GraphQLFieldsContainer fieldsContainer = !isTypeNameIntrospectionField ? (GraphQLFieldsContainer) unwrapAll(parentEnv.getOutputType()) : null; GraphQLCodeRegistry codeRegistry = schema.getCodeRegistry(); diff --git a/src/main/java/graphql/introspection/Introspection.java b/src/main/java/graphql/introspection/Introspection.java index 7bf0c70338..ced6cbf818 100644 --- a/src/main/java/graphql/introspection/Introspection.java +++ b/src/main/java/graphql/introspection/Introspection.java @@ -784,7 +784,8 @@ public static boolean isIntrospectionTypes(String typeName) { /** * This will look up a field definition by name, and understand that fields like __typename and __schema are special - * and take precedence in field resolution + * and take precedence in field resolution. If the parent type is a union type, then the only field allowed + * is `__typename`. * * @param schema the schema to use * @param parentType the type of the parent object @@ -792,7 +793,6 @@ public static boolean isIntrospectionTypes(String typeName) { * * @return a field definition otherwise throws an assertion exception if it's null */ - @Deprecated(since = "2024-03-24", forRemoval = true) public static GraphQLFieldDefinition getFieldDef(GraphQLSchema schema, GraphQLCompositeType parentType, String fieldName) { GraphQLFieldDefinition fieldDefinition = getSystemFieldDef(schema, parentType, fieldName); @@ -800,11 +800,10 @@ public static GraphQLFieldDefinition getFieldDef(GraphQLSchema schema, GraphQLCo return fieldDefinition; } - assertTrue(parentType instanceof GraphQLFieldsContainer, () -> String.format("should not happen : parent type must be an object or interface %s", parentType)); - @SuppressWarnings("DataFlowIssue") + assertTrue(parentType instanceof GraphQLFieldsContainer, "should not happen : parent type must be an object or interface %s", parentType); GraphQLFieldsContainer fieldsContainer = (GraphQLFieldsContainer) parentType; fieldDefinition = schema.getCodeRegistry().getFieldVisibility().getFieldDefinition(fieldsContainer, fieldName); - assertTrue(fieldDefinition != null, () -> String.format("Unknown field '%s' for type %s", fieldName, fieldsContainer.getName())); + assertTrue(fieldDefinition != null, "Unknown field '%s' for type %s", fieldName, fieldsContainer.getName()); return fieldDefinition; } From b5f3df8701f960f930a61a0cce72c0d1ac02c0e3 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 26 Mar 2024 16:44:04 +1100 Subject: [PATCH 4/6] Made old code the same --- .../normalized/ExecutableNormalizedOperationFactory.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java b/src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java index 07b449e69a..7ed78470d5 100644 --- a/src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java +++ b/src/main/java/graphql/normalized/ExecutableNormalizedOperationFactory.java @@ -563,7 +563,7 @@ public CollectNFResult collectFromMergedField(ExecutableNormalizedField executab if (fieldAndAstParent.field.getSelectionSet() == null) { continue; } - GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDefinition(graphQLSchema, (GraphQLFieldsContainer) fieldAndAstParent.astParentType, fieldAndAstParent.field.getName()); + GraphQLFieldDefinition fieldDefinition = Introspection.getFieldDef(graphQLSchema, fieldAndAstParent.astParentType, fieldAndAstParent.field.getName()); GraphQLUnmodifiedType astParentType = unwrapAll(fieldDefinition.getType()); this.collectFromSelectionSet(fieldAndAstParent.field.getSelectionSet(), collectedFields, From d65674130203cb1f29ed9567edb1db80caf533d9 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 26 Mar 2024 16:48:50 +1100 Subject: [PATCH 5/6] Made old code the same - more of that --- .../normalized/ExecutableNormalizedOperationToAstCompiler.java | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java b/src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java index daa70ecc0d..b8ccc21d43 100644 --- a/src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java +++ b/src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java @@ -24,6 +24,7 @@ import graphql.language.TypeName; import graphql.language.Value; import graphql.normalized.incremental.NormalizedDeferredExecution; +import graphql.schema.GraphQLCompositeType; import graphql.schema.GraphQLFieldDefinition; import graphql.schema.GraphQLFieldsContainer; import graphql.schema.GraphQLObjectType; @@ -465,7 +466,7 @@ private static Value argValue(ExecutableNormalizedField executableNormalizedF private static GraphQLFieldDefinition getFieldDefinition(GraphQLSchema schema, String parentType, ExecutableNormalizedField nf) { - return Introspection.getFieldDefinition(schema, (GraphQLFieldsContainer) schema.getType(parentType), nf.getName()); + return Introspection.getFieldDef(schema, (GraphQLCompositeType) schema.getType(parentType), nf.getName()); } From 9669753c7563776375b38289f5bbe3b8fbe49655 Mon Sep 17 00:00:00 2001 From: Brad Baker Date: Tue, 26 Mar 2024 16:49:37 +1100 Subject: [PATCH 6/6] Made old code the same - more of that - bad import --- .../normalized/ExecutableNormalizedOperationToAstCompiler.java | 1 - 1 file changed, 1 deletion(-) diff --git a/src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java b/src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java index b8ccc21d43..877decb767 100644 --- a/src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java +++ b/src/main/java/graphql/normalized/ExecutableNormalizedOperationToAstCompiler.java @@ -26,7 +26,6 @@ import graphql.normalized.incremental.NormalizedDeferredExecution; import graphql.schema.GraphQLCompositeType; import graphql.schema.GraphQLFieldDefinition; -import graphql.schema.GraphQLFieldsContainer; import graphql.schema.GraphQLObjectType; import graphql.schema.GraphQLSchema; import graphql.schema.GraphQLUnmodifiedType;