brianc
fix: use existing Result types for new Result
Context
Closes https://github.com/brianc/node-postgres/issues/3309 Related to https://github.com/sequelize/sequelize/issues/17479
Approach
The issue seems to be that the _types member of Result is directly overwritten by Client.query:
https://github.com/brianc/node-postgres/blob/54eb0fa216aaccd727765641e7d1cf5da2bc483d/packages/pg/lib/client.js#L567
However, in _checkForMultiRow, we then create new Results, but without the overridden types:
https://github.com/brianc/node-postgres/blob/54eb0fa216aaccd727765641e7d1cf5da2bc483d/packages/pg/lib/query.js#L68
The approach is to reuse the types of the original Result when creating a new one.
Thanks for proposing a fix.
Client probably shouldn’t be reaching into Query and Result like that in the first place, and should forward its types to the query objects it creates instead. That would be a breaking change now, when passing query objects to Client#query, but maybe it’s possible to add the types to the options now to fix this bug and defer removing query._result._types = this._types until the next major version? @brianc
(Either way, it should also have a test.)
Got it, I'll add a test. I'll try to find some time to work on it over the next couple of days.
Just checking, are there any further changes I should make? No rush, just wondering if there's anything I can do to help!
Not a huge fan of reaching into the client like that, but also I think I went a bit overboard with what was "private" in the past (particularly since not a lot is really actually private in JS). I'm okay w/ this as a quick fix for now, and since there are tests covering it it will be resilient to refactoring when that time comes. :)
Seems like tests are failing for pg-native in NodeJS 20 and 22. Do you think this is related to the changes, or might it be that the tests are flaky? Noticed a recent test run for an unrelated change that failed with a similar error code: https://github.com/brianc/node-postgres/actions/runs/10098627832/job/27926116837?pr=3271
Happy to debug further if you think the changes caused the test to fail.
Hey @brianc @charmander! The tests are passing for me locally on both Node 20 and 22 (see screenshots). Do you think it might be worth rerunning the actions to see if the test failures might be due flaky tests? Alternatively let me know if you think I need to debug further.
Do you think it might be worth rerunning the actions to see if the test failures might be due flaky tests?
I'll re-run! Once tests pass I'll merge this. sorry for the delay, this looks good. :)
This has to do w/ randomly segfaulting (sometimes, but not regularly) in the native bindings, which is quite annoying. I'm unable to re-run the job? I don't see the button anywhere anyway. Mind pushing an empty or just whitespace commit & hopefully that'll trigger a re-run?