postgres
postgres copied to clipboard
Fix ambiguous error on startup
Fixes https://github.com/porsager/postgres/issues/923
Before & after:
Would be nice if this could be merged and released soon. In my team multiple developers ran into this when trying to connect with wrong credentials and the "cannot read property "replace" of undefined" was not very helpful.
This runtime type error causes the application to terminate if the connection to the database fails unexpectedly. As a result, the reconnection mechanism cannot take effect.
Is there anything blocking this from being merged? Would be really good to get this into a release. The issue has been around since beginning of this year causing applications to terminate like @fin-dogedev says.
Any thoughts @patrickReiis , @porsager ?
Please merge this. Is this project still being maintained?
there are no tests so that means I need to spend time to make them and verify the fix myself - I'm not on a big surplus of time at the moment, and don't use deno myself, so can't easily verify the pr myself.
also changes should only be made to the src, the rest of the files are auto generated
FYI it also happens in the Node.js version I am using v20.13.1
Also it is an uncaught exception that kills the process, so not sure how to write a test for it. I guess spawn/exec and read the error output? The fix is very simple though so not sure a test is required.
In addition to the code changed, also see the root of the issue noted here: https://github.com/porsager/postgres/issues/923#issuecomment-2266235503
During development, I had the same issue as https://github.com/porsager/postgres/issues/778. I can reproduce the issue with my repository.
- Prod runtime: Cloudflare Workers
- Dev runtime: Node (v22.3.0)
- Framework: Remix
- Database: Supabase
- Query Builder: Kysely
I confirmed the change in the src/connection.js (Node.js) in this PR has solved the issue.
I support this PR because the queryError function expects query to be an object. If the query is NOT an object, the queryError function fails anyway. I hope this PR gets merged.
Side note: During development, we need to use NodeJS's module by adding an alias.
also changes should only be made to the src, the rest of the files are auto generated
I updated to only include changes to src.
I do not know how to reproduce an SSL connection error in a test. This issue is apparently also occurring in Node https://github.com/porsager/postgres/issues/923#issuecomment-2462024110
@porsager This is a simple change that is 1 line of code and makes intuitive sense because it is just a guard clause. Will you please merge it?
I'll take a look at it tonight @alexgleason !
We have been running a fork of this PR in production for awhile now, and there seems to be another case where query is actually is an object. But it doesn't have the origin property causing the same issue to be thrown. I think it would be good to adapt the code so that origin is only appended if the property exist.
Additionally it might good safe-guard for null as well, since typeof null === 'object'
@coder-se I've never encountered that, but since it does seem possible for query to be null (eg here: https://github.com/porsager/postgres/blob/6ec85a432b17661ccacbdf7f765c651e88969d36/src/connection.js#L105) I added a check for it.
This is causing issues for me as well (on Node and Deno). Thanks @alexgleason for your work on this, I hope it can be merged in soon.
Hey @porsager, is anything blocking this PR? We're using it in https://github.com/immich-app/immich and it'd be helpful if users can get proper error messages :) Thanks!
If it helps, this ^ linked pr shows the result of some testing of this patch.
@porsager Could you please clarify what is blocking this PR to be merged and released?
My time was the thing blocking it. I felt the fix was a code smell, and not getting to the bottom of the issue. Also why I wanted some way to reproduce it and thereby include some tests.
I've spent some time looking into it now, and it turns out the issue indeed was a bit deeper. There is both an issue around how the initial query was handled causing the original issue, but then there was another issue causing some of the other reports in this PR around err not always being an object.
I've made fixes and tests now. Thanks a lot for the initial suggestion for a fix @alexgleason — I'm very sorry I couldn't dig into this sooner.