Skip to content

Instrument usage of ExecutableNormalizedOperationFactory.createExecutableNormalizedOperation #4045

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

Open
timward60 wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Instrument usage of ExecutableNormalizedOperationFactory.createExecutableNormalizedOperation #4045

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
d0b2634
Instrument usage of ExecutableNormalizedOperationFactory.createExecut…
timward60 Jul 21, 2023
6ac9b17
Use interface for parsed normalized operation
timward60 Jul 9, 2025
3729496
Fix imports.
timward60 Jul 10, 2025
3a4401f
Add Parameters suffix to InstrumentationParsedNormalizedOperation
timward60 Jul 10, 2025
0ed10be
Fix imports for NoContextChainedInstrumentation
timward60 Jul 10, 2025
61400e6
Rename beginParsedNormalizedOperation to beginCreateNormalizedOperation
timward60 Jul 10, 2025
9aa6d72
Fix imports.
timward60 Jul 10, 2025
5b74d1f
Update naming in tests.
timward60 Jul 10, 2025
7c8b8aa
Rename ParsedNormalizedOperation to GraphQlNormalizedOperation
timward60 Jul 10, 2025
e9baf2f
Remove public common interface for ExectuableNormalizedOperations and…
timward60 Jul 13, 2025
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
13 changes: 12 additions & 1 deletion src/main/java/graphql/execution/ExecutionContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@
import graphql.execution.incremental.IncrementalCallState;
import graphql.execution.instrumentation.Instrumentation;
import graphql.execution.instrumentation.InstrumentationState;
import graphql.execution.instrumentation.parameters.InstrumentationParsedNormalizedOperation;
import graphql.language.Document;
import graphql.language.FragmentDefinition;
import graphql.language.OperationDefinition;
Expand Down Expand Up @@ -100,7 +101,7 @@ public class ExecutionContext {
this.localContext = builder.localContext;
this.executionInput = builder.executionInput;
this.dataLoaderDispatcherStrategy = builder.dataLoaderDispatcherStrategy;
this.queryTree = FpKit.interThreadMemoize(() -> ExecutableNormalizedOperationFactory.createExecutableNormalizedOperation(graphQLSchema, operationDefinition, fragmentsByName, coercedVariables));
this.queryTree = FpKit.interThreadMemoize(this::createExecutableNormalizedOperation);
this.propagateErrorsOnNonNullContractFailure = builder.propagateErrorsOnNonNullContractFailure;
this.engineRunningState = builder.engineRunningState;
}
Expand Down Expand Up @@ -368,6 +369,16 @@ public ResultNodesInfo getResultNodesInfo() {
return resultNodesInfo;
}

private ExecutableNormalizedOperation createExecutableNormalizedOperation() {
var parameters = new InstrumentationParsedNormalizedOperation(executionInput, graphQLSchema);
var instrument = instrumentation.beginParsedNormalizedOperation(parameters, instrumentationState);
var result = ExecutableNormalizedOperationFactory.createExecutableNormalizedOperation(graphQLSchema, operationDefinition, fragmentsByName, coercedVariables);
if (instrument != null) {
instrument.onCompleted(result, null);
}
return result;
}

