node-postgres icon indicating copy to clipboard operation
node-postgres copied to clipboard

fix(parser): handle exceptions within handlePacket

Open avallete opened this issue 9 months ago • 3 comments

Hey there !

This is a proposal, in order to address #2653 it introduce another error type in addition to DatabaseError named ParserError, in charge of bubbling up any unexpected error that might occurs not on the Database itself, but between the database and the javascript parsing code (such as string too long for javascript to handle).

This is not the perfect solution since the parser will still throw an error. The benefit of this is that the pg package will no longer throw uncatchable exceptions when faced with such cases. Instead the error will bubble up into the same .on('error') channel and can be caught by the caller program.

One of the parsing error (string too long) can be reproduced with this database:

CREATE TABLE public.large_data(data text);
INSERT INTO public.large_data(data) VALUES (repeat('A', 750 * 1024 * 1024)); -- create a string larger than what JS can handle
-- within pg:  client.query(`SELECT * FROM public.large_data`)

avallete avatar Mar 27 '25 22:03 avallete

thanks for this PR! is it possilbe to write a test for this?

brianc avatar Apr 01 '25 20:04 brianc

@brianc I did my best but had to forge a specific invalid packet to force the parse error (since we can't reproduce the "max array error" in JS really).

I would like to make a more e2e test (ensuring within pg that the pooler/client now emit an error rather than an uncaught exception) but I can't find a way to do this without bumping the pg-protocol dependency. Do you have recommendation for how to proceed in that case ? I'm thinking I could do a commit by binding the "pg-protocol" lib to the local one so you can checkout and ensure it run. Keep the test skipped for now, deploy the pg-protocol next version and unskip the test.

Edit:

I want ahead and created the test in this commit: https://github.com/brianc/node-postgres/pull/3409/commits/65e7882e39b27b9b597c1bec4d0e58d9f15e6d79

Can be tested locally by checking out on the branch, rebuilding pg-protocol, re-instaling deps for pg and running the tests.

Commented on the next commit, can be enabled once the new release of pg-protocol land.

avallete avatar Apr 02 '25 16:04 avallete

Also, #2653’s case can be a query error, same as if a type parser failed. It doesn’t need to be treated like a protocol error.

charmander avatar Apr 03 '25 02:04 charmander