Skip to content

Remove FetchedValue from public API #3922

@bbakerman

Description

@bbakerman

The graphql.execution.FetchedValue is a wrapper object for "data fetched values" that are then passed around in the engine

Its shape is like this

@PublicApi
public class FetchedValue {
    private final Object fetchedValue;
    private final Object localContext;
    private final ImmutableList<GraphQLError> errors;

That is its the actual fetched object and if the fetched result is a graphql.execution.DataFetcherResult then the errors and local context are placed into the wrapper object.

The thing is this is never actually used by the graphql engine for anything other than the private final Object fetchedValue aspect, eg the unboxed fetched value.

So we could avoid a MAJOR allocation here by NOT wrapping values in a graphql.execution.FetchedValue

Every value that comes via a DataFetcher is wrapped today in a graphql.execution.FetchedValue

This means we do an allocation for every value returned which is mostly pointless.

The wrinkle here in getting ride of this this

  • ExecutionStrategy has a protected DUCK type method
    @DuckTyped(shape = "CompletableFuture<FetchedValue> | FetchedValue")
    protected Object fetchField(ExecutionContext executionContext, ExecutionStrategyParameters parameters) {

It is either a promise or a materialised FetchedValue here.

  • graphql.execution.instrumentation.parameters.InstrumentationFieldCompleteParameters#getFetchedValue returns an object that is assumed to be a FetchedValue
    public Object getFetchedValue() {
        return fetchedValue;
    }

This is actually declared as Object but it was always FetchedValue so nominally it might have the errors and localcontext on it. This Instrumentation is called BEFORE we complete a value.

IF we got rid of FetchedValue then this be a semantic breaking change to the Instrumentation - ie its not an API breaking change but it is a semantic one.

Brads view

I think that the fetchField method change is ok - we dont really want ExecutionStrategy to be API even if it is.

I think the Instrumentation semantic breaking change is more impactful BUT if we can avoid a double allocation for EVERY value we ever return then I think this will be worth the trade off

Metadata

Metadata

Assignees

No one assigned

    Labels

    Stalekeep-openTells Stale Bot to keep PRs and issues open

    Type

    No type

    Projects

    No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions