Support for DataFetcherResult values in subscriptions and better instrumentation. #1804

bbakerman · 2020-02-28T21:40:52Z

This supports BOTH the Publisher being in a DFR and also any streamed events being in DFRs

It also adds error support correctly for sending back a stream of possible errors and data as per spec

It also adds a new Instrumentation callback for each "reactive stream event"

This supports BOTH the Publisher being in a DFR and also any streamed events being in DFRs It also adds error support correctly for sending back a stream of possible errors and data as per spec

bbakerman · 2020-02-28T21:41:22Z

src/main/java/graphql/GraphqlErrorBuilder.java

+        @Override
+        public String toString() {
+            return message;
+        }


helps with debugging

bbakerman · 2020-02-28T22:38:23Z

src/main/java/graphql/execution/ExecutionContextBuilder.java

+    public ExecutionContextBuilder resetErrors() {
+        this.errors.clear();
+        return this;
+    }


we need this because in subscroptions we start a new execution per streamed event

bbakerman · 2020-02-28T22:38:53Z

src/main/java/graphql/execution/SubscriptionExecutionStrategy.java

+        ExecutionContext newExecutionContext = executionContext.transform(builder -> builder
+                .root(eventPayload)
+                .resetErrors()
+        );


We reset errors so that we have errors per streamed event BUT we use the same other execution context

bbakerman · 2020-02-28T22:39:14Z

src/main/java/graphql/execution/SubscriptionExecutionStrategy.java

+        FetchedValue fetchedValue = unboxPossibleDataFetcherResult(newExecutionContext, parameters, eventPayload);
+        FieldValueInfo fieldValueInfo = completeField(newExecutionContext, newParameters, fetchedValue);
+        return fieldValueInfo
+                .getFieldValue()


The fix here was to called unboxPossibleDataFetcherResult

bbakerman · 2020-02-28T22:40:09Z

src/test/groovy/graphql/execution/SubscriptionExecutionStrategyTest.groovy

+            }
+            assert message.data == ["newMessage": [sender: "sender" + i, text: "text" + i]]
+        }
+    }


A test of DataFectherResult + errors at the Publisher level and also at each streamed event level and also errors on sub levels

bbakerman · 2020-02-29T22:54:13Z

src/main/java/graphql/execution/Execution.java

@@ -96,6 +96,7 @@ public Execution(ExecutionStrategy queryStrategy, ExecutionStrategy mutationStra
                .cacheControl(executionInput.getCacheControl())
                .locale(executionInput.getLocale())
                .valueUnboxer(valueUnboxer)
+                .executionInput(executionInput)


We need this later in subscriptions and the instrumentDataFetcherResult calls. Its nominally useful as well but execution context unpacks it any way

bbakerman · 2020-02-29T22:54:46Z

src/main/java/graphql/execution/ExecutionContext.java

+    private final ExecutionInput executionInput;
+
+    @Internal
+    ExecutionContext(ExecutionContextBuilder builder) {


After 18 parameters its time for this pattern

bbakerman · 2020-02-29T22:56:51Z

src/main/java/graphql/execution/SubscriptionExecutionStrategy.java

+        Instrumentation instrumentation = executionContext.getInstrumentation();
+        InstrumentationExecutionStrategyParameters instrumentationParameters = new InstrumentationExecutionStrategyParameters(executionContext, parameters);
+        ExecutionStrategyInstrumentationContext executionStrategyCtx = instrumentation.beginExecutionStrategy(instrumentationParameters);
+


Better instrumentation of subscription queries

bbakerman · 2020-02-29T22:58:24Z

src/main/java/graphql/execution/SubscriptionExecutionStrategy.java

+        GraphQLObjectType parentType = (GraphQLObjectType) parameters.getExecutionStepInfo().getUnwrappedNonNullType();
+        GraphQLFieldDefinition fieldDef = getFieldDef(executionContext.getGraphQLSchema(), parentType, field);
+        return createExecutionStepInfo(executionContext, parameters, fieldDef, parentType);
+    }


This is weird but needed. We want a execution step info here for the instrumentation - the soon to be called completeValue call will do something similar soon but we need this to descrive the "beginSubscribedFieldEvent" in instrumentation terms

bbakerman · 2020-02-29T22:59:07Z

src/test/groovy/graphql/execution/ExecutionStrategyTest.groovy

+                .locale(Locale.getDefault())
+                .valueUnboxer(ValueUnboxer.DEFAULT)
+
+        new ExecutionContext(builder)


code fix up of change in constructor

bbakerman · 2020-02-29T22:59:47Z

src/test/groovy/graphql/execution/pubsub/ReactiveStreamsObjectPublisher.java

+        };
+    }
+
+}


A reactive stream thats a little more generic

felipe-gdr · 2020-03-05T06:31:17Z

src/main/java/graphql/execution/ExecutionContextBuilder.java

