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.

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

Incorrect GaxiosError message type when request responseType "arrayBuffer" #529

Closed
TeemuKoivisto opened this issue Mar 15, 2023 · 2 comments · Fixed by #537
Closed

Incorrect GaxiosError message type when request responseType "arrayBuffer" #529

TeemuKoivisto opened this issue Mar 15, 2023 · 2 comments · Fixed by #537
Assignees
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@TeemuKoivisto
Copy link

TeemuKoivisto commented Mar 15, 2023

I'm using responseType: "arraybuffer", with my drive_v3.Drive requests that sometimes fail with eg expired credentials, but the returned error is no longer readable as it's an ArrayBuffer. Parsing the array buffer shows the original error again.

Steps to reproduce

Use this with expired access_token. Incorrect parameters could work as well.

import { Auth } from 'googleapis'
import { GaxiosError, GaxiosPromise } from 'gaxios'

const CHUNK_SIZE = 1024 * 1024 * 5

function createClient(credentials?: Auth.Credentials): Auth.OAuth2Client {
  const client = new Auth.OAuth2Client(
    GOOGLE_CLIENT_ID,
    GOOGLE_CLIENT_SECRET,
    GOOGLE_OAUTH_CALLBACK_URL
  )
  if (credentials) {
    client.setCredentials(credentials)
  }
  return client
}

function getFile(fileId: string) {
  const client = createClient()
  client.setCredentials({
    access_token: token,
    scope: 'https://www.googleapis.com/auth/drive.readonly',
    token_type: 'Bearer'
  })
  client.files.get(
    { fileId, alt: 'media' },
    {
      responseType: 'arraybuffer',
      headers: {
        Range: `bytes=0-${CHUNK_SIZE - 1}`
      }
    }
  )
}

async function run() {
  try {
    await getFile(<google drive file id>)
  } catch (err: any) {
    if (err instanceof GaxiosError) {
      const errCode = parseInt(err.code || '500')
      const parsed = typeof err.message === ArrayBuffer ? Buffer.from(err.message).toString('utf-8') : 'was string'
      console.log('original message: ', err.message)
      console.log('parsed message: ', parsed)
    }
    console.log(err)
  }
}

Environment details

  System:
    OS: macOS 12.5.1
    CPU: (10) arm64 Apple M1 Pro
    Memory: 77.03 MB / 32.00 GB
    Shell: 5.8.1 - /bin/zsh
  Binaries:
    Node: 18.13.0 - ~/.nvm/versions/node/v18.13.0/bin/node
    Yarn: 1.22.19 - ~/.nvm/versions/node/v18.13.0/bin/yarn
    npm: 8.19.3 - ~/.nvm/versions/node/v18.13.0/bin/npm
  Browsers:
    Chrome: 111.0.5563.64
    Firefox: 110.0
    Safari: 15.6.1
  npmPackages:
    gaxios: ^5.0.2 => 5.0.2

The bug should exist with the latest (5.1.0) version as well.

@TeemuKoivisto TeemuKoivisto added priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. labels Mar 15, 2023
@sofisl
Copy link
Contributor

sofisl commented May 23, 2023

Hi @TeemuKoivisto,

I was able to reproduce with a few tweaks on my end, since the code you pasted didn't completely run out of the gate (specifically, it doesn't seem to instantiate the Drive instance anywhere). However, I'm having trouble understanding the bug. It seems to me that if you are requesting for the reply to come back to you in 'arraybuffer', your error would also be in that format. Seems like we could be subverting people's expectations by always returning it as a string. Could you share a little bit of why you think it should be a string, despite explicitly requesting otherwise? Perhaps if there's other patterns that other code follows, that might be worth looking into!

Thanks!

@TeemuKoivisto
Copy link
Author

Hi and thanks for the reply! Yeah, sorry about the example - just hacked it together. I guess the problem here lies with the thrown error. Since its contents are in binary as well, it's rather impossible to figure out what went wrong. Here's what I ended up doing:

import { GaxiosError, GaxiosPromise } from "gaxios";

import { Result } from "@focal/types/result";

function sleep(ms: number): Promise<boolean> {
  return new Promise((resolve) => {
    setTimeout(() => {
      resolve(true);
    }, ms);
  });
}

/**
 * Wrap Gaxios promises in order to not to throw errors and instead return handled Result types
 * @param promise
 * @returns
 */
export async function wrapGaxios<T>(
  promise: GaxiosPromise<T>,
  timeout = 10000,
): Promise<Result<T>> {
  try {
    const res = await Promise.race([promise, sleep(timeout)]);
    if (typeof res === "boolean") {
      return { ok: false, err: "Gaxios request timeout", errCode: 400 };
    } else if (res.status >= 400) {
      return {
        ok: false,
        err: ((res.data as any) || "").toString(),
        errCode: res.status,
      };
    }
    return { ok: true, data: res.data };
  } catch (err: any) {
    if (err instanceof GaxiosError) {
      // When "responseType" is "arraybuffer" like in getFile.ts, the error message is also an array buffer
      const message = err.message as string | ArrayBuffer;
      const parsedBuffer =
        message instanceof ArrayBuffer &&
        Buffer.from(message).toString("utf-8");
      const errJson = parsedBuffer && JSON.parse(parsedBuffer);
      const parsedMsg = typeof errJson === "object" && errJson?.error?.message;
      const errCode = parseInt(err.code || "500");
      return { ok: false, err: parsedMsg || message, errCode };
    }
    return { ok: false, err, errCode: err?.status || 500 };
  }
}

I added a manual timeout since Gaxios didn't seem to do it itself. However, as this code became rather messy I actually ended up just using fetch as it's now native in NodeJS v18 and I reuse the code in client as well.

But in conclusion, IMO it would be better if you could just parse the error when the response type is array buffer automatically. Just makes life a lot easier.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p2 Moderately-important priority. Fix may not be included in next release. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants