Skip to content

Commit

Permalink
Fix FOR UPDATE OF with explicit schema (knex#5053)
Browse files Browse the repository at this point in the history
When the _tableNames helper was written there were multiple methods
hitting it, but they've since been merged into the single _lockingClause
method. Given that the only really interesting thing it was doing was
prepending the schema name--which is the incorrect behavior this commit
fixes--it didn't seem worth keeping at this point.

I'm not sure you'd say `.withSchema('public')` in a real query but it
lets me exercise the behavior in question without having to learn enough
about knex's test environment to stand up a discrete schema. I suppose
it's plausible that a reasonable maintainer would go "hm that's silly
and redundant" and inadvertently remove coverage for this bug, though.
  • Loading branch information
ErinCall committed Jan 19, 2024
1 parent 3764ded commit d69d9dc
Show file tree
Hide file tree
Showing 2 changed files with 5 additions and 21 deletions.
23 changes: 3 additions & 20 deletions lib/dialects/postgres/query/pg-querycompiler.js
Expand Up @@ -197,29 +197,12 @@ class QueryCompiler_PG extends QueryCompiler {
}
}

// Join array of table names and apply default schema.
_tableNames(tables) {
const schemaName = this.single.schema;
const sql = [];

for (let i = 0; i < tables.length; i++) {
let tableName = tables[i];

if (tableName) {
if (schemaName) {
tableName = `${schemaName}.${tableName}`;
}
sql.push(this.formatter.wrap(tableName));
}
}

return sql.join(', ');
}

_lockingClause(lockMode) {
const tables = this.single.lockTables || [];

return lockMode + (tables.length ? ' of ' + this._tableNames(tables) : '');
return lockMode + (tables.length
? ' of ' + tables.filter(Boolean).map(table => this.formatter.wrap(table)).join(', ')
: '');
}

_groupOrder(item, type) {
Expand Down
3 changes: 2 additions & 1 deletion test/integration2/query/select/selects.spec.js
Expand Up @@ -993,7 +993,7 @@ describe('Selects', function () {
try {
await knex.transaction((trx) => {
// select all from two test tables and lock only one table
return trx('test_default_table')
return trx.withSchema('public').from('test_default_table')
.innerJoin(
'test_default_table2',
'test_default_table.tinyint',
Expand All @@ -1010,6 +1010,7 @@ describe('Selects', function () {
});
});
} catch (err) {
console.log(err.message);
expect(err.message).to.be.contain(
'Defined query timeout of 100ms exceeded when running query'
);
Expand Down

0 comments on commit d69d9dc

Please sign in to comment.