1227: add assertion for basic where clause values

[littlemaneuver]

Meant to mitigate #1227

Checked also whereJsonObject and it throws the binding error if nested object is given. So this case is handled, but in other way. I can add the other assert there as well to do smth like Object.values(object).some(isObject) so the interface is unified somewhat.

[r2dev2]

#1227 still applies if value is 0 the integer.

Adding value.toString() instead of value when pushing onto the where statement stack should solve the issue.

[lauripiisang]

boolean value false , if not passed as a string, yields the same result (talking about MySQL here, don't know whether it applies to #1227 ) https://www.db-fiddle.com/f/w8CJ5SAYEKwJqAXyoYPSwC/2

[r2dev2]

Instead of "= 1", it should be "= `1`" because for mysql, if = 0 is used, it may match other values. I'm not sure if it should be this way for other databases though, can someone versed in the other databases comment on this?

[lauripiisang]

I tested this on db-fiddle, postgre does not allow comparisons between types with = operator and expectedly returns an error, so that should be fine.

[littlemaneuver]

Yeah, this is scary but it is rather a MySQL issue than a library concern. Or at least this is a separate case with the type coercion in MySQL while this pr handles the invalid usage of the where clause with objects as plain values.

[r2dev2]

Yeah, this is scary but it is rather a MySQL issue than a library concern.

I disagree that this is not a library concern. The purpose of knex is to abstract away database-specific quirks to allow safe query-building. Since mysql has the quirk of "= 0" matching all values, knex is responsible for escaping it to "= `0`" to represent the developer's intent.

Or at least this is a separate case with the type coercion in MySQL while this pr handles the invalid usage of the where clause with objects as plain values.

Should adding backticks around values for mysql be added in a different pr then? Two prs addressing the fundamental issue of knex not expecting non-strings through assertions and escaping seems unnecessarily complex, error-prone, and time-consuming compared to calling .toString() on inputs.

[lauripiisang]

Since mysql has the quirk of "= 0" matching all values, knex is responsible for escaping it to "= 0" to represent the developer's intent.

@r2dev2 there are some issues with this.
1 - backticks for values is invalid syntax in MySQL, backticks should only be used for identifiers.
2 - when we consider quotes instead of backticks, even this is a bit contentious, I thought it would break with integers, but MySQL is full of surprises. = 0 is perfectly valid in case of integers, and for reasonable types, probably should be used.
Here are the related tests: https://www.db-fiddle.com/f/vytxD1kFa3xBiPf7TBADxq/4

[r2dev2]

1 - backticks for values is invalid syntax in MySQL, backticks should only be used for identifiers

Ah, it appears I got a bit confused with previous experiments. It is the quote that should be used for values, not backtick. I added a quote escaping test to your fiddle (so it does = '0' instead of = 0 for secret column) and that works properly.

Fiddle: https://www.db-fiddle.com/f/vytxD1kFa3xBiPf7TBADxq/5

Edit: I added db-fiddle tests demonstrating quote-escaping works for postgress and sqlite as well below

Postgres: https://www.db-fiddle.com/f/w8CJ5SAYEKwJqAXyoYPSwC/3

Sqlite: https://www.db-fiddle.com/f/w8CJ5SAYEKwJqAXyoYPSwC/4

[littlemaneuver]

The change is not simple toString. In raw queries you need to convert bindings to string as well, while the collection of bindings could be either array or object (named bindings)

[r2dev2]

The change is not simple toString. In raw queries you need to convert bindings to string as well, while the collection of bindings could be either array or object (named bindings)

Yea, I tried calling .toString() at various places in the codebase and it seems like calling .toString() on all the bindings is not a solution as a binding could map to many different situations such as limit = ?

Perhaps only calling .toString() for where = clauses and other operators where the value can be quoted is a solution? One dirty way I did this was by adding

whereBasic(statement) {
  if (['number', 'boolean', 'array', 'object'].includes(typeof statement.value)) {
    statement.value = statement.value.toString();
  }
  return super.whereBasic(statement);
}

to lib/dialects/mysql/query/mysql-querycompiler.js

However, I'm sure there is a better way of doing this which someone more familiar with the knex codebase than me can think of.

[coveralls]

Coverage: 92.369% (-0.001%) from 92.37% when pulling 0a5efcc on antoniopuero:1227-not-accepting-objects-in-where-clause into 962bb0a on knex:master.

[OlivierCavadenti]

Thanks for the PR !
Check CI + tests

[sofalse]

Thanks for fixing this!

[nathanburgess]

What's the timeline on having this merged in?

[TimeTerminal]

Merging is blocked until @OlivierCavadenti is able to review it.

[TimeTerminal]

Nit: This should say "empty"

[OlivierCavadenti]

@kibertoad maybe we could release a version now ?

[kibertoad]

Sure, I'll release it in the morning

[piyushchauhan2011]

Cool

[sgarbesi]

@kibertoad Thank you.

[Ockejanssen]

This fix breaks
where('column', new Date())