Skip to content

fix: avoid mutating query config #3720

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.

Sign up for GitHub

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

Merged
brianc merged 3 commits into from
Aug 12, 2026
+58 −1
Merged

fix: avoid mutating query config #3720

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
ea6cf43
fix: don't mutate the caller's query config object
GiHoon1123 Jul 26, 2026
f0bbfcf
test: simplify query config comments
GiHoon1123 Jul 26, 2026
b4d11e4
fix: preserve query config property access
GiHoon1123 Jul 27, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion packages/pg/lib/utils.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -143,7 +143,8 @@ function dateToStringUTC(date) {

function normalizeQueryConfig(config, values, callback) {
// can take in strings or config objects
config = typeof config === 'string' ? { text: config } : config
// Copy config so normalization does not mutate the caller's object.
config = typeof config === 'string' ? { text: config } : { ...config }

@charmander charmander Jul 27, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a potential breaking change for query objects that don’t have own enumerable query config properties. I’d say return an object like { callback: config.callback, values: config.values, config } and have callers access normalizedConfig.config?.otherProperty, but that would be a breaking change for normalizeQueryConfig itself, so… another function?

@GiHoon1123 GiHoon1123 Jul 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, good catch. I switched this away from object spread so it keeps the original prototype and property descriptors while still avoiding mutation of the caller's object.

I added a unit test with an inherited text getter for this case. Also ran make test-unit, yarn lint, and yarn build.

if (values) {
if (typeof values === 'function') {
config.callback = values
Expand Down
14 changes: 14 additions & 0 deletions packages/pg/test/unit/client/simple-query-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -150,4 +150,18 @@ test('executing query', function () {
)
})
})

test('reusing a config object across calls', function () {
// Regression test for https://github.com/brianc/node-postgres/issues/2651.
test('does not leak callback state into a later promise-style call', function () {
const client = helper.client()
const config = { text: 'SELECT $1', values: [1] }

client.query(config, function () {})
const result = client.query(config)

assert.ok(result instanceof Promise, 'expected client.query() to return a Promise')
result.catch(() => {})
})
})
})
12 changes: 12 additions & 0 deletions packages/pg/test/unit/utils-tests.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -33,6 +33,18 @@ test('normalizing query configs', function () {
assert.deepEqual(config, { text: 'TEXT', values: [10], callback: callback })
})

test('normalizeQueryConfig does not mutate the passed-in config object', function () {
// Regression test for https://github.com/brianc/node-postgres/issues/2651.
const original = { text: 'TEXT' }
const callback = function () {}

const normalized = utils.normalizeQueryConfig(original, [10], callback)

assert.equal(original.callback, undefined)
assert.equal(original.values, undefined)
assert.deepEqual(normalized, { text: 'TEXT', values: [10], callback: callback })
})

test('prepareValues: buffer prepared properly', function () {
const buf = Buffer.from('quack')
const out = utils.prepareValue(buf)
Expand Down
Loading