content-type parsing is performed even if a text response type is explicitly requested, causing an unmanageable error

[rotemdan]

Just upgraded to 6.0.0 and immediately I'm facing a problem. The new behavior of unconditionally parsing the data based on content-type of the response is preventing me to override an invalid content-type that is sent by a server.

I've set the response type to 'text', to intentionally override and prevent any attempt at parsing the data by Gaxios, but that doesn't seem to work anymore in 6.0.0.

After downgrading back to 5.1.3 I realized that in the previous version, even if I set the response type to 'json', it would ignore the parsing error and provide the response as text instead. Here is the relevant method in the Gaxios code:

  private async getResponseData(
    opts: GaxiosOptions,
    res: FetchResponse
  ): Promise<any> {
    switch (opts.responseType) {
      case 'stream':
        return res.body;
      case 'json': {
        let data = await res.text();
        try {
          data = JSON.parse(data);
        } catch {
          // continue
        }
        return data as {};
      }
      case 'arraybuffer':
        return res.arrayBuffer();
      case 'blob':
        return res.blob();
      default:
        return this.getResponseDataFromContentType(res);
    }
  }

In getResponseData, there is no handling of the case where the user explicitly specifies 'text' as response type. The new behavior, of having the content-type unconditionally parsed, is now causing an attempt to parse the response as JSON even if that wasn't requested, and failing in an unrecoverable way when the parsing fails.

I suggest that the function would be extended to consider when the user explicitly specifies the text response type:

  private async getResponseData(
    opts: GaxiosOptions,
    res: FetchResponse
  ): Promise<any> {
    switch (opts.responseType) {
      case 'stream':
        return res.body;
      case 'text':
        return res.text();
      case 'json': {
        let data = await res.text();
        try {
          data = JSON.parse(data);
        } catch {
          // continue
        }
        return data as {};
      }
      case 'arraybuffer':
        return res.arrayBuffer();
      case 'blob':
        return res.blob();
      default:
        return this.getResponseDataFromContentType(res);
    }
  }

I also suggest that the new auto-parsing behavior would be modified to ignore JSON parsing errors, similarly to getResponseData:

  /**
   * Attempts to parse a response by looking at the Content-Type header.
   * @param {FetchResponse} response the HTTP response.
   * @returns {Promise<any>} a promise that resolves to the response data.
   */
  private async getResponseDataFromContentType(
    response: FetchResponse
  ): Promise<any> {
    let contentType = response.headers.get('Content-Type');
    if (contentType === null) {
      // Maintain existing functionality by calling text()
      return response.text();
    }
    contentType = contentType.toLowerCase();
    if (contentType.includes('application/json')) {
      let data = await response.text();

      try {
        data = JSON.parse(data);
      }
      catch (_a) {
        // continue
      }

      return data;
    } else if (
      contentType.includes('text/plain') ||
      contentType.includes('text/html')
    ) {
      return response.text();
    } else {
      // If the content type is something not easily handled, just return the raw data (blob)
      return response.blob();
    }
  }
}

Please let me know if you would like a pull request for this.

[ddelgrosso1]

Please see: #554 I believe this should resolve the issue you are facing with the missing text content handling.

[rotemdan]

Thanks, I'll test it to ensure it works correctly in my case.

Looking at the code, I tried to find remaining potential places where the response is attempted to be parsed as JSON, in which gaxios would produce an error when the JSON fails to parse:

• If the user doesn't specify a response type, getResponseDataFromContentType may throw an error if the server reports a content-type of application/json but the response fails to parse as JSON.
• On the other hand, due to the try { ... } catch { /* continue */} in getResponseData, if the user explicitly requests a 'json' response type, and the parsing fails, they will not get an error, even if the server's returned content-type was actually application/json (the content-type is not checked in this case). That looks somewhat inconsistent, since I would assume that the expectation here would be stronger, since the both the user has explicitly indicated they expect a JSON response, and the server itself indicated that its content-type was supposed to be JSON (to clarify - I'm not necessarily suggesting having it error in this case is a good or bad idea, just mentioning this scenario for comparison).
• I also noticed, in common.ts, that when constructing a GaxiosError, translateData may fail to parse the response as JSON in the cases where the user requested 'json', 'arraybuffer' or 'blob'. So it would fail to create a new GaxiosError object. Usually when constructing an Error object, it is undesirable to have the Error construction itself throw an Error. This isn't completely related to the original issue, but I noticed it.

[ddelgrosso1]

Regarding 1 & 2. I will put a fix in place to make it consistent behavior, thanks for the heads up! Let me take a closer look at 3.