postgres icon indicating copy to clipboard operation
postgres copied to clipboard

Fix ambiguous error on startup

Open alexgleason opened this issue 1 year ago • 3 comments

Fixes https://github.com/porsager/postgres/issues/923

Before & after:

Screenshot from 2024-09-15 18-58-17

alexgleason avatar Sep 16 '24 00:09 alexgleason

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.

martinwepner-otto avatar Sep 20 '24 08:09 martinwepner-otto

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.

fin-dogedev avatar Sep 21 '24 00:09 fin-dogedev

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 ?

coder-se avatar Oct 18 '24 09:10 coder-se

Please merge this. Is this project still being maintained?

chanon avatar Oct 24 '24 12:10 chanon

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.

porsager avatar Oct 24 '24 12:10 porsager

also changes should only be made to the src, the rest of the files are auto generated

porsager avatar Oct 24 '24 12:10 porsager

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

chanon avatar Oct 24 '24 12:10 chanon

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.

wataruoguchi avatar Nov 11 '24 17:11 wataruoguchi

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.

alexgleason avatar Nov 18 '24 19:11 alexgleason

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

alexgleason avatar Nov 18 '24 20:11 alexgleason

@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?

alexgleason avatar Dec 19 '24 16:12 alexgleason

I'll take a look at it tonight @alexgleason !

porsager avatar Dec 19 '24 16:12 porsager

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 avatar Dec 19 '24 18:12 coder-se

@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.

alexgleason avatar Dec 19 '24 19:12 alexgleason

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.

termermc avatar Jan 18 '25 02:01 termermc

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!

danieldietzler avatar Mar 28 '25 21:03 danieldietzler

If it helps, this ^ linked pr shows the result of some testing of this patch.

bo0tzz avatar Mar 28 '25 22:03 bo0tzz

@porsager Could you please clarify what is blocking this PR to be merged and released?

theoludwig avatar May 20 '25 21:05 theoludwig

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.

porsager avatar May 20 '25 23:05 porsager