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

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

Closed
itareeee opened this issue Feb 17, 2023 · 6 comments · Fixed by #479
Closed

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

itareeee opened this issue Feb 17, 2023 · 6 comments · Fixed by #479
Labels
enhancement New feature or request

Comments

@itareeee
Copy link

itareeee commented Feb 17, 2023

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

@itareeee itareeee added the enhancement New feature or request label Feb 17, 2023
@timostamm
Copy link
Member

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:

  • a HTTP status code indicating an error
  • a content-type Connect believes to be an in-protocol error (see the last example)

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

@itareeee
Copy link
Author

itareeee commented Feb 17, 2023

@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
Copy link
Member

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
Copy link
Author

itareeee commented Feb 20, 2023

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
Copy link
Member

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

This is fixed in #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
Copy link
Author

itareeee commented Feb 22, 2023

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants