Batched queries result in undefined context

[chrissm79]

Not sure when this started but when I send a batched query it results in an undefined context. Using Apollo Client, if I use createNetworkInterface my queries run just fine, but when switching to createBatchingNetworkInterface, my server errors out with cannot call authorized on undefined.

I'm using graphql-server-express

[freiksenet]

Hi!

Thank you for the report. I wonder if you have a bigger repro case for this? I've tried running batched queries and it seems to be working properly on my side.

Thanks!

[chrissm79]

Well, the query itself runs just fine but the context is undefined which is something I rely on to hold the current user. Attached some screenshots below, you can see that the only difference between the requests is wrapping the second in array brackets. It makes its way to the resolver but the context isn't set so it throws an error.

[chrissm79]

Jumping in and out of this project today but I might have found the issue. My context is a class (similar to this project). If I use a plain object instead, it seems to work I just haven't had time to fully test.

[chrissm79]

Confirmed that it is indeed due to the context being a class and not a POJO. If this is expected behavior just let me know and I'll close the issue. Otherwise, I can rename the issue to something more relevant and if time prevails I'll submit a PR to allow for the context to be set to a class (maybe by requiring a factory function that's called on each request?). Thanks!

[freiksenet]

Hi!

Yeah, the context is copied by doing Object.assign({}, context) so that a new version of context is used in each batch (in case it's mutated). It does break for class-like contexts.

So there are two options on how to proceed here. We could create a context via context function every time we create a batch job, but this might be inefficient in case context creation is somehow expensive. However, this will be a more semantically correct way to do this, because in normal scenario we do create a context for each function. Alternatively we can mandate that context should be a POJO.

Pinging more people to chip in. @helfer @stubailo

[helfer]

Another alternative would be to explicitly say that the context should not be mutated, which would let us skip the assign step. Although we should probably check why it was introduced in the first place before we do that.

[ericclemmons]

I'm using a class for Context, and ran into similar issues.

I had no idea that was the culprit.

I agree that we should look into why the POJO assumption exists.

[freiksenet]

I've been thinking about this over the weekend. I think we should definitely remove the copying, because it doesn't really solve the problem, just makes it seem not there for trivial stuff.

I'll write out basic assumptions and ideas about this.

We naturally want batched queries to be executed exactly like the non-batched query, in a way that the results should be the same. The only thing that will guarantee that happening is actually creating context (and thus the whole GraphQLOptions type) from scratch for every request in a batched request. Copying doesn't really remove the problem of mutation in context as a) mutation might change nested objects b) state might be kept in a stateful object inside the context (eg I've had things like "stats" inside context, which had imperative API and which resolvers called).

However, there might be performance penalties to creating so many contexts. One might want to share, eg, database connection or some other expensive context object. Having said that, I'm not sure this concern is more important than correctness of execution. In addition, one can do such performance optimizations by separating expensive parts of context from context creation (eg by having some database client abstraction that maintains connection).

Therefore, my proposal is - modify the logic, so that in batched queries context (GraphQLOptions) is created separately for each query.

CC @helfer

[csampson]

I ran into this same issue with batching, where only my context object's own properties were defined.

[bennyhobart]

@freiksenet Reusing context between these operations gives a huge performance boost, as you mentioned when using data loaders... I'd prefer if you allowed both "reuse" or "creation" for every operation in a batch request

[bennyhobart]

The more I think about it the more I think this should be removed, as it's only a shallow clone we aren't even able to guarantee the context isn't unique for each request, it requires a builder function as you described @freiksenet .

happy to push forward with this but need a clear signal from you guys that this work isn't in vain

[abernix]

Does anyone know if this issue still exists in the latest version of Apollo Server?

If this is still a problem, can someone who's experiencing the problem (or anyone who comes across this issue and is able to assist) build a reproduction of the problem in to a runnable CodeSandbox reproduction using the latest version of Apollo Server and sharing the link to that CodeSandbox in this issue?

Building reproductions takes a large portion of core contributor's and issue triager's time, particularly when trying to determine if an issue has been resolved in a newer version of Apollo Server, so this would be tremendously helpful.

@chrissm79 If this problem is resolved, please feel free to close this issue, otherwise, we will close it if we don't hear back on the above. Thanks!

[JacksonKearl]

Haven't heard back, so going to go ahead and close this. If anyone else experiences this we can definitely take another look!