Skip to content

Adding GraphQlContext in preference to the legacy context #2368

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

Merged
bbakerman merged 7 commits into from
Jul 5, 2021
Merged

Adding GraphQlContext in preference to the legacy context #2368

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
bc27157
Adding GraphQlContext in preference to the legacy context
bbakerman Jun 3, 2021
f8fdb49
made the GraphQLContext not be settable into ExecutionInput
bbakerman Jun 11, 2021
b471852
You can now change the context during ExecutionInput build but the va…
bbakerman Jun 11, 2021
d4eb13c
Now with methods on ExecutionInput that allows you to add to context
bbakerman Jun 11, 2021
ce64d9c
spillin misteak
bbakerman Jun 21, 2021
07d79d1
Make GraphqlContext extendable
bbakerman Jun 30, 2021
e146136
Make GraphqlContext not extendable
bbakerman Jun 30, 2021
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
85 changes: 83 additions & 2 deletions src/main/java/graphql/ExecutionInput.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,7 @@ public class ExecutionInput {
private final String query;
private final String operationName;
private final Object context;
private final GraphQLContext graphQLContext;
private final Object localContext;
private final Object root;
private final Map<String, Object> variables;
Expand All @@ -36,6 +37,7 @@ private ExecutionInput(Builder builder) {
this.query = assertNotNull(builder.query, () -> "query can't be null");
this.operationName = builder.operationName;
this.context = builder.context;
this.graphQLContext = assertNotNull(builder.graphQLContext);
this.root = builder.root;
this.variables = builder.variables;
this.dataLoaderRegistry = builder.dataLoaderRegistry;
Expand All @@ -61,12 +63,25 @@ public String getOperationName() {
}

/**
* The legacy context object has been deprecated in favour of the more shareable
* {@link #getGraphQLContext()}
*
* @return the context object to pass to all data fetchers
*
* @deprecated - use {@link #getGraphQLContext()}
*/
@Deprecated
public Object getContext() {
return context;
}

/**
* @return the shared {@link GraphQLContext} object to pass to all data fetchers
*/
public GraphQLContext getGraphQLContext() {
return graphQLContext;
}

/**
* @return the local context object to pass to all top level (i.e. query, mutation, subscription) data fetchers
*/
Expand Down Expand Up @@ -130,13 +145,15 @@ public Map<String, Object> getExtensions() {
* the current values and allows you to transform it how you want.
*
* @param builderConsumer the consumer code that will be given a builder to transform
*
* @return a new ExecutionInput object based on calling build on that builder
*/
public ExecutionInput transform(Consumer<Builder> builderConsumer) {
Builder builder = new Builder()
.query(this.query)
.operationName(this.operationName)
.context(this.context)
.transfer(this.graphQLContext)
.localContext(this.localContext)
.root(this.root)
.dataLoaderRegistry(this.dataLoaderRegistry)
Expand All @@ -157,6 +174,7 @@ public String toString() {
"query='" + query + '\'' +
", operationName='" + operationName + '\'' +
", context=" + context +
", graphQLContext=" + graphQLContext +
", root=" + root +
", variables=" + variables +
", dataLoaderRegistry=" + dataLoaderRegistry +
Expand All @@ -176,6 +194,7 @@ public static Builder newExecutionInput() {
* Creates a new builder of ExecutionInput objects with the given query
*
* @param query the query to execute
*
* @return a new builder of ExecutionInput objects
*/
public static Builder newExecutionInput(String query) {
Expand All @@ -186,7 +205,8 @@ public static class Builder {

private String query;
private String operationName;
private Object context = GraphQLContext.newContext().build();
private GraphQLContext graphQLContext = GraphQLContext.newContext().build();
Copy link

@rstoyanchev rstoyanchev Jun 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@bbakerman bbakerman Jun 9, 2021

Choose a reason for hiding this comment

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

Great idea - thanks

Copy link
Member Author

@bbakerman bbakerman Jun 11, 2021

Choose a reason for hiding this comment

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

done

private Object context = graphQLContext; // we make these the same object on purpose - legacy code will get the same object if this change nothing
private Object localContext;
private Object root;
private Map<String, Object> variables = Collections.emptyMap();
Expand Down Expand Up @@ -214,6 +234,7 @@ public Builder operationName(String operationName) {
* A default one will be assigned, but you can set your own.
*
* @param executionId an execution id object
*
* @return this builder
*/
public Builder executionId(ExecutionId executionId) {
Expand All @@ -226,6 +247,7 @@ public Builder executionId(ExecutionId executionId) {
* Sets the locale to use for this operation
*
* @param locale the locale to use
*
* @return this builder
*/
public Builder locale(Locale locale) {
Expand All @@ -237,6 +259,7 @@ public Builder locale(Locale locale) {
* Sets initial localContext in root data fetchers
*
* @param localContext the local context to use
*
* @return this builder
*/
public Builder localContext(Object localContext) {
Expand All @@ -245,27 +268,84 @@ public Builder localContext(Object localContext) {
}

/**
* By default you will get a {@link GraphQLContext} object but you can set your own.
* The legacy context object
*
* @param context the context object to use
*
* @return this builder
*
* @deprecated - the {@link ExecutionInput#getGraphQLContext()} is a fixed mutable instance now
*/
@Deprecated
public Builder context(Object context) {
this.context = context;
return this;
}

/**
* The legacy context object
*
* @param contextBuilder the context builder object to use
*
* @return this builder
*
* @deprecated - the {@link ExecutionInput#getGraphQLContext()} is a fixed mutable instance now
*/
@Deprecated
public Builder context(GraphQLContext.Builder contextBuilder) {
this.context = contextBuilder.build();
return this;
}

/**
* The legacy context object
*
* @param contextBuilderFunction the context builder function to use
*
* @return this builder
*
* @deprecated - the {@link ExecutionInput#getGraphQLContext()} is a fixed mutable instance now
*/
@Deprecated
public Builder context(UnaryOperator<GraphQLContext.Builder> contextBuilderFunction) {
GraphQLContext.Builder builder = GraphQLContext.newContext();
builder = contextBuilderFunction.apply(builder);
return context(builder.build());
}

/**
* This will give you a builder of {@link GraphQLContext} and any values you set will be copied
* into the underlying {@link GraphQLContext} of this execution input
*
* @param builderFunction a builder function you can use to put values into the context
*
* @return this builder
*/
public Builder graphQLContext(Consumer<GraphQLContext.Builder> builderFunction) {
GraphQLContext.Builder builder = GraphQLContext.newContext();
Copy link

@rstoyanchev rstoyanchev Jun 11, 2021

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@bbakerman bbakerman Jun 21, 2021

Choose a reason for hiding this comment

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

Same here - this is putAll into the one context object

builderFunction.accept(builder);
this.graphQLContext.putAll(builder);
return this;
}

/**
* This will put all the values from the map into the underlying {@link GraphQLContext} of this execution input
*
* @param mapOfContext a map of values to put in the context
*
* @return this builder
*/
public Builder graphQLContext(Map<?, Object> mapOfContext) {
this.graphQLContext.putAll(mapOfContext);
return this;
}

Copy link

@rstoyanchev rstoyanchev Jun 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@bbakerman bbakerman Jun 9, 2021

Choose a reason for hiding this comment

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

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)

Copy link

@rstoyanchev rstoyanchev Jun 10, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@bbakerman bbakerman Jun 11, 2021

Choose a reason for hiding this comment

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

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

// hidden on purpose
private Builder transfer(GraphQLContext graphQLContext) {
this.graphQLContext = Assert.assertNotNull(graphQLContext);
return this;
}
Copy link

@rstoyanchev rstoyanchev Jun 9, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

@bbakerman bbakerman Jun 11, 2021

Choose a reason for hiding this comment

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

This comment does not match any more


public Builder root(Object root) {
this.root = root;
return this;
Expand All @@ -287,6 +367,7 @@ public Builder extensions(Map<String, Object> extensions) {
* instances as this will create unexpected results.
*
* @param dataLoaderRegistry a registry of {@link org.dataloader.DataLoader}s
*
* @return this builder
*/
public Builder dataLoaderRegistry(DataLoaderRegistry dataLoaderRegistry) {
Expand Down
2 changes: 1 addition & 1 deletion src/main/java/graphql/GraphQL.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -462,7 +462,7 @@ private ExecutionInput ensureInputHasId(ExecutionInput executionInput) {
}
String queryString = executionInput.getQuery();
String operationName = executionInput.getOperationName();
Object context = executionInput.getContext();
Object context = executionInput.getGraphQLContext();
return executionInput.transform(builder -> builder.executionId(idProvider.provide(queryString, operationName, context)));
}

Expand Down
Loading