first class dataloader support #1263
Conversation
This reverts commit 38dfab1
bbakerman
added
the
Not to be merged
|
|
||
|
|
||
| public ExecutionInput(String query, String operationName, Object context, Object root, Map<String, Object> variables) { |
There was a problem hiding this comment.
I might want to change this to prevent broken API for those who called create direct
| instrumentations.add(assertNotNull(instrumentation)); | ||
| return new ChainedInstrumentation(instrumentations); | ||
| } | ||
|
|
There was a problem hiding this comment.
need to kill this method. Not used
| * | ||
| * fragment BarFragment on Query { | ||
| * bar | ||
| * } | ||
| * |
bbakerman
removed
the
Not to be merged
| @@ -106,6 +126,7 @@ public static Builder newExecutionInput() { | |||
| private Object context; | |||
| private Object root; | |||
| private Map<String, Object> variables = Collections.emptyMap(); | |||
| private DataLoaderRegistry dataLoaderRegistry = new DataLoaderRegistry(); | |||
There was a problem hiding this comment.
so default is we always have one?
There was a problem hiding this comment.
yes we always have one in play
| @@ -35,13 +36,14 @@ | |||
| private final Object context; | |||
| private final Instrumentation instrumentation; | |||
| private final List<GraphQLError> errors = new CopyOnWriteArrayList<>(); | |||
| private final DataLoaderRegistry dataLoaderRegistry; | |||
| private final DeferSupport deferSupport = new DeferSupport(); | |||
|
|
|||
| public ExecutionContext(Instrumentation instrumentation, ExecutionId executionId, GraphQLSchema graphQLSchema, InstrumentationState instrumentationState, ExecutionStrategy queryStrategy, ExecutionStrategy mutationStrategy, ExecutionStrategy subscriptionStrategy, Map<String, FragmentDefinition> fragmentsByName, Document document, OperationDefinition operationDefinition, Map<String, Object> variables, Object context, Object root) { | |||
There was a problem hiding this comment.
can we mark the constructor as Internal and I am not sure we should always create a new DLR here.
There was a problem hiding this comment.
I did the extra new DLR here JUST in case some one created it by hand because it was a public method
There was a problem hiding this comment.
But since this is already a breaking change - I am going to clean this up by removing the old public constructor.
Our code should be the only constructor of these things
| private boolean aggressivelyBatching = true; | ||
|
|
||
| public DataLoaderDispatcherInstrumentationState(Logger log, DataLoaderRegistry dataLoaderRegistry) { | ||
|
|
||
| this.dataLoaderRegistry = dataLoaderRegistry; | ||
| this.approach = new FieldLevelTrackingApproach(log, dataLoaderRegistry); | ||
| this.state = approach.createState(); | ||
| hasNoDataLoaders = dataLoaderRegistry.getKeys().isEmpty(); |
There was a problem hiding this comment.
I see: so instead a nullalble or Optional DRL we check for content here. DLR is immutable, right?
There was a problem hiding this comment.
right - we always have a DLR in play
| .build() | ||
|
|
||
| then: "execution shouldn't hang" | ||
| List<CompletableFuture<ExecutionResult>> futures = [] | ||
| for (int i = 0; i < NUM_OF_REPS; i++) { | ||
| def result = graphql.executeAsync(ExecutionInput.newExecutionInput() | ||
| def result = graphql.executeAsync(newExecutionInput() | ||
| .dataLoaderRegistry(dataLoaderRegistry) |
There was a problem hiding this comment.
it should always be a new DLR, right?
There was a problem hiding this comment.
yes - it should always be a new DLR - this pattern is to try and encourage that
See #1254
This changes the signature of things so that
DataLoaderRegistry's need to be added per execution and not at the schema level.This will encourage people to do the right thing in creating new DataLoaders per execution.
It also ensure thats DataLoaderInstrumentation is ALWAYS loaded if its not already present and it makes it efficient if you have no data loaders in action.