Describe the Feature Request

Today each value that comes from a field, be it object, list, scalar, enum or even null is wrapped in a ExecutionResult

This means that for every field value another object is created that holds this value and then at some later staging this ExecutionResult is unwrapped and put back into a fields object map and a final ExecutionResult is produced for ending operation.

The cost

This does not seem to add time to operations (allocating objects is fast in java) but it does increase memory pressure and could induce more garbage collection depending on whether the ExecutionResult make it into an old GC generation or not.

Why is this

I suspect this was done for 2 reasons. The first is that the original graphql-js implementation (we graphql-java inspired its execution from originally) used a callback from ExecutionStrategy.execute to implement the "completeObject" when a field returns a object value. Every object field results in a recursive chain of calls and ExecutionStrategy.execute returns a ExecutionResult

Also a ExecutionResult can hold both data and errors and while in practice this is NOT used (not really) for queries and mutations, it is used for subscriptions.

Each value published to a subscription stream is in fact a ExecutionResult with adta and possible errors.

(This is actually error prone error handling - which I can explain later - it does contain errors but its buggy really)

Less than great code

The current creation of ExecutionResult does the following

return completedFuture(new ExecutionResultImpl(serialized, executionContext.getErrors()));

That is it wraps the value and then throws in the current set of errors into the ExecutionResult However later field map building code just ignores the errors and puts the executionContext.getErrors() into the very final result ExecutionResult object.

So really these error capturing here is pointless for queries and mutations - its never used.

However there is one case where it is and that's in scubscriptions.

Subscriptions are a series of completeObject calls for ever ebent passed into the Publisher. So if there are errors they go out in the ExecutionResult into the target Publisher. However this code is less than stellar becauase in theory the error just just grows and grows because its coming from executionContext.getErrors() and hence if more errors are added as events happen, the list of sent errors will just grow.

Subscriptions are in fact NOT like a described above - they create a ExecutionContext per published event and hence the error map is indeed per completed value. And hence is fixable.

Describe Preferred Solution

The preferred solution is to stop the wrapping of values into ExecutionResult objects and unwrapping them on object field map processing.

This means we would need a new ExecutionStrategy methods maybe called executeObject that returns CompleteableFuture<Map<String,Object>> and not a CompleteableFuture<ExecutionResult> like today.

When a object field is being completed, this new method will be called.

Complications

A complication here is graphql.execution.FieldValueInfo which has public CompletableFuture<ExecutionResult> getFieldValue() and also the Instrumentation calls that require a ExecutionResult in them like

ExecutionStrategyInstrumentationContext beginExecutionStrategy(InstrumentationExecutionStrategyParameters parameters, InstrumentationState state)

where:

public interface ExecutionStrategyInstrumentationContext extends InstrumentationContext<ExecutionResult>


InstrumentationContext<ExecutionResult> beginField(InstrumentationFieldParameters parameters, InstrumentationState state)

InstrumentationContext<ExecutionResult> beginFieldComplete(InstrumentationFieldCompleteParameters parameters, InstrumentationState state)

InstrumentationContext<ExecutionResult> beginFieldListComplete(InstrumentationFieldCompleteParameters parameters, InstrumentationState state)

These require a CF<ExecutionResult> but would need to get to a ExecutionResult<Object> or ExecutionResult<List<Object>> for lists

I think the new ExecutionStrategy.executeObject would actually call a new Instrumentation method like

FieldObjectInstrumentationContext beginFieldObjectComplete(InstrumentationExecutionStrategyParameters parameters, InstrumentationState state)

where:

public interface FieldObjectInstrumentationContext extends InstrumentationContext<Map<String,Object>>

The new instrumentation methods could delegate / capture the original CFs so they could MAP to a ExecutionResult if the instrumentation has not moved to the new schema and hence backward compatibility could be maintained.

Breaking Changes

The current ExecutionStrategy protected methods would change and this is nominally public API

But we should in retrospect never made ExecutionStrategy public API because of the case like there where it prevents us from being more efficient in the name of methods that we dont think ever get used.

As an example

    protected CompletableFuture<ExecutionResult> completeValueForScalar(ExecutionContext executionContext, ExecutionStrategyParameters parameters, GraphQLScalarType scalarType, Object result) {


    protected CompletableFuture<ExecutionResult> completeValueForNull(ExecutionContext executionContext, ExecutionStrategyParameters parameters) {


    protected CompletableFuture<ExecutionResult> completeValueForEnum(ExecutionContext executionContext, ExecutionStrategyParameters parameters, GraphQLEnumType enumType, Object result) {


    protected CompletableFuture<ExecutionResult> completeValueForObject(ExecutionContext executionContext, ExecutionStrategyParameters parameters, GraphQLObjectType resolvedObjectType, Object result) {

These need to become Object or List<Object> say and we cant maintain API backwards compatibility here.