connectrpc / connect-es

The TypeScript implementation of Connect: Protobuf RPC that works.
https://connectrpc.com/
Apache License 2.0
1.38k stars 80 forks source link

Way to recover from errors caused by non-compliant response body #469

Closed itareeee closed 1 year ago

itareeee commented 1 year ago

Is your feature request related to a problem? Please describe.

When connect-web client receives non-200 response with non-compliant response body, then it raises an error of Internal or Unknown though I'd rather have the error code corresponding to the http status code.

In my setup, there's an authentication proxy in front of connect-web server, and it returns http 401 response with empty body when failing to authenticate a request.

I suppose, it's common to have various reverse proxies, and they are usually not compatible with connect protocol.

Describe the solution you'd like

An option to enable fallback process that determines error type based on http status code in case of failing to parse response body.

Just like how error type is determined when content-type is not application/json. https://github.com/bufbuild/connect-es/blob/d6598c5f9fe67f9165e3d06f84cf1a4c8d81f989/packages/connect-web/src/connect-transport.ts#L256-L259

timostamm commented 1 year ago

Thank you for the request, Itaru. The response probably looks like this?

HTTP/1.1 401 Unauthorized
Content-Type: application/json

{"json": "but not a valid Connect error"}

So there is:

And you see a ConnectError with Code.Internal and a message starting with "cannot decode ConnectError...", correct?

itareeee commented 1 year ago

@timostamm Exactly!

Initially, the proxy returned "empty" body, and it caused Unexpected end of JSON input probably because script error occurred in response.json().

I made a change to the proxy so that it returns {}, and it produced the same exact error as you described.

timostamm commented 1 year ago

Thank you for confirming.

I agree that it would be very convenient to raise an error code matching the HTTP status here. On the other hand, this will shadow malformed responses. For example, consider this response:

HTTP/1.1 401 Unauthorized
Content-Type: application/json

{"code": "unauthenticatedTYPO", "message": "important message you want to share"}

It would be odd to see a ConnectError with code Unauthenticated raised, but never see the error message provided from the server.

We are adding a "cause" property to ConnectError:

https://github.com/bufbuild/connect-es/blob/e12db7ccca3be7ac6af7c7eec6c509834ec52de3/packages/connect-core/src/connect-error.ts#L68-L73

I think this might help us to surface this condition.

In the mean time, returning just the HTTP status - without a Content-Type - would be the most simple way to get unblocked:

HTTP/1.1 401 Unauthorized
itareeee commented 1 year ago

Thank you for the advice.

As I don't have control over the proxy behavior, I ended up with monkey-patching fetch function. This is a bit ugly, but I couldn't find a way to customize behaviors of The ConnectTransport generated by createConnectTransport().

const originalFetch = fetch;

window.fetch = async function(input: RequestInfo, init?: RequestInit) {
  const reqHeaders = new Headers(init?.headers)
  const connectVer = reqHeaders.get('Connect-Protocol-Version') ?? '';
  const isConnect = parseInt(connectVer) > 0;

  const res = await originalFetch(input, init);

  if (isConnect && res.status === 401 && res.headers.get('Content-Type') === 'application/json') {
    res.headers.set('Content-Type', '');
  }

  return res;
}

We are adding a "cause" property to ConnectError:

This sounds great. This way, you can access the underlying http response error? I'd definitely replace the monkey-patching with this approach once it's released!

timostamm commented 1 year ago

@itareeee, we ran into this very problem with Fastify :smile: It replies with Content-Type application/json for a HTTP 404 when the request is also application/json.

This is fixed in https://github.com/bufbuild/connect-es/pull/479. We are simply raising a ConnectError with the proper code and a message like "HTTP 404". Using the cause property didn't seem helpful in this case. You should still have access to the response headers via the metadata propery of ConnectError. This is likely going to be released tomorrow.

itareeee commented 1 year ago

This fix seems to perfectly solves my issue. Thank you so much!