Skip to content

Commit a634053

Browse files
bbakermanbbakerman
authored
#1650 - upgrades the dataloader version AND fixes lazy data loader support (#1651)
* #1650 - upgrades the dataloader version AND fixes lazy data loader support * #1650 - wont allow the default one to be used * #1650 - test fix up * #1650 - fixing annoying async test * Pr feedback
1 parent 40db9de commit a634053

File tree

9 files changed

+86
-28
lines changed

9 files changed

+86
-28
lines changed

‎build.gradle‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -53,7 +53,7 @@ jar {
5353
dependencies {
5454
compile 'org.antlr:antlr4-runtime:4.7.2'
5555
compile 'org.slf4j:slf4j-api:' + slf4jVersion
56-
compile 'com.graphql-java:java-dataloader:2.1.1'
56+
compile 'com.graphql-java:java-dataloader:2.2.3'
5757
compile 'org.reactivestreams:reactive-streams:' + reactiveStreamsVersion
5858
antlr "org.antlr:antlr4:4.7.2"
5959
testCompile group: 'junit', name: 'junit', version: '4.12'

‎src/main/java/graphql/ExecutionInput.java‎

Lines changed: 6 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
import graphql.cachecontrol.CacheControl;
44
import graphql.execution.ExecutionId;
5+
import graphql.execution.instrumentation.dataloader.DataLoaderDispatcherInstrumentationState;
56
import org.dataloader.DataLoaderRegistry;
67

78
import java.util.Collections;
@@ -99,7 +100,6 @@ public ExecutionId getExecutionId() {
99100
* the current values and allows you to transform it how you want.
100101
*
101102
* @param builderConsumer the consumer code that will be given a builder to transform
102-
*
103103
* @return a new ExecutionInput object based on calling build on that builder
104104
*/
105105
public ExecutionInput transform(Consumer<Builder> builderConsumer) {
@@ -143,7 +143,6 @@ public static Builder newExecutionInput() {
143143
* Creates a new builder of ExecutionInput objects with the given query
144144
*
145145
* @param query the query to execute
146-
*
147146
* @return a new builder of ExecutionInput objects
148147
*/
149148
public static Builder newExecutionInput(String query) {
@@ -157,7 +156,11 @@ public static class Builder {
157156
private Object context = GraphQLContext.newContext().build();
158157
private Object root;
159158
private Map<String, Object> variables = Collections.emptyMap();
160-
private DataLoaderRegistry dataLoaderRegistry = new DataLoaderRegistry();
159+
//
160+
// this is important - it allows code to later known if we never really set a dataloader and hence it can optimize
161+
// dataloader field tracking away.
162+
//
163+
private DataLoaderRegistry dataLoaderRegistry = DataLoaderDispatcherInstrumentationState.EMPTY_DATALOADER_REGISTRY;
161164
private CacheControl cacheControl = CacheControl.newCacheControl();
162165
private ExecutionId executionId = null;
163166

@@ -175,7 +178,6 @@ public Builder operationName(String operationName) {
175178
* A default one will be assigned, but you can set your own.
176179
*
177180
* @param executionId an execution id object
178-
*
179181
* @return this builder
180182
*/
181183
public Builder executionId(ExecutionId executionId) {
@@ -187,7 +189,6 @@ public Builder executionId(ExecutionId executionId) {
187189
* By default you will get a {@link GraphQLContext} object but you can set your own.
188190
*
189191
* @param context the context object to use
190-
*
191192
* @return this builder
192193
*/
193194
public Builder context(Object context) {
@@ -222,7 +223,6 @@ public Builder variables(Map<String, Object> variables) {
222223
* instances as this will create unexpected results.
223224
*
224225
* @param dataLoaderRegistry a registry of {@link org.dataloader.DataLoader}s
225-
*
226226
* @return this builder
227227
*/
228228
public Builder dataLoaderRegistry(DataLoaderRegistry dataLoaderRegistry) {

‎src/main/java/graphql/Internal.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
import java.lang.annotation.Target;
66

77
import static java.lang.annotation.ElementType.CONSTRUCTOR;
8+
import static java.lang.annotation.ElementType.FIELD;
89
import static java.lang.annotation.ElementType.METHOD;
910
import static java.lang.annotation.ElementType.TYPE;
1011

@@ -15,6 +16,6 @@
1516
* In general unnecessary changes will be avoided but you should not depend on internal classes being stable
1617
*/
1718
@Retention(RetentionPolicy.RUNTIME)
18-
@Target(value = {CONSTRUCTOR, METHOD, TYPE})
19+
@Target(value = {CONSTRUCTOR, METHOD, TYPE, FIELD})
1920
public @interface Internal {
2021
}

‎src/main/java/graphql/PublicApi.java‎

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,7 @@
66
import java.lang.annotation.Target;
77

88
import static java.lang.annotation.ElementType.CONSTRUCTOR;
9+
import static java.lang.annotation.ElementType.FIELD;
910
import static java.lang.annotation.ElementType.METHOD;
1011
import static java.lang.annotation.ElementType.TYPE;
1112

@@ -17,7 +18,7 @@
1718
* maybe be added which would break derivations but not callers.
1819
*/
1920
@Retention(RetentionPolicy.RUNTIME)
20-
@Target(value = {CONSTRUCTOR, METHOD, TYPE})
21+
@Target(value = {CONSTRUCTOR, METHOD, TYPE, FIELD})
2122
@Documented
2223
public @interface PublicApi {
2324
}

‎src/main/java/graphql/execution/instrumentation/dataloader/DataLoaderDispatcherInstrumentationState.java‎

Lines changed: 16 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,9 @@
11
package graphql.execution.instrumentation.dataloader;
22

3+
import graphql.Assert;
4+
import graphql.Internal;
35
import graphql.execution.instrumentation.InstrumentationState;
6+
import org.dataloader.DataLoader;
47
import org.dataloader.DataLoaderRegistry;
58
import org.slf4j.Logger;
69

@@ -9,6 +12,14 @@
912
*/
1013
public class DataLoaderDispatcherInstrumentationState implements InstrumentationState {
1114

15+
@Internal
16+
public static final DataLoaderRegistry EMPTY_DATALOADER_REGISTRY = new DataLoaderRegistry() {
17+
@Override
18+
public DataLoaderRegistry register(String key, DataLoader<?, ?> dataLoader) {
19+
return Assert.assertShouldNeverHappen("You MUST set in your own DataLoaderRegistry to use data loader");
20+
}
21+
};
22+
1223
private final FieldLevelTrackingApproach approach;
1324
private final DataLoaderRegistry dataLoaderRegistry;
1425
private final InstrumentationState state;
@@ -20,8 +31,11 @@ public DataLoaderDispatcherInstrumentationState(Logger log, DataLoaderRegistry d
2031
this.dataLoaderRegistry = dataLoaderRegistry;
2132
this.approach = new FieldLevelTrackingApproach(log, dataLoaderRegistry);
2233
this.state = approach.createState();
23-
hasNoDataLoaders = dataLoaderRegistry.getKeys().isEmpty();
24-
34+
//
35+
// if they have never set a dataloader into the execution input then we can optimize
36+
// away the tracking code
37+
//
38+
hasNoDataLoaders = dataLoaderRegistry == EMPTY_DATALOADER_REGISTRY;
2539
}
2640

2741
boolean isAggressivelyBatching() {
@@ -47,6 +61,4 @@ boolean hasNoDataLoaders() {
4761
InstrumentationState getState() {
4862
return state;
4963
}
50-
51-
5264
}

‎src/main/java/graphql/schema/DataFetchingEnvironment.java‎

Lines changed: 9 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import graphql.language.FragmentDefinition;
1212
import graphql.language.OperationDefinition;
1313
import org.dataloader.DataLoader;
14+
import org.dataloader.DataLoaderRegistry;
1415

1516
import java.util.List;
1617
import java.util.Map;
@@ -31,7 +32,6 @@ public interface DataFetchingEnvironment {
3132
* For the root query, it is equal to {{@link DataFetchingEnvironment#getRoot}
3233
*
3334
* @param <T> you decide what type it is
34-
*
3535
* @return can be null for the root query, otherwise it is never null
3636
*/
3737
<T> T getSource();
@@ -45,7 +45,6 @@ public interface DataFetchingEnvironment {
4545
* Returns true of the named argument is present
4646
*
4747
* @param name the name of the argument
48-
*
4948
* @return true of the named argument is present
5049
*/
5150
boolean containsArgument(String name);
@@ -72,13 +71,12 @@ public interface DataFetchingEnvironment {
7271
<T> T getArgumentOrDefault(String name, T defaultValue);
7372

7473
/**
75-
* Returns a context argument that is set up when the {@link graphql.GraphQL#execute} method
74+
* Returns a context argument that is set up when the {@link graphql.GraphQL#execute(graphql.ExecutionInput)} )} method
7675
* is invoked.
7776
* <p>
7877
* This is a info object which is provided to all DataFetchers, but never used by graphql-java itself.
7978
*
8079
* @param <T> you decide what type it is
81-
*
8280
* @return can be null
8381
*/
8482
<T> T getContext();
@@ -94,7 +92,6 @@ public interface DataFetchingEnvironment {
9492
* If the field is a top level field then 'localContext' equals the global 'context'
9593
*
9694
* @param <T> you decide what type it is
97-
*
9895
* @return can be null if no field context objects are passed back by previous parent fields
9996
*/
10097
<T> T getLocalContext();
@@ -103,7 +100,6 @@ public interface DataFetchingEnvironment {
103100
* This is the source object for the root query.
104101
*
105102
* @param <T> you decide what type it is
106-
*
107103
* @return can be null
108104
*/
109105
<T> T getRoot();
@@ -116,7 +112,6 @@ public interface DataFetchingEnvironment {
116112

117113
/**
118114
* @return the list of fields
119-
*
120115
* @deprecated Use {@link #getMergedField()}.
121116
*/
122117
@Deprecated
@@ -127,9 +122,9 @@ public interface DataFetchingEnvironment {
127122
* are querying the same data. If this is the case they get merged
128123
* together and fetched only once, but this method returns all of the Fields
129124
* from the query.
130-
*
125+
* <p>
131126
* Most of the time you probably want to use {@link #getField()}.
132-
*
127+
* <p>
133128
* Example query with more than one Field returned:
134129
*
135130
* <pre>
@@ -195,7 +190,6 @@ public interface DataFetchingEnvironment {
195190
* This gives you access to the directives related to this field
196191
*
197192
* @return the {@link graphql.execution.directives.QueryDirectives} for the currently executing field
198-
*
199193
* @see graphql.execution.directives.QueryDirectives for more information
200194
*/
201195
QueryDirectives getQueryDirectives();
@@ -206,14 +200,16 @@ public interface DataFetchingEnvironment {
206200
* @param dataLoaderName the name of the data loader to fetch
207201
* @param <K> the key type
208202
* @param <V> the value type
209-
*
210203
* @return the named data loader or null
211-
*
212-
* @see graphql.execution.ExecutionContext#getDataLoaderRegistry()
213204
* @see org.dataloader.DataLoaderRegistry#getDataLoader(String)
214205
*/
215206
<K, V> DataLoader<K, V> getDataLoader(String dataLoaderName);
216207

208+
/**
209+
* @return the {@link org.dataloader.DataLoaderRegistry} in play
210+
*/
211+
DataLoaderRegistry getDataLoaderRegistry();
212+
217213
/**
218214
* @return the current {@link CacheControl} instance used to add cache hints to the response
219215
*/

‎src/main/java/graphql/schema/DataFetchingEnvironmentImpl.java‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -171,6 +171,11 @@ public <K, V> DataLoader<K, V> getDataLoader(String dataLoaderName) {
171171
return dataLoaderRegistry.getDataLoader(dataLoaderName);
172172
}
173173

174+
@Override
175+
public DataLoaderRegistry getDataLoaderRegistry() {
176+
return dataLoaderRegistry;
177+
}
178+
174179
@Override
175180
public CacheControl getCacheControl() {
176181
return cacheControl;

‎src/main/java/graphql/schema/DelegatingDataFetchingEnvironment.java‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@
1111
import graphql.language.FragmentDefinition;
1212
import graphql.language.OperationDefinition;
1313
import org.dataloader.DataLoader;
14+
import org.dataloader.DataLoaderRegistry;
1415

1516
import java.util.List;
1617
import java.util.Map;
@@ -122,6 +123,11 @@ public <K, V> DataLoader<K, V> getDataLoader(String dataLoaderName) {
122123
return delegateEnvironment.getDataLoader(dataLoaderName);
123124
}
124125

126+
@Override
127+
public DataLoaderRegistry getDataLoaderRegistry() {
128+
return delegateEnvironment.getDataLoaderRegistry();
129+
}
130+
125131
public CacheControl getCacheControl() {
126132
return delegateEnvironment.getCacheControl();
127133
}

‎src/test/groovy/graphql/execution/instrumentation/dataloader/DataLoaderDispatcherInstrumentationTest.groovy‎

Lines changed: 39 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,7 +1,9 @@
11
package graphql.execution.instrumentation.dataloader
22

3+
34
import graphql.ExecutionResult
45
import graphql.GraphQL
6+
import graphql.TestUtil
57
import graphql.execution.AsyncExecutionStrategy
68
import graphql.execution.AsyncSerialExecutionStrategy
79
import graphql.execution.ExecutionContext
@@ -11,6 +13,7 @@ import graphql.execution.batched.BatchedExecutionStrategy
1113
import graphql.execution.instrumentation.ChainedInstrumentation
1214
import graphql.execution.instrumentation.Instrumentation
1315
import graphql.execution.instrumentation.SimpleInstrumentation
16+
import graphql.schema.DataFetcher
1417
import org.dataloader.BatchLoader
1518
import org.dataloader.DataLoader
1619
import org.dataloader.DataLoaderRegistry
@@ -23,6 +26,8 @@ import java.util.concurrent.ForkJoinPool
2326

2427
import static graphql.ExecutionInput.newExecutionInput
2528
import static graphql.StarWarsSchema.starWarsSchema
29+
import static graphql.schema.idl.RuntimeWiring.newRuntimeWiring
30+
import static graphql.schema.idl.TypeRuntimeWiring.newTypeWiring
2631

2732
class DataLoaderDispatcherInstrumentationTest extends Specification {
2833

@@ -52,15 +57,15 @@ class DataLoaderDispatcherInstrumentationTest extends Specification {
5257
chainedInstrumentation.instrumentations.any { instr -> instr instanceof DataLoaderDispatcherInstrumentation }
5358
}
5459

55-
def "dispatch is never called if there are no data loaders"() {
60+
def "dispatch is never called if not data loader registry is set in"() {
5661
def dataLoaderRegistry = new DataLoaderRegistry() {
5762
@Override
5863
void dispatchAll() {
5964
assert false, "This should not be called when there are no data loaders"
6065
}
6166
}
6267
def graphQL = GraphQL.newGraphQL(starWarsSchema).build()
63-
def executionInput = newExecutionInput().dataLoaderRegistry(dataLoaderRegistry).query('{ hero { name } }').build()
68+
def executionInput = newExecutionInput().query('{ hero { name } }').build()
6469

6570
when:
6671
def er = graphQL.execute(executionInput)
@@ -225,4 +230,36 @@ class DataLoaderDispatcherInstrumentationTest extends Specification {
225230
starWarsWiring.batchFunctionLoadCount == 2
226231
starWarsWiring.naiveLoadCount == 8
227232
}
233+
234+
def "can be efficient with lazily computed data loaders"() {
235+
236+
def sdl = '''
237+
type Query {
238+
field : String
239+
}
240+
'''
241+
242+
BatchLoader batchLoader = { keys -> CompletableFuture.completedFuture(keys) }
243+
244+
DataFetcher df = { env ->
245+
def dataLoader = env.getDataLoaderRegistry().computeIfAbsent("key", { key -> DataLoader.newDataLoader(batchLoader) })
246+
247+
return dataLoader.load("working as expected")
248+
}
249+
def runtimeWiring = newRuntimeWiring().type(
250+
newTypeWiring("Query").dataFetcher("field", df).build()
251+
).build()
252+
253+
def graphql = TestUtil.graphQL(sdl, runtimeWiring).build()
254+
255+
DataLoaderRegistry dataLoaderRegistry = new DataLoaderRegistry()
256+
257+
when:
258+
def executionInput = newExecutionInput().dataLoaderRegistry(dataLoaderRegistry).query('{ field }').build()
259+
def er = graphql.execute(executionInput)
260+
261+
then:
262+
er.errors.isEmpty()
263+
er.data["field"] == "working as expected"
264+
}
228265
}

0 commit comments

Comments
 (0)