Adding GraphQlContext in preference to the legacy context #2368

bbakerman · 2021-06-03T06:42:23Z

The original graphql-java code copied the graphql-js pattern of having a opaque context object

Since everything in JS is untyped and mutable this works because one can just "wack" another property into the context

However we have learnt over time that since the context is sa shared place (for both frameworks and user code to use) we need to have a more formal mechanism that just a Object getContext() pattern.

So we have made the GraphqlContext getGraphqlContext() the preferred context mechanism and deprecated the older Object getContext methods.

At some point the legacy getContext will disappear but since this is a WIDELY used mechanism that will not be any time soon.

andimarek · 2021-06-03T06:53:20Z

cc @bclozel @rstoyanchev @rwinch
This is the more specific context we talked about. Would love to get your input. Thanks

bbakerman · 2021-06-05T04:34:49Z

Related to #2357

rstoyanchev

LGTM overall. A few comments below.

rstoyanchev · 2021-06-09T09:18:04Z

src/main/java/graphql/ExecutionInput.java

@@ -187,6 +206,7 @@ public static Builder newExecutionInput(String query) {
        private String query;
        private String operationName;
        private Object context = GraphQLContext.newContext().build();
+        private GraphQLContext graphQLContext = GraphQLContext.newContext().build();


Given that context is also an instance of GraphQLContext by default, perhaps the new graphQLContext can be initialized to the same instance, so that the new getGraphQLContext simply exposes the existing default context instance but for its actual type.

This can also be helpful for migration in cases where the existing context is not replaced since code can be switched to simply access it through the new graphQLContext methods. And it shouldn't change anything for cases where the existing context is replaced.

Great idea - thanks

rstoyanchev · 2021-06-09T09:26:45Z

src/main/java/graphql/ExecutionInput.java

+        public Builder graphQLContext(GraphQLContext.Builder contextBuilder) {
+            return graphQLContext(contextBuilder.build());
+        }
+


I'm wondering if there is even a need to replace the GraphQLContext instance? A single instance that's not exposed for replacement seems fine for a shared context, or at least it could start out that way until there is evidence to allow it. I cannot think of reasons to replace it but if such reasons do surface, it could also lead to enhancingGraphQLContext itself rather than allowing the instance to be replaced.

This is about trying to seed it up with information from outside the graphql query run

Eg imagine some how you now the principal of the user making the request at the time - you can set this in via this.

Of course you could do

ExecutionInput ei - ExecutionInput.newExecutionInput().query(s).variables(..).build() ei.getGraphqlContext().put(Principal.class, requestPrincipal)

I do see how key-value pairs can be inserted into the context through an:

ExecutionInput instance via getGraphQLContext

ExecutionInput.Builder instance via graphQLContext(UnaryOperator<GraphQLContext.Builder>) although this one currently replaces the underlying context instance rather than transforming it (as per my next comment) so doesn't quite work for just seeding (without replacing) the context.

What I don't see is why even allow replacing the GraphQLContext instance? If all you want to seed it, that should be possible with a single, shared instance only that isn't exposed for replacing.

gerat idea - done - goes into the one and only context

rstoyanchev · 2021-06-09T09:30:51Z

src/main/java/graphql/ExecutionInput.java

+            GraphQLContext.Builder builder = GraphQLContext.newContext();
+            builder = contextBuilderFunction.apply(builder);
+            return graphQLContext(builder.build());
+        }


Irrespective of my previous comment about whether graphQLContext is a fixed instance that can or cannot be replaced, I think this method should operate on the default instance. I can imagine cases where some code only has access to GraphQLContext.Builder and needs to update the context without actually replacing the instance.

This comment does not match any more

…lues are copied into the known instance

rstoyanchev · 2021-06-11T07:42:05Z

src/main/java/graphql/ExecutionInput.java

