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.24k stars 2.12k forks source link

Using '.where' with 'knex.raw' statements and '.fetchOne' from bookshelf 0.8.1 causes an error. #902

Closed wilsontgh closed 8 years ago

wilsontgh commented 9 years ago
{ [error: select "cm_vehicles".* from "cm_vehicles" where details ->> 'last_fine_check' < $1 or (details ->> 'last_fine_check') is null order by (details ->> 'last_fine_check') ASC limit $2 - bind message supplies 1 parameters, but prepared statement "" requires 2]
  name: 'error',
  severity: 'ERROR',
  code: '08P01',
  detail: undefined,
  hint: undefined,
  position: undefined,
  internalPosition: undefined,
  internalQuery: undefined,
  where: undefined,
  file: 'postgres.c',
  line: '1500',
  routine: 'exec_bind_message' }

Here are the statements:

qb.where(knex.raw("details ->> 'last_fine_check' < ?", oneDayAgo))
            .orWhere(knex.raw("(details ->> 'last_fine_check') is null"))
            .orderBy(knex.raw("(details ->> 'last_fine_check')"), "ASC");

Model.Vehicle.collection().query(query).fetchOne().then(...)

This happens if I use knex.js 0.8.6 but does not happen in knex.js 0.7.6.

rhys-vdw commented 9 years ago

Hi @wilsontgh, sorry for the late reply.

The problem here is that .raw requires its bindings in an array.

This should work:

qb.where(knex.raw("details ->> 'last_fine_check' < ?", [oneDayAgo]))
            .orWhere(knex.raw("(details ->> 'last_fine_check') is null"))
            .orderBy(knex.raw("(details ->> 'last_fine_check')"), "ASC");

An argument could certainly be made for raising an error if a non-array value is supplied as the "bindings" argument for raw. Either way the documentation should be made more clear.

By the way, you can simplify your code like so:

qb.whereRaw("details ->> 'last_fine_check' < ?", [oneDayAgo])
  .orWhereRaw("(details ->> 'last_fine_check') is null")
  .orderByRaw("(details ->> 'last_fine_check') asc");
rhys-vdw commented 9 years ago

Request PR for the following changes:

I considered that .raw could also accept a single value for bindings. However, it is possible to supply an array as one of the bindings, so that would create ambiguity.

xdc0 commented 8 years ago

@rhys-vdw I'm working on a PR for this. I notice there is a test that uses a plain string on a raw query. Throwing an error breaks this test so there seems to be some cases where the raw expression works when passing anything other than an array. raw.js handles cases when the bindings are not an array.

I encountered this problem when passing a date object on the query, it works fine when not using raw but looking at the toSQL method on raw.js it seems that when a plain object is passed, it iterates over the values and handles accordingly, but if you pass anything other than a plain object, it tries to do the same and ends up "silently" failing, I say silently failing because the procedure returns '' when passing objects that are not plain object. I think the right fix would be to check if bindings is a plain object, but I might be missing something. What do you think?

rhys-vdw commented 8 years ago

Closed by #1051.