From a58b528e3b86e4c6a101870feb0607646f52c848 Mon Sep 17 00:00:00 2001 From: alessia Date: Thu, 6 Apr 2023 14:20:48 -0400 Subject: [PATCH 1/3] fix: issue where query is stuck on loading if final chunk contains only hasNext: false --- .changeset/serious-feet-sparkle.md | 5 ++ src/link/http/__tests__/HttpLink.ts | 77 ++++++++++++++++++++++ src/link/http/parseAndCheckHttpResponse.ts | 8 +++ 3 files changed, 90 insertions(+) create mode 100644 .changeset/serious-feet-sparkle.md diff --git a/.changeset/serious-feet-sparkle.md b/.changeset/serious-feet-sparkle.md new file mode 100644 index 00000000000..c592d656447 --- /dev/null +++ b/.changeset/serious-feet-sparkle.md @@ -0,0 +1,5 @@ +--- +"@apollo/client": patch +--- + +If a multipart chunk contains only `hasNext: false`, immediately complete the observable. diff --git a/src/link/http/__tests__/HttpLink.ts b/src/link/http/__tests__/HttpLink.ts index 63b24b634bf..a0c99a1d46d 100644 --- a/src/link/http/__tests__/HttpLink.ts +++ b/src/link/http/__tests__/HttpLink.ts @@ -1346,6 +1346,18 @@ describe('HttpLink', () => { '-----', ].join("\r\n"); + const nonNullErrorBody = [ + "--graphql", + "content-type: application/json", + "", + '{"data":{"allProducts":[null,null,null]},"errors":[{"message":"Cannot return null for non-nullable field Product.nonNullErrorField.","locations":[{"line":1,"column":53}],"path":["allProducts",0,"nonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.nonNullErrorField."," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)"," at executeField (/usr/src/app/node_modules/graphql/execution/execute.js:489:19)"," at executeFields (/usr/src/app/node_modules/graphql/execution/execute.js:413:20)"," at completeObjectValue (/usr/src/app/node_modules/graphql/execution/execute.js:914:10)"," at completeAbstractValue (/usr/src/app/node_modules/graphql/execution/execute.js:795:10)"," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:624:12)"," at /usr/src/app/node_modules/graphql/execution/execute.js:696:25"," at Function.from ()"," at completeListValue (/usr/src/app/node_modules/graphql/execution/execute.js:676:34)"," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:607:12)"]}}},{"message":"Cannot return null for non-nullable field Product.nonNullErrorField.","locations":[{"line":1,"column":53}],"path":["allProducts",1,"nonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.nonNullErrorField."," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)"," at executeField (/usr/src/app/node_modules/graphql/execution/execute.js:489:19)"," at executeFields (/usr/src/app/node_modules/graphql/execution/execute.js:413:20)"," at completeObjectValue (/usr/src/app/node_modules/graphql/execution/execute.js:914:10)"," at completeAbstractValue (/usr/src/app/node_modules/graphql/execution/execute.js:795:10)"," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:624:12)"," at /usr/src/app/node_modules/graphql/execution/execute.js:696:25"," at Function.from ()"," at completeListValue (/usr/src/app/node_modules/graphql/execution/execute.js:676:34)"," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:607:12)"]}}},{"message":"Cannot return null for non-nullable field Product.nonNullErrorField.","locations":[{"line":1,"column":53}],"path":["allProducts",2,"nonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.nonNullErrorField."," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)"," at executeField (/usr/src/app/node_modules/graphql/execution/execute.js:489:19)"," at executeFields (/usr/src/app/node_modules/graphql/execution/execute.js:413:20)"," at completeObjectValue (/usr/src/app/node_modules/graphql/execution/execute.js:914:10)"," at completeAbstractValue (/usr/src/app/node_modules/graphql/execution/execute.js:795:10)"," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:624:12)"," at /usr/src/app/node_modules/graphql/execution/execute.js:696:25"," at Function.from ()"," at completeListValue (/usr/src/app/node_modules/graphql/execution/execute.js:676:34)"," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:607:12)"]}}}],"hasNext":true}', + "--graphql", + "content-type: application/json", + "", + '{"hasNext":false}', + "--graphql--", + ].join("\r\n"); + it('whatwg stream bodies', (done) => { const stream = new ReadableStream({ async start(controller) { @@ -1418,6 +1430,71 @@ describe('HttpLink', () => { ); }); + it('whatwg stream bodies, non-null errors', (done) => { + const stream = new ReadableStream({ + async start(controller) { + const lines = nonNullErrorBody.split("\r\n"); + try { + for (const line of lines) { + await new Promise((resolve) => setTimeout(resolve, 10)); + controller.enqueue(line + "\r\n"); + } + } finally { + controller.close(); + } + }, + }); + + const fetch = jest.fn(async () => ({ + status: 200, + body: stream, + headers: new Headers({ 'Content-Type': 'multipart/mixed;boundary="graphql";deferSpec=20220824' }), + })); + + const link = new HttpLink({ + fetch: fetch as any, + }); + + let i = 0; + execute(link, { query: sampleDeferredQuery }).subscribe( + result => { + try { + if (i === 0) { + expect(result).toMatchObject({ + data: { + allProducts: [ + null, + null, + null + ] + }, + // errors is also present, but for the purpose of this test + // we're not interested in its (lengthy) content. + // errors: [{...}], + hasNext: true, + }); + } + // Since the second chunk contains only hasNext: false, + // there is no next result to receive. + } catch (err) { + done(err); + } finally { + i++; + } + }, + err => { + done(err); + }, + () => { + if (i !== 1) { + done(new Error("Unexpected end to observable")); + } + + done(); + }, + ); + }); + it('node stream bodies', (done) => { const stream = Readable.from(body.split("\r\n").map((line) => line + "\r\n")); diff --git a/src/link/http/parseAndCheckHttpResponse.ts b/src/link/http/parseAndCheckHttpResponse.ts index c6a86c737aa..53e30c29082 100644 --- a/src/link/http/parseAndCheckHttpResponse.ts +++ b/src/link/http/parseAndCheckHttpResponse.ts @@ -100,6 +100,14 @@ export async function readMultipartBody< // we don't need to call observer.next as there is no data/errors observer.next?.(result); } + } else if ( + // If the chunk contains only a "hasNext: false", we can call + // observer.complete() immediately. + Object.keys(result).length === 1 && + "hasNext" in result && + !result.hasNext + ) { + observer.complete?.(); } } catch (err) { handleError(err, observer); From cfe13596f16bb7df854e2bae9c03df80611ff926 Mon Sep 17 00:00:00 2001 From: alessia Date: Thu, 6 Apr 2023 14:43:05 -0400 Subject: [PATCH 2/3] chore: truncate repetitive errors in test --- src/link/http/__tests__/HttpLink.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/link/http/__tests__/HttpLink.ts b/src/link/http/__tests__/HttpLink.ts index a0c99a1d46d..07bc6b801b8 100644 --- a/src/link/http/__tests__/HttpLink.ts +++ b/src/link/http/__tests__/HttpLink.ts @@ -1350,7 +1350,7 @@ describe('HttpLink', () => { "--graphql", "content-type: application/json", "", - '{"data":{"allProducts":[null,null,null]},"errors":[{"message":"Cannot return null for non-nullable field Product.nonNullErrorField.","locations":[{"line":1,"column":53}],"path":["allProducts",0,"nonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.nonNullErrorField."," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)"," at executeField (/usr/src/app/node_modules/graphql/execution/execute.js:489:19)"," at executeFields (/usr/src/app/node_modules/graphql/execution/execute.js:413:20)"," at completeObjectValue (/usr/src/app/node_modules/graphql/execution/execute.js:914:10)"," at completeAbstractValue (/usr/src/app/node_modules/graphql/execution/execute.js:795:10)"," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:624:12)"," at /usr/src/app/node_modules/graphql/execution/execute.js:696:25"," at Function.from ()"," at completeListValue (/usr/src/app/node_modules/graphql/execution/execute.js:676:34)"," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:607:12)"]}}},{"message":"Cannot return null for non-nullable field Product.nonNullErrorField.","locations":[{"line":1,"column":53}],"path":["allProducts",1,"nonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.nonNullErrorField."," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)"," at executeField (/usr/src/app/node_modules/graphql/execution/execute.js:489:19)"," at executeFields (/usr/src/app/node_modules/graphql/execution/execute.js:413:20)"," at completeObjectValue (/usr/src/app/node_modules/graphql/execution/execute.js:914:10)"," at completeAbstractValue (/usr/src/app/node_modules/graphql/execution/execute.js:795:10)"," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:624:12)"," at /usr/src/app/node_modules/graphql/execution/execute.js:696:25"," at Function.from ()"," at completeListValue (/usr/src/app/node_modules/graphql/execution/execute.js:676:34)"," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:607:12)"]}}},{"message":"Cannot return null for non-nullable field Product.nonNullErrorField.","locations":[{"line":1,"column":53}],"path":["allProducts",2,"nonNullErrorField"],"extensions":{"code":"INTERNAL_SERVER_ERROR","exception":{"stacktrace":["Error: Cannot return null for non-nullable field Product.nonNullErrorField."," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:594:13)"," at executeField (/usr/src/app/node_modules/graphql/execution/execute.js:489:19)"," at executeFields (/usr/src/app/node_modules/graphql/execution/execute.js:413:20)"," at completeObjectValue (/usr/src/app/node_modules/graphql/execution/execute.js:914:10)"," at completeAbstractValue (/usr/src/app/node_modules/graphql/execution/execute.js:795:10)"," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:624:12)"," at /usr/src/app/node_modules/graphql/execution/execute.js:696:25"," at Function.from ()"," at completeListValue (/usr/src/app/node_modules/graphql/execution/execute.js:676:34)"," at completeValue (/usr/src/app/node_modules/graphql/execution/execute.js:607:12)"]}}}],"hasNext":true}', + '{"data":{"allProducts":[null,null,null]},"errors":[{"message":"Cannot return null for non-nullable field Product.nonNullErrorField."},{"message":"Cannot return null for non-nullable field Product.nonNullErrorField."},{"message":"Cannot return null for non-nullable field Product.nonNullErrorField."}],"hasNext":true}', "--graphql", "content-type: application/json", "", From 30f672631920e8af767bfa68e381916b1bdef593 Mon Sep 17 00:00:00 2001 From: alessia Date: Thu, 6 Apr 2023 14:45:34 -0400 Subject: [PATCH 3/3] chore: add comment, improve naming in test --- src/link/http/__tests__/HttpLink.ts | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/src/link/http/__tests__/HttpLink.ts b/src/link/http/__tests__/HttpLink.ts index 07bc6b801b8..dd7ffd9b8cf 100644 --- a/src/link/http/__tests__/HttpLink.ts +++ b/src/link/http/__tests__/HttpLink.ts @@ -1346,7 +1346,7 @@ describe('HttpLink', () => { '-----', ].join("\r\n"); - const nonNullErrorBody = [ + const finalChunkOnlyHasNextFalse = [ "--graphql", "content-type: application/json", "", @@ -1430,10 +1430,12 @@ describe('HttpLink', () => { ); }); - it('whatwg stream bodies, non-null errors', (done) => { + // Verify that observable completes if final chunk does not contain + // incremental array. + it('whatwg stream bodies, final chunk of { hasNext: false }', (done) => { const stream = new ReadableStream({ async start(controller) { - const lines = nonNullErrorBody.split("\r\n"); + const lines = finalChunkOnlyHasNextFalse.split("\r\n"); try { for (const line of lines) { await new Promise((resolve) => setTimeout(resolve, 10));