@@ -84,6 +85,7 @@ public ExecutionContextBuilder() {
        locale = other.getLocale();
        errors = new ArrayList<>(other.getErrors());
        valueUnboxer = other.getValueUnboxer();
+        this.executionInput = other.getExecutionInput();


nit: use this on other assignments for consistency

felipe-gdr · 2020-03-05T06:33:20Z

src/main/java/graphql/execution/instrumentation/Instrumentation.java

@@ -114,6 +114,17 @@ default InstrumentationState createState(InstrumentationCreateStateParameters pa
     */
    DeferredFieldInstrumentationContext beginDeferredField(InstrumentationDeferredFieldParameters parameters);

+    /**
+     * This is called each time a subscription field produces a new reactive stream event value and its needs to be mapped over via the graphql field subselection.


nit: typo "and its needs to be mapped" should be "and it needs to be mapped"

felipe-gdr · 2020-03-05T06:34:50Z

src/test/groovy/graphql/execution/pubsub/ReactiveStreamsObjectPublisher.java

+import java.util.function.Function;
+
+/**
+ * This example publisher will create count "objects" and then terminate. Its


typo: "Its uses" should be "It uses"

…tcher-result-in-subscription # Conflicts: # src/main/java/graphql/execution/ExecutionContext.java # src/main/java/graphql/execution/ExecutionContextBuilder.java # src/test/groovy/graphql/execution/ExecutionStrategyTest.groovy

In graphql-java 15, changes were made to SubscriptionExecutionStrategy to allow for DataFetcherResults to be returned, and to instrument the results of a subscription properly. This updates the Flow strategy with the same changes and adds tests for both changes. See graphql-java/graphql-java#1804

…oup#742) In graphql-java 15, changes were made to SubscriptionExecutionStrategy to allow for DataFetcherResults to be returned, and to instrument the results of a subscription properly. This updates the Flow strategy with the same changes and adds tests for both changes. See graphql-java/graphql-java#1804

Support for DataFetcherResult values in subscriptions.

a415bde

This supports BOTH the Publisher being in a DFR and also any streamed events being in DFRs It also adds error support correctly for sending back a stream of possible errors and data as per spec

bbakerman added this to the 15.0 milestone Feb 28, 2020

bbakerman requested a review from andimarek February 28, 2020 21:41

bbakerman commented Feb 28, 2020

View reviewed changes

src/main/java/graphql/GraphqlErrorBuilder.java

@Override

public String toString() {

return message;

}

Copy link

Member Author

bbakerman Feb 28, 2020

Choose a reason for hiding this comment

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

helps with debugging

bbakerman commented Feb 28, 2020

View reviewed changes

bbakerman added 2 commits February 29, 2020 18:36

Made sure local context works as expected

8e7bf3b

Added on subscribed field instrumentation

14c1808

bbakerman mentioned this pull request Feb 29, 2020

Have subscriptions apply result instrumentation #1789

Closed

bbakerman requested review from danieltreacy, felipe-gdr and JaidenAshmore February 29, 2020 22:53

bbakerman commented Feb 29, 2020

View reviewed changes

Added on subscribed field instrumentation

2e03573

bbakerman commented Feb 29, 2020

View reviewed changes

bbakerman changed the title ~~Support for DataFetcherResult values in subscriptions.~~ Support for DataFetcherResult values in subscriptions and better instrumentation. Feb 29, 2020

felipe-gdr approved these changes Mar 5, 2020

View reviewed changes

bbakerman merged commit 9eb260e into master May 21, 2020

josephlbarnett mentioned this pull request Jun 3, 2020

Port graphql-java 15 subscription changes to Flow strategy ExpediaGroup/graphql-kotlin#742

Merged

andimarek deleted the 1795-support-datafetcher-result-in-subscription branch June 5, 2020 10:44

smyrick mentioned this pull request Jun 30, 2020

Support returning DataFetcherResult from subscriptions #1795

Closed

dushaniw mentioned this pull request Nov 23, 2021

Error while anlayzing query complexity for GraphQL subscriptions : Data fetcher must return a Publisher of events when using graphql subscriptions wso2/product-apim#12044

Closed

dondonz mentioned this pull request Jan 6, 2024

Instrumentations and subscriptions #1568

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support for DataFetcherResult values in subscriptions and better instrumentation. #1804

Support for DataFetcherResult values in subscriptions and better instrumentation. #1804

bbakerman commented Feb 28, 2020 •

edited

bbakerman Feb 28, 2020

bbakerman Feb 28, 2020

bbakerman Feb 28, 2020

bbakerman Feb 28, 2020

bbakerman Feb 28, 2020

bbakerman Feb 29, 2020

bbakerman Feb 29, 2020

bbakerman Feb 29, 2020

bbakerman Feb 29, 2020

bbakerman Feb 29, 2020

bbakerman Feb 29, 2020

felipe-gdr Mar 5, 2020

felipe-gdr Mar 5, 2020

felipe-gdr Mar 5, 2020

+                      };
+                  }
+              }

Support for DataFetcherResult values in subscriptions and better instrumentation. #1804

Support for DataFetcherResult values in subscriptions and better instrumentation. #1804

Conversation

bbakerman commented Feb 28, 2020 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bbakerman commented Feb 28, 2020 •

edited