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

Disregard non-JSON error response bodies #695

Merged
merged 10 commits into from
Jun 28, 2023
Merged

Conversation

smaye81
Copy link
Member

@smaye81 smaye81 commented Jun 28, 2023

This PR modifies the response validation so that Connect will only attempt to parse the body of a unary error if the content type is application/json.

For all other error responses, the body is ignored and the code and message is built from the received HTTP error status and it's corresponding Connect error code.

@smaye81 smaye81 requested a review from timostamm June 28, 2023 17:37
@smaye81 smaye81 changed the title Sayers/disregard json on error Disregard non-JSON error response bodies Jun 28, 2023
@smaye81 smaye81 marked this pull request as ready for review June 28, 2023 17:48
new Headers({
"Content-Type": "application/proto",
})
);
} catch (e) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This test will pass even if validate response never throws. It is missing a fail

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed. Thanks for catching that

@smaye81 smaye81 merged commit 7f31bf1 into main Jun 28, 2023
2 checks passed
@smaye81 smaye81 deleted the sayers/disregard_json_on_error branch June 28, 2023 19:25
smaye81 added a commit to connectrpc/conformance that referenced this pull request Jun 29, 2023
This adds a new Connect server using `connect-fastify` and
`connect-node`. It wires it into the docker-compose setup and allows for
serving HTTP/1.1 and HTTP/2 traffic using cleartext and TLS. Fastify
does not offer HTTP/3 support as of yet, so that path is not available.
Note: this does not yet use TLS, but can be added as a follow-up PR.

In addition, this also refactors a few things such as:
* adds TypeScript support to the web portion
* renames images, Dockerfiles, etc. to match the language so that adding
new languages can follow one pattern. For example,
`client-connect-to-server-connect-go-h2` becomes
`client-connect-go-to-server-connect-go-h2` and uses
`Dockerfile.crosstestgo` instead of `Dockerfile.crosstest`.
* updates to connect-go v1.9.0 for
https://github.com/bufbuild/connect-go/pull/528 which fixes an issue
with parsing response headers from a connect-node server.
* updates to connect-es v0.11.0 for
connectrpc/connect-es#695, which fixes an issue
with parsing response bodies for non-JSON content types.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants