feat: uniquely identify batched queries #849
Conversation
I've added the new (and missing) context properties, but I'm unsure how to test these types: never wrote type tests and by looking at the existing ones it's unclear to me how to test this specific ones. Perhaps you can lend a hand? 🙏 |
Check this out @drakhart https://github.com/mercurius-js/mercurius/blob/master/test/types/index.ts |
Yes yes, I did, that's what I meant with "by looking at the existing ones it's unclear to me how to test this specific ones". I'll try my best, no promises made. |
You probably want a test similar in structure to this one where the parameter types are checked: https://github.com/mercurius-js/mercurius/blob/master/test/types/index.ts#L748 Happy to help if you have any further questions :) |
I think it should be fine now, but I'm not sure because it felt too easy in the end. Please have a look and let me know. |
|
|
||
| return Promise.all(request.body.map(async (r, operationId) => { | ||
| // Create individual reqs for multiple operations, otherwise reference the original req | ||
| const operationReq = operationsCount > 1 ? Object.create(request) : request |
There was a problem hiding this comment.
I would like to hear @mcollina's opinion on the approach to cloning a request object. We need it to be cloned because every query in the batch needs a different context, but I don't know if this is a sensible way to clone it
| sinon.assert.calledTwice(contextSpy) | ||
| sinon.assert.calledWith(contextSpy, 0, 2, sinon.match(/TestQuery/)) | ||
| sinon.assert.calledWith(contextSpy, 1, 2, sinon.match(/DoubleQuery/)) |
There was a problem hiding this comment.
why are we not testing that operationId and operationCount are also passed around properly?
There was a problem hiding this comment.
Maybe I don't understand your question, but I believe that's what we're doing here: when calling the resolver we're reading the operationId and operationsCount attributes from the context, and checking that they've been passed as expected: a call with operationId === 0, another call with operationId === 1, both of them with operationsCount === 2, and each one of them with the correct query contents.
There was a problem hiding this comment.
sorry I read that wrong, I thought those were the number of the call and the position of the argument for some reason!
There was a problem hiding this comment.
Haha, I feel you: I even had to double check the spy parameters when writing that assertion.
resolves #842
Fixes
context.__currentQuerybeing overwritten by subsequent batched query operations.Stores
context.operationsCountandcontext.operationId(same naming as with subscriptions) to uniquely identify batched query operations at a later stage.