Skip to content
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

fix(middleware): Correlate responses by index when server does not re… #56

Merged
merged 1 commit into from
Nov 24, 2017
Merged

fix(middleware): Correlate responses by index when server does not re… #56

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
44 changes: 41 additions & 3 deletions src/middleware/__tests__/batch.test.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,44 @@ describe('batchMiddleware', () => {
expect(req2.payload).toEqual({ response: { ok: 2 } });
});

it('should make a successfully batch request without server IDs', async () => {
fetchMock.mock({
matcher: '/graphql/batch',
response: {
status: 200,
body: [{ data: { ok: 1 } }, { data: { ok: 2 } }],
},
method: 'POST',
});

const req1 = mockReq(1);
const req2 = mockReq(2);
await rnl.sendQueries([req1, req2]);
expect(req1.payload).toEqual({ response: { ok: 1 } });
expect(req2.payload).toEqual({ response: { ok: 2 } });
});

it('should reject if server returns a different number of responses than requests', async () => {
fetchMock.mock({
matcher: '/graphql/batch',
response: {
status: 200,
body: [{ data: { ok: 2 } }],
},
method: 'POST',
});

const req1 = mockReq(1);
const req2 = mockReq(2);
await rnl.sendQueries([req1, req2]).catch(() => {});
expect(req1.error.toString()).toMatch(
'Server returned a different number of responses than requested.'
);
expect(req2.error.toString()).toMatch(
'Server returned a different number of responses than requested.'
);
});

it('should make a successfully batch request with duplicate request ids', async () => {
fetchMock.mock({
matcher: '/graphql/batch',
Expand All @@ -63,7 +101,7 @@ describe('batchMiddleware', () => {
matcher: '/graphql/batch',
response: {
status: 200,
body: [{ data: {} }, { id: 2, data: { ok: 2 } }],
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed to preserve the original error message - Otherwise it will hit the new path, because a response does not have an id. Alternatively I can change it to return an ID that has not been requested

body: [{ id: 2, data: { ok: 2 } }],
},
method: 'POST',
});
Expand All @@ -82,14 +120,14 @@ describe('batchMiddleware', () => {
matcher: '/graphql/batch',
response: {
status: 200,
body: [{ data: {} }, { id: 2, data: { ok: 2 } }],
body: [{ id: 2, data: { ok: 2 } }],
},
method: 'POST',
});

const req1 = mockReq(1);
const req2 = mockReq(2);
const req3 = mockReq(3);
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The test description says duplicated ids, but the IDs were not actually duplicated :)

const req3 = mockReq(1);
await rnl.sendQueries([req1, req2, req3]).catch(() => {});

expect(req1.error).toBeInstanceOf(Error);
Expand Down
11 changes: 9 additions & 2 deletions src/middleware/batch.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -174,9 +174,16 @@ function sendRequests(requestMap: BatchRequestMap, next, opts) {
throw new Error('Wrong response from server');
}

batchResponse.payload.forEach(res => {
const responseHasIds: boolean = batchResponse.payload.every(response => response.id);
if (!responseHasIds && ids.length !== batchResponse.payload.length) {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This check could be worthwile to have in any case - I guess the number of responses should always be the same as the number of requests?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Who knows. Any server may have bugs, and we should catch this somehow and reject with error by spec.

throw new Error(`Server returned a different number of responses than requested.
It's not possible to correlate requests and responses`);
}

batchResponse.payload.forEach((res, i) => {
if (!res) return;
const request = requestMap[res.id];
const request = responseHasIds ? requestMap[res.id] : requestMap[ids[i]];

if (request) {
const responsePayload = copyBatchResponse(batchResponse, res);
request.completeOk(responsePayload);
Expand Down