+        }
+
+        /**
+         * This will put all the valuyes from the map into the underlying {@link GraphQLContext} of this execution input


Suggested change

* This will put all the valuyes from the map into the underlying {@link GraphQLContext} of this execution input

* This will put all the values from the map into the underlying {@link GraphQLContext} of this execution input

yeah I think this is doing what you want - its a new context here but that is "copied / putAll" into the one context object

rstoyanchev · 2021-06-11T07:44:51Z

src/main/java/graphql/ExecutionInput.java

+         * @return this builder
+         */
+        public Builder graphQLContext(Consumer<GraphQLContext.Builder> builderFunction) {
+            GraphQLContext.Builder builder = GraphQLContext.newContext();


I think this should operate on the underlying GraphQLContext instance rather than creating a new one, right? It would be consistent with the changes we've been discussing and I think it's meant to be that way judging from the Javadoc.

Same here - this is putAll into the one context object

paulbakker · 2021-06-28T18:37:10Z

Would it be possible to make this class extendable?

In the DGS framework we have a DgsContext, which is very similar in concept to the GraphQLContext. We have a mechanism in the framework to create custom context, which is accessible through the DgsContext.

If DgsContext can extend GraphQLContext, we can get DGS users onto GraphQLContext without breaking existing code. This would also make the integration between DGS and spring-graphql better - this is one of the issues I ran into because spring-graphql assumes GraphQLContext.

bbakerman · 2021-06-30T07:53:05Z

@paulbakker

Yeah this makes sense to me - GraphQLContext probably should have been an interface in retrospect (said everyone in Java) but allowing it to extended is the next best thing.

you won't get the fancy builders however because UNLESS you know of a tricky way there is no Java way to inherit static builder methods which is a PITA

bbakerman · 2021-06-30T07:57:26Z

@paulbakker I have made it protected extendable

rstoyanchev · 2021-06-30T13:26:57Z

I think it's important to emphasize the changes in this PR provide a migration path for existing APIs that currently rely on a custom object.

Existing usage of the context methods will continue to work next to the new graphQlContext methods which will return a separate instance. That way existing applications and frameworks can support both existing and new ways of accessing the context at the same time.

The idea is the new graphQlContext method becomes a fixed, top-level container that multiple frameworks or components can share. The existing context methods will just be depreciated so existing users can migrate gradually.

If this is all used in this way, I imagine there shouldn't be a need to extend GraphQLContext which is something that only one party or framework can do.

bbakerman · 2021-06-30T22:54:36Z

@rstoyanchev - you right - there is no need for extension because long term we dont allow you to actually set a GraphqlContext in.

@paulbakker - today there is two context objects (transitonary) - one is the old context that was an opaque type with no contract

The second is the new graphqlContext which is a glorified map but has a fixed contract as a map like store. So frameworks and code can graphqlcontext.get("com.netflix.dgs.whatever.key) to get a value you know should be there because your framework put it there.

Since longer term you don't SET the context object but rather set values into a fixed context value (we create a graphql context per Executionnput) then there is no need for extension

I have reverted that change back - it is still fixed.

Closes vert-x3#2023 With GraphQL-Java 17, te GraphQLContext object becomes the standard to share contextual data between components of a GraphQL Java application (see https://github.com/graphql-java/graphql-java/releases/tag/v17.0 and graphql-java/graphql-java#2368). This commit: - deprecates the current context hooks in Vert.x Web GraphQL handlers - adds a new hook that allows to manipulate the ExecutionInputBuilder - updates the docs Signed-off-by: Thomas Segismont <tsegismont@gmail.com>

Closes #2023 With GraphQL-Java 17, te GraphQLContext object becomes the standard to share contextual data between components of a GraphQL Java application (see https://github.com/graphql-java/graphql-java/releases/tag/v17.0 and graphql-java/graphql-java#2368). This commit: - deprecates the current context hooks in Vert.x Web GraphQL handlers - adds a new hook that allows to manipulate the ExecutionInputBuilder - updates the docs Signed-off-by: Thomas Segismont <tsegismont@gmail.com>

…ts` utility (#4201) Motivation: The context field of ExecutionInput/DataFetchingEnvironment is a deprecated method. This should be replaced. - See https://github.com/graphql-java/graphql-java/releases/tag/v17.0 - See graphql-java/graphql-java#2368 Modifications: - Added `ServiceRequestContex`t to `GraphQLContext` - Added `GraphqlServiceContexts` utility class for retrieving `ServiceRequestContext` from a `GraphQLContext`. Result: - `armeria-graphql` doesn't use deprecated API anymore. - A user can retrieve the current `ServiceRequestContext` using `GraphServiceContexts`. Co-authored-by: Trustin Lee <t@motd.kr> Co-authored-by: Ikhun Um <ih.pert@gmail.com>

Adding GraphQlContext in preference to the legacy context

bc27157

andimarek added this to the 17.0 milestone Jun 3, 2021

rstoyanchev reviewed Jun 9, 2021

View reviewed changes

bbakerman added 3 commits June 11, 2021 11:40

made the GraphQLContext not be settable into ExecutionInput

f8fdb49

You can now change the context during ExecutionInput build but the va…

b471852

…lues are copied into the known instance

Now with methods on ExecutionInput that allows you to add to context

d4eb13c

rstoyanchev reviewed Jun 11, 2021

View reviewed changes

spillin misteak

ce64d9c

Make GraphqlContext extendable

07d79d1

Make GraphqlContext not extendable

e146136

andimarek approved these changes Jul 5, 2021

View reviewed changes

bbakerman merged commit 97855c6 into master Jul 5, 2021

tsegismont mentioned this pull request Aug 16, 2021

Context management with GraphQLContext object vert-x3/vertx-web#2023

Closed

oryan-block mentioned this pull request Aug 17, 2021

Update custom context class option to be compatible with new GraphQLContext class graphql-java-kickstart/graphql-java-tools#566

Merged

2 tasks

tsegismont mentioned this pull request Sep 15, 2021

Context management with GraphQLContext object vert-x3/vertx-web#2043

Merged

hrkfdn mentioned this pull request Oct 22, 2021

Support graphql-java GraphQLContext injection ExpediaGroup/graphql-kotlin#1277

Closed

heowc mentioned this pull request Apr 11, 2022

Fix deprecated graphql-java API usage and add GraphqlServiceContexts utility line/armeria#4201

Merged

theJC mentioned this pull request Apr 24, 2022

feature request: Support multiple registered GraphQLContext contributors, allowing them to provide GraphQLContext key/value entries Netflix/dgs-framework#1006

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding GraphQlContext in preference to the legacy context #2368

Adding GraphQlContext in preference to the legacy context #2368

bbakerman commented Jun 3, 2021

andimarek commented Jun 3, 2021

bbakerman commented Jun 5, 2021

rstoyanchev left a comment

rstoyanchev Jun 9, 2021 •

edited

bbakerman Jun 9, 2021

bbakerman Jun 11, 2021

rstoyanchev Jun 9, 2021 •

edited

bbakerman Jun 9, 2021

rstoyanchev Jun 10, 2021 •

edited

bbakerman Jun 11, 2021

rstoyanchev Jun 9, 2021 •

edited

bbakerman Jun 11, 2021

rstoyanchev Jun 11, 2021

bbakerman Jun 21, 2021

rstoyanchev Jun 11, 2021

bbakerman Jun 21, 2021

paulbakker commented Jun 28, 2021

bbakerman commented Jun 30, 2021

bbakerman commented Jun 30, 2021

rstoyanchev commented Jun 30, 2021

bbakerman commented Jun 30, 2021

	* This will put all the valuyes from the map into the underlying {@link GraphQLContext} of this execution input
	* This will put all the values from the map into the underlying {@link GraphQLContext} of this execution input

Adding GraphQlContext in preference to the legacy context #2368

Adding GraphQlContext in preference to the legacy context #2368

Conversation

bbakerman commented Jun 3, 2021

andimarek commented Jun 3, 2021

bbakerman commented Jun 5, 2021

rstoyanchev left a comment

Choose a reason for hiding this comment

rstoyanchev Jun 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rstoyanchev Jun 9, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rstoyanchev Jun 10, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rstoyanchev Jun 9, 2021 • 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

paulbakker commented Jun 28, 2021

bbakerman commented Jun 30, 2021

bbakerman commented Jun 30, 2021

rstoyanchev commented Jun 30, 2021

bbakerman commented Jun 30, 2021

rstoyanchev Jun 9, 2021 •

edited

rstoyanchev Jun 9, 2021 •

edited

rstoyanchev Jun 10, 2021 •

edited

rstoyanchev Jun 9, 2021 •

edited