knex / knex

A query builder for PostgreSQL, MySQL, CockroachDB, SQL Server, SQLite3 and Oracle, designed to be flexible, portable, and fun to use.
https://knexjs.org/
MIT License
19.36k stars 2.12k forks source link

.delete() breaks escaping of literals supplied to onVal() #6124

Open ChrisCrossKudos opened 2 months ago

ChrisCrossKudos commented 2 months ago

Environment

Knex version: 2.5.1 and later Database + version: Postgres 15.5 OS: Mac & Alpine linux

Bug

db // Knex instance

// Basic query that includes a join with literal join value
const qb = db('table_a').innerJoin('table_b', join => {
  join.onVal('table_b.id', '123')
})

// Executing select on this query results in proper formatting of literal value
// SQL: select "table_a"."id" from "table_a" inner join "table_b" on "table_b"."id" = '123'
console.log(qb.select('table_a.id').toQuery())

// Executing delete on this query results in literal value being formatted as if it were a column name
// SQL: delete from "table_a" using "table_b" where "table_b"."id" = "123"
console.log(qb.delete().toQuery())

In select queries, onVal, andOnVal etc. work as expected and documented. The supplied second value is treated as a literal value and, in the case of strings, is quoted with ' single quotes. If the same query syntax is used in a delete() query, the value is suddenly treated as a column name and not a value and is formatted with " double quotes. This obviously breaks the execution because no column exists with that name.

It should be noted that this issue only seems to affect string values. Using the value 123 instead of '123' works OK. However this is no use if your database is using UUID identifiers.

Feature discussion / request

Use case:

Workaround

We can work around this by wrapping the value in raw('?', '123') but that defeats the point of the onVal interface. For now we are just doing join.on('table_a.id', db.raw('?', '123'))

Missing / erroneus documentation

In case it helps, this was the PR that introduced these methods: https://github.com/knex/knex/pull/2746

YongSeok-Choi51 commented 1 month ago

i created a Fix PR!

please consider at

ChrisCrossKudos commented 1 month ago

Thank you so much for helping with this. Looking forward to it merging 🤞