Body parsing error for non-JSON 401 response

[oreqizer]

Hi! 👋

Firstly, thanks for your work on this project! 🙂

Today I used patch-package to patch @urql/[email protected] for the project I'm working on.

I have a middleware on my server that returns a status 401 with a plain text of input: unauthorized request for requests whose auth token verification fails (e.g. for a timeout). However, I am getting an error from this function, because it expects the response to be a JSON.

I noticed the function's implementation checks for header type to determine the response handling, however, it'd be IMO better to explicitly check for application/json, rather that checking for not multipart/mixed.

Here is the diff that solved my problem:

diff --git a/node_modules/@urql/core/dist/bda9b693.mjs b/node_modules/@urql/core/dist/bda9b693.mjs
index 80025d5..e6bfb6f 100644
--- a/node_modules/@urql/core/dist/bda9b693.mjs
+++ b/node_modules/@urql/core/dist/bda9b693.mjs
@@ -314,7 +314,7 @@ var h = /boundary="?([^=";]+)"?/i;
 
 function executeIncrementalFetch(e, r, t) {
   var n = t.headers && t.headers.get("Content-Type") || "";
-  if (!/multipart\/mixed/i.test(n)) {
+  if (/application\/json/i.test(n)) {
     return t.json().then((function(n) {
       e(makeResult(r, n, t));
     }));

This issue body was partially generated by patch-package.

[kitten]

I'm not quite exactly sure what you're patching without a reproduction, sorry 😅

As far as I recall this, the test is simply to check for multipart responses and that's simpler this way since the Content-Type cannot be trusted and we'll always try to parse JSON. In other words, you're sending the code into a different code path (which btw doesn't support IE11, but I guess that may be fine) which both parse JSON (although that said, one does JSON.parse on chunks, and the other does response.json()

However, both eventually end up here: https://github.com/FormidableLabs/urql/blob/1ab77b65ef6ed0e85fb50a61b374b306ef046cf4/packages/core/src/internal/fetchSource.ts#L156

So I'm a little confused as to what difference this would make 🤔

[JoviDeCroock]

Hmm, we might not in case we hit the hasResult false part. Another option might be doing

if (!/multipart\/mixed/i.test(n)) {
  return t.text().then(text => {
    hasResults = true;
    try {
      const n = JSON.parse(text)
      e(makeResult(r, n, t));
    } catch (e) {
      throw e;
    }
  })
}

[oreqizer]

I'm not quite exactly sure what you're patching without a reproduction, sorry 😅

As far as I recall this, the test is simply to check for multipart responses and that's simpler this way since the Content-Type cannot be trusted and we'll always try to parse JSON. In other words, you're sending the code into a different code path (which btw doesn't support IE11, but I guess that may be fine) which both parse JSON (although that said, one does JSON.parse on chunks, and the other does response.json()

However, both eventually end up here:

https://github.com/FormidableLabs/urql/blob/1ab77b65ef6ed0e85fb50a61b374b306ef046cf4/packages/core/src/internal/fetchSource.ts#L156

So I'm a little confused as to what difference this would make 🤔

The problem is that this part of code assumes that the response is always a JSON string, in case the content type is different than multipart/mixed. The issue here is that, if a server returns an error response with a plain text as the body, res.json() fails, since the response's just a text, and not a JSON.

  //       v--- this throws
  return t.json()

To be specific, my server (and a common pattern to do so) returns text/plain as Content-Type. This really shouldn't be tried to be parsed as a JSON.

This is what my server's auth handling structure looks like: https://gqlgen.com/recipes/authentication/

Is this understandable? I may whip up a reproduction, though I will need a custom server for it, and thought the issue's simple enough to understand for that effort.

[oreqizer]

Hmm, we might not in case we hit the hasResult false part. Another option might be doing

if (!/multipart\/mixed/i.test(n)) {
  return t.text().then(text => {
    hasResults = true;
    try {
      const n = JSON.parse(text)
      e(makeResult(r, n, t));
    } catch (e) {
      throw e;
    }
  })
}

This would still throw in case of a non-JSON response, wouldn't it?

[JoviDeCroock]

I mean, we could also just use e(makeResult(r, text, t)); which would tell you no content. There's a lot of solutions 😅 but I think parsing as text and optionally going to json is a more reliable result because not everything is application/json, we also have application/graphql+json

Feel free to try the build at #2444