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

Fail gracefully when connecting to other database #3026

Merged
merged 2 commits into from
Sep 15, 2023
Merged

Fail gracefully when connecting to other database #3026

merged 2 commits into from
Sep 15, 2023

Conversation

romain-gilliotte
Copy link
Contributor

@romain-gilliotte romain-gilliotte commented Jul 5, 2023

Fixes #2627

I'm currently working at Forest Admin.
We're a French company building a SaaS that generates admin panels.

During onboarding, we connect to our customer's databases to introspect the structure.
However, customers don't always know which database vendor they are using, and click on the wrong one during onboarding.

This causes pg-protocol to throw an unhandled exception, and our backend workers to go down.

I tried to follow the coding style and test this.
Don't hesitate to come back to me if anything is not at it should be!

Note that I never managed to run all the tests locally... so I'm likely to break others.
I'm creating the PR so that the CI runs, and that I can check if everything is OK

@romain-gilliotte romain-gilliotte changed the title Fail gracefully when connecting to other SGDB vendor Fail gracefully when connecting to other database Jul 5, 2023
@brianc
Copy link
Owner

brianc commented Aug 12, 2023

Hey @romain-gilliotte thanks for doing this! This has actually be a long time thorn in the side of folks, and any UnahandledRejection is no beuno. The tests failed because the native driver bindings actually already properly throw and don't crash with an unhandled rejection, but the error message is different. I'm going go ahead and push to your fork and fix the tests so we're good to merge this, and then merge it!

@romain-gilliotte
Copy link
Contributor Author

Thank you so much for looking into this!

@Scra3
Copy link

Scra3 commented Aug 25, 2023

Hello @brianc and @romain-gilliotte.
@brianc apparently there's still a test that's failing, do you need help or do you need a little time to fix the test? 🙏

@charmander
Copy link
Collaborator

(Re-ran the test with the Cloudflare-worker-related segfault, it passed.)

@Scra3
Copy link

Scra3 commented Sep 11, 2023

Hello,
Can we merge 🙏 ?
Thank you very much for the work and time spent on the bug.

@slimee
Copy link

slimee commented Sep 12, 2023

It seems this PR is ready to merge, can we go please?
It would helps a lot 🙏🏽

@brianc brianc merged commit b1a8947 into brianc:master Sep 15, 2023
15 checks passed
@slimee
Copy link

slimee commented Sep 15, 2023 via email

@Scra3
Copy link

Scra3 commented Sep 18, 2023

ty

@Scra3
Copy link

Scra3 commented Nov 13, 2023

Hello,
Is it possible to release it on npm 🙏 ?

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.

I accidentally tried to connect to mysql db and there's an error not captured and makes nodejs thread down
5 participants