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

fix: check the X-ClickHouse-Exception-Code for err code #350

Open
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

dermasmid
Copy link

@dermasmid dermasmid commented Nov 5, 2024

Summary

use the X-ClickHouse-Exception-Code header ClickHouse/ClickHouse#8786

Related: #332 (similar issue can happen with inserts)

@slvrtrn
Copy link
Contributor

slvrtrn commented Nov 5, 2024

fixes: #332

It does not fix it for select streaming, as the headers will already be received; this is only for inserts.

@dermasmid
Copy link
Author

ok, so this solves a similar issue we had, should i open a separate issue for that?

@slvrtrn
Copy link
Contributor

slvrtrn commented Nov 5, 2024

ok, so this solves a similar issue we had, should i open a separate issue for that?

No, this is fine. Thanks for the contribution!

@slvrtrn slvrtrn changed the title fix: check the X-ClickHouse-Exception-Code for err code, fixes: #332 fix: check the X-ClickHouse-Exception-Code for err code Nov 5, 2024
@slvrtrn
Copy link
Contributor

slvrtrn commented Nov 5, 2024

@dermasmid, is it possible to add an integration test for that? And one more question: in case of a CH error rejection, we usually return an instance of ClickHouseError. Is it the same when X-ClickHouse-Exception-Code is present in the response?

@dermasmid
Copy link
Author

dermasmid commented Nov 5, 2024

in theory the error should be parsed just fine, since it's using regex to find the error from the body:

/(Code|Error): (?<code>\d+).*Exception: (?<message>.+)\((?<type>(?=.+[A-Z]{3})[A-Z0-9_]+?)\)/s

ill try to get a test in too

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.

3 participants