Comment thread
timward60 marked this conversation as resolved.
Resolved
@Internal
public boolean hasIncrementalSupport() {
GraphQLContext graphqlContext = getGraphQLContext();
Expand Down
16 changes: 8 additions & 8 deletions src/main/java/graphql/execution/instrumentation/ChainedInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,15 +8,9 @@
import graphql.execution.Async;
import graphql.execution.ExecutionContext;
import graphql.execution.FieldValueInfo;
import graphql.execution.instrumentation.parameters.InstrumentationCreateStateParameters;
import graphql.execution.instrumentation.parameters.InstrumentationExecuteOperationParameters;
import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters;
import graphql.execution.instrumentation.parameters.InstrumentationExecutionStrategyParameters;
import graphql.execution.instrumentation.parameters.InstrumentationFieldCompleteParameters;
import graphql.execution.instrumentation.parameters.InstrumentationFieldFetchParameters;
import graphql.execution.instrumentation.parameters.InstrumentationFieldParameters;
import graphql.execution.instrumentation.parameters.InstrumentationValidationParameters;
import graphql.execution.instrumentation.parameters.*;
import graphql.language.Document;
import graphql.normalized.ParsedNormalizedOperation;
import graphql.schema.DataFetcher;
import graphql.schema.GraphQLSchema;
import graphql.validation.ValidationError;
Expand Down Expand Up @@ -126,6 +120,12 @@ public InstrumentationContext<Document> beginParse(InstrumentationExecutionParam
return chainedCtx(state, (instrumentation, specificState) -> instrumentation.beginParse(parameters, specificState));
}

@Override
public InstrumentationContext<ParsedNormalizedOperation> beginParsedNormalizedOperation(InstrumentationParsedNormalizedOperation parameters, InstrumentationState state) {
return chainedCtx(state, (instrumentation, specificState) ->
instrumentation.beginParsedNormalizedOperation(parameters, specificState));
}


@Override
public InstrumentationContext<List<ValidationError>> beginValidation(InstrumentationValidationParameters parameters, InstrumentationState state) {
Expand Down
23 changes: 15 additions & 8 deletions src/main/java/graphql/execution/instrumentation/Instrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,15 +5,10 @@
import graphql.ExperimentalApi;
import graphql.PublicSpi;
import graphql.execution.ExecutionContext;
import graphql.execution.instrumentation.parameters.InstrumentationCreateStateParameters;
import graphql.execution.instrumentation.parameters.InstrumentationExecuteOperationParameters;
import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters;
import graphql.execution.instrumentation.parameters.InstrumentationExecutionStrategyParameters;
import graphql.execution.instrumentation.parameters.InstrumentationFieldCompleteParameters;
import graphql.execution.instrumentation.parameters.InstrumentationFieldFetchParameters;
import graphql.execution.instrumentation.parameters.InstrumentationFieldParameters;
import graphql.execution.instrumentation.parameters.InstrumentationValidationParameters;
import graphql.execution.instrumentation.parameters.*;
Comment thread
timward60 marked this conversation as resolved.
Resolved
import graphql.language.Document;
import graphql.normalized.ExecutableNormalizedOperation;
import graphql.normalized.ParsedNormalizedOperation;
import graphql.schema.DataFetcher;
import graphql.schema.GraphQLSchema;
import graphql.validation.ValidationError;
Expand Down Expand Up @@ -120,6 +115,18 @@ default InstrumentationContext<ExecutionResult> beginExecuteOperation(Instrument
return noOp();
}

/**
* This is called just before the parsing of the normalized operation is started.
*
* @param parameters the parameters to this step
* @param state the state created during the call to {@link #createState(InstrumentationCreateStateParameters)}
* @return a nullable {@link InstrumentationContext} object that will be called back when the step ends (assuming it's not null)
*/
@Nullable
default InstrumentationContext<ParsedNormalizedOperation> beginParsedNormalizedOperation(InstrumentationParsedNormalizedOperation parameters, InstrumentationState state) {
return noOp();
}

/**
* This is called each time an {@link graphql.execution.ExecutionStrategy} is invoked, which may be multiple times
* per query as the engine recursively descends over the query.
Expand Down
15 changes: 8 additions & 7 deletions src/main/java/graphql/execution/instrumentation/NoContextChainedInstrumentation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,14 +2,10 @@

import graphql.ExecutionResult;
import graphql.PublicApi;
import graphql.execution.instrumentation.parameters.InstrumentationExecuteOperationParameters;
import graphql.execution.instrumentation.parameters.InstrumentationExecutionParameters;
import graphql.execution.instrumentation.parameters.InstrumentationExecutionStrategyParameters;
import graphql.execution.instrumentation.parameters.InstrumentationFieldCompleteParameters;
import graphql.execution.instrumentation.parameters.InstrumentationFieldFetchParameters;
import graphql.execution.instrumentation.parameters.InstrumentationFieldParameters;
import graphql.execution.instrumentation.parameters.InstrumentationValidationParameters;
import graphql.execution.instrumentation.parameters.*;
import graphql.language.Document;
import graphql.normalized.ExecutableNormalizedOperation;
import graphql.normalized.ParsedNormalizedOperation;
import graphql.validation.ValidationError;
import org.jspecify.annotations.Nullable;

Expand Down Expand Up @@ -83,6 +79,11 @@ public ExecutionStrategyInstrumentationContext beginExecutionStrategy(Instrument
return runAll(state, (instrumentation, specificState) -> instrumentation.beginExecuteObject(parameters, specificState));
}

@Override
public @Nullable InstrumentationContext<ParsedNormalizedOperation> beginParsedNormalizedOperation(InstrumentationParsedNormalizedOperation parameters, InstrumentationState state) {
return runAll(state, (instrumentation, specificState) -> instrumentation.beginParsedNormalizedOperation(parameters, specificState));
}

@Override
public InstrumentationContext<ExecutionResult> beginSubscribedFieldEvent(InstrumentationFieldParameters parameters, InstrumentationState state) {
return runAll(state, (instrumentation, specificState) -> instrumentation.beginSubscribedFieldEvent(parameters, specificState));
Expand Down
17 changes: 17 additions & 0 deletions ...raphql/execution/instrumentation/parameters/InstrumentationParsedNormalizedOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
package graphql.execution.instrumentation.parameters;

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

/**
* Parameters sent to {@link Instrumentation} methods
*/
@SuppressWarnings("TypeParameterUnusedInFormals")
@PublicApi
public class InstrumentationParsedNormalizedOperation extends InstrumentationExecutionParameters {
public InstrumentationParsedNormalizedOperation(ExecutionInput executionInput, GraphQLSchema schema) {
super(executionInput, schema);
}
}
Comment thread
timward60 marked this conversation as resolved.
Resolved
2 changes: 1 addition & 1 deletion src/main/java/graphql/normalized/ExecutableNormalizedOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@
* An operation consists of a list of {@link ExecutableNormalizedField}s in a parent child hierarchy
*/
@PublicApi
public class ExecutableNormalizedOperation {
public class ExecutableNormalizedOperation implements ParsedNormalizedOperation {

@bbakerman bbakerman Jul 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahh you are going for a common interface - again I dont think its "parsed" - this is not parsing - parsing has a very specific name in this code base - eg text -> AST elements

@timward60 timward60 Jul 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Yes this bit I expected some discussion. Possibly I should rename the new normalized operation implementation classes to something else then use NormalizedOperation as the interface name, otherwise if there is a better interface name then we can change to that. Let me know what you think?

@timward60 timward60 Jul 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

For reference here is the follow up PR (WIP) to enable a config to switch between ExecutableNormalizedOperation and NormalizedOperation. #4047

@bbakerman bbakerman Jul 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ahh didnt see your replies when I made more naming suggestions

@timward60 timward60 Jul 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

name storming

NormalizedOperation + ExecutableNormalizedOperation

What is the common name here?

NormalOperation
GraphQlOperation
GraphQlNormalizedOperation

Leaning towards GraphQlNormalizedOperation.

@andimarek andimarek Jul 12, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@bbakerman and I had a chat and thinking more about it I am not 100% sure we really want an interface and two different implementations.

Not only the naming makes it clear that this is very hard to distinguish ... maybe we really want to have just NormalizedField which is a combination of ENF and the current NF.

@timward60 timward60 Jul 12, 2025 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

I see. I will still need an interface internally to allow the switching between ENO and NO for experimental usage, but can hide that.

Re the instrumentation do we want a separate method for executable normalized operation and normalized document creation, or should I just create a result container that can hold either types?

private final OperationDefinition.Operation operation;
private final String operationName;
private final List<ExecutableNormalizedField> topLevelFields;
Expand Down
13 changes: 13 additions & 0 deletions src/main/java/graphql/normalized/ParsedNormalizedOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,13 @@
package graphql.normalized;

import graphql.PublicApi;

/**
* A {@link ParsedNormalizedOperation} represents a parsed and normalized operation in GraphQL.
* It is used to handle the execution of GraphQL operations according to the specification,
* including merging duplicate fields and handling type detection for fields that may correspond
* to multiple object types.
*/
@PublicApi
public interface ParsedNormalizedOperation {

@bbakerman bbakerman Jul 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

name is hard - but again I dont think this is Parsed - but I am not sure on a name - but I cant help shaking the feeling its not parsed

@timward60 timward60 Jul 10, 2025

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Tracking the naming discussion here: https://github.com/graphql-java/graphql-java/pull/4045/files#r2196386356

@bbakerman bbakerman Jul 10, 2025

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

name storming

NormalizedOperation + ExecutableNormalizedOperation

What is the common name here?

  • NormalOperation
  • GraphQlOperation
  • GraphQlNormalizedOperation

}
3 changes: 2 additions & 1 deletion src/main/java/graphql/normalized/nf/NormalizedOperation.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,6 +8,7 @@
import graphql.execution.directives.QueryDirectives;
import graphql.language.Field;
import graphql.language.OperationDefinition;
import graphql.normalized.ParsedNormalizedOperation;
import graphql.schema.FieldCoordinates;
import graphql.schema.GraphQLFieldsContainer;

Expand All @@ -23,7 +24,7 @@
* An operation consists of a list of {@link NormalizedField}s in a parent child hierarchy
*/
@ExperimentalApi
public class NormalizedOperation {
public class NormalizedOperation implements ParsedNormalizedOperation {
private final OperationDefinition.Operation operation;
private final String operationName;
private final List<NormalizedField> rootFields;
Expand Down
1 change: 1 addition & 0 deletions src/test/groovy/graphql/StarWarsData.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -105,6 +105,7 @@ class StarWarsData {
static TypeResolver characterTypeResolver = new TypeResolver() {
@Override
GraphQLObjectType getType(TypeResolutionEnvironment env) {
env.getSelectionSet().getFields() // Used to validate instrumentation
def id = env.getObject().id
if (humanData[id] != null)
return StarWarsSchema.humanType
Expand Down
6 changes: 6 additions & 0 deletions src/test/groovy/graphql/execution/instrumentation/ChainedInstrumentationStateTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,8 @@ class ChainedInstrumentationStateTest extends Specification {
"start:fetch-hero",
"end:fetch-hero",
"start:complete-hero",
"start:parsed-normalized-operation",
"end:parsed-normalized-operation",

"start:execute-object",

Expand Down Expand Up @@ -143,6 +145,8 @@ class ChainedInstrumentationStateTest extends Specification {
"start:fetch-hero",
"end:fetch-hero",
"start:complete-hero",
"start:parsed-normalized-operation",
"end:parsed-normalized-operation",

"start:execute-object",

Expand Down Expand Up @@ -183,6 +187,8 @@ class ChainedInstrumentationStateTest extends Specification {
"start:fetch-hero",
"end:fetch-hero",
"start:complete-hero",
"start:parsed-normalized-operation",
"end:parsed-normalized-operation",

"start:execute-object",

Expand Down
2 changes: 2 additions & 0 deletions src/test/groovy/graphql/execution/instrumentation/InstrumentationTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -312,6 +312,8 @@ class InstrumentationTest extends Specification {
"onDispatched:fetch-hero",
"end:fetch-hero",
"start:complete-hero",
"start:parsed-normalized-operation",
"end:parsed-normalized-operation",
"start:execute-object",
"start:field-id",
"start:fetch-id",
Expand Down
9 changes: 9 additions & 0 deletions src/test/groovy/graphql/execution/instrumentation/ModernTestingInstrumentation.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,11 @@ import graphql.execution.instrumentation.parameters.InstrumentationExecutionStra
import graphql.execution.instrumentation.parameters.InstrumentationFieldCompleteParameters
import graphql.execution.instrumentation.parameters.InstrumentationFieldFetchParameters
import graphql.execution.instrumentation.parameters.InstrumentationFieldParameters
import graphql.execution.instrumentation.parameters.InstrumentationParsedNormalizedOperation
import graphql.execution.instrumentation.parameters.InstrumentationValidationParameters
import graphql.language.Document
import graphql.normalized.ExecutableNormalizedOperation
import graphql.normalized.ParsedNormalizedOperation
import graphql.schema.DataFetcher
import graphql.schema.DataFetchingEnvironment
import graphql.schema.GraphQLSchema
Expand Down Expand Up @@ -73,6 +76,12 @@ class ModernTestingInstrumentation implements Instrumentation {
return new TestingInstrumentContext("execute-operation", executionList, throwableList, useOnDispatch)
}

@Override
InstrumentationContext<ParsedNormalizedOperation> beginParsedNormalizedOperation(InstrumentationParsedNormalizedOperation parameters, InstrumentationState state) {
assert state == instrumentationState
return new TestingInstrumentContext("parsed-normalized-operation", executionList, throwableList, useOnDispatch)
}

@Override
InstrumentationContext<ExecutionResult> beginSubscribedFieldEvent(InstrumentationFieldParameters parameters, InstrumentationState state) {
assert state == instrumentationState
Expand Down
9 changes: 9 additions & 0 deletions src/test/groovy/graphql/execution/instrumentation/NamedInstrumentation.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,8 +7,11 @@ import graphql.execution.instrumentation.parameters.InstrumentationExecutionPara
import graphql.execution.instrumentation.parameters.InstrumentationExecutionStrategyParameters
import graphql.execution.instrumentation.parameters.InstrumentationFieldFetchParameters
import graphql.execution.instrumentation.parameters.InstrumentationFieldParameters
import graphql.execution.instrumentation.parameters.InstrumentationParsedNormalizedOperation
import graphql.execution.instrumentation.parameters.InstrumentationValidationParameters
import graphql.language.Document
import graphql.normalized.ExecutableNormalizedOperation
import graphql.normalized.ParsedNormalizedOperation
import graphql.schema.DataFetcher
import graphql.validation.ValidationError

Expand Down Expand Up @@ -72,6 +75,12 @@ class NamedInstrumentation extends ModernTestingInstrumentation {
return super.beginFieldExecution(parameters, state)
}

@Override
InstrumentationContext<ParsedNormalizedOperation> beginParsedNormalizedOperation(InstrumentationParsedNormalizedOperation parameters, InstrumentationState state) {
assertState(state)
return super.beginParsedNormalizedOperation(parameters, state)
}

@Override
InstrumentationContext<Object> beginFieldFetch(InstrumentationFieldFetchParameters parameters, InstrumentationState state) {
assertState(state)
Expand Down
1 change: 1 addition & 0 deletions src/test/groovy/graphql/execution/instrumentation/NoContextChainedInstrumentationTest.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ class NoContextChainedInstrumentationTest extends Specification {
"start:field-hero",
"start:fetch-hero",
"start:complete-hero",
"start:parsed-normalized-operation",

"start:execute-object",

Expand Down