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.

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

Conflict with Postgresql jsonb operators #519

Closed
metainfa opened this Issue Oct 3, 2014 · 33 comments

Comments

Projects
None yet
@metainfa

metainfa commented Oct 3, 2014

Hi!

I have sql:

SELECT id, name, price, files->'photos'->0->>'name' as photo, annotation
FROM product
WHERE tags ?& array['17']
ORDER BY (tags->>'17')::int
LIMIT 10

If i run this sql direct in postgresql client it work fine

If i run this sql through node-postgres it work fine again:

var pg = require('pg');

var connPg = "postgres://user:password@localhost/mydbname";
var clientPg = new pg.Client(connPg);
var sqlProductsByTags = [
  "SELECT id, name, price, files->'photos'->0->>'name' as photo, annotation ",
  "FROM product ",
  "WHERE tags ?& array['17'] ",
  "ORDER BY (tags->>'17')::int ",
  "LIMIT 10 "
];

clientPg.connect(function(err) {
  if(err) {
    return console.error('could not connect to postgres', err);
  }
  //console.log('SQL = ' + sqlProductsByTags.join(''));

  clientPg.query(sqlProductsByTags.join(''), function(err, result) {
    if(err) {
      return console.error('error select products by tags', err);
    }

    for(var i = 0, max = result.rows.length; i < max; i++) {
      console.log('name: ' +  result.rows[i].name);
    }

    clientPg.end();
  });
});

If i run raw sql by knex, it fail with error (below listed):

var connPg = "postgres://user:password@localhost/mydbname"; 
var pg = require('knex')({
  client: 'pg',
  connection: connPg
});
var sqlProductsByTags = [
  "SELECT id, name, price, files->'photos'->0->>'name' as photo, annotation ",
  "FROM product ",
  "WHERE tags ?& array['17'] ",
  "ORDER BY (tags->>'17')::int ",
  "LIMIT 10 "
];

pg.raw(sqlProductsByTags.join(''))

This Error

{ [error: syntax error (at or near "$1")]
  name: 'error',
  length: 142,
  severity: 'ERROR',
  code: '42601',
  detail: undefined,
  hint: undefined,
  position: '98',
  internalPosition: undefined,
  internalQuery: undefined,
  where: undefined,
  file: 'scan.l',
  line: '1053',
  routine: 'scanner_yyerror' }

I think that it fail because of i used jsonb operator '?&', precise only one symbol '?'
Reference to PostgreSQL JSONB operators

@tgriesser tgriesser added the bug label Oct 3, 2014

@vschoettke

This comment has been minimized.

Collaborator

vschoettke commented Oct 3, 2014

The problem is that the postgres dialect replaces every question mark with $ for the parameters bindings before executing the query in Client_PG.prototype.positionBindings.

The easiest fix is probably changing the regex to only replace occurrences of a question mark at the end or any followed by space, comma with $. @tgriesser any thoughts?

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Oct 3, 2014

That seemed to be what I was thinking as well, though I'm not sure if the issue is complicated by the changes to raw which splits on ? to allow raw queries as binding values from #379

@vschoettke

This comment has been minimized.

Collaborator

vschoettke commented Oct 3, 2014

positionBindings is executed right before the query so no problem for the raw bindings.

@vschoettke

This comment has been minimized.

Collaborator

vschoettke commented Oct 3, 2014

/\?(?=$|[,\)\(\s])/g as regex in positionBindings function will only replace any ? followed by ,,(, ) or any whitespace and solve ?& problem.
However after reading the jsonb prostgres documentation I think this is futile because:

The default GIN operator class for jsonb supports queries with the @>, ?, ?& and ?| operators. (For details of the semantics that these operators implement, see Table 9-41.)

So a single question mark is also a valid operator.

Maybe escaping/replacing the question mark if no param was supplied and converting it back right before execution is a solution.

tgriesser added a commit that referenced this issue Oct 3, 2014

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Oct 3, 2014

@vschoettke take a look at the PR I just opened and see if that's what you were thinking. It basically does what you described, allow for raw queries to have a ? operator so long as there are no bindings in the same raw query. Replaces them temporarily with $Q and then replaces them in the positionBindings.

@vschoettke

This comment has been minimized.

Collaborator

vschoettke commented Oct 3, 2014

@tgriesser your PR looks good to me. If somebody wants to use bindings and a question mark as is then $Q can be used in place of the question mark too. I'd prefer getting rid of the question mark for bindings in the future though. e.g. knex.raw('select * from "test" where id=:id", {id: 1});

@whitelynx

This comment has been minimized.

Contributor

whitelynx commented Jan 15, 2015

One way of fixing this completely (and keep things future-proof for other dialects) would be to generate the SQL text as an array instead of a flat string, starting a new array element at the position of each binding.

For example, instead of:

{ method: 'select',
  options: undefined,
  bindings: [ 3857, 999999 ],
  sql: 'select "geom", ? as "srid" from "map" where ("id" = ?)' }

...we'd have:

{ method: 'select',
  options: undefined,
  bindings: [ 3857, 999999 ],
  sql: ['select "geom", ', ' as "srid" from "map" where ("id" = ', ')'] }

This means that each dialect would get the appropriate placeholders by just calling .join() with the placeholder (or, in the case where an incrementing number is needed in the placeholder, using .reduce()), and this wouldn't interfere with operators like ?.

@ghost

This comment has been minimized.

ghost commented Mar 30, 2015

Any updates on this issue?
will it reach the master ?
I'm also having several problems using postgresql jsob operators.
Thanks.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jul 7, 2015

+1 Is there any workarounds how I could make queries with json ?,?|,?& operators already with latest stable knex version?

@whitelynx

This comment has been minimized.

Contributor

whitelynx commented Jul 9, 2015

I'm also curious whether this will be fixed soon. I'll need this before I can switch my generic web-based query tool over to using knex, since this would prevent the user from being able to use the JSONB operators.

@tgriesser Your PR looks fine for now, since it would provide a workaround for the time being. Eventually, I think it might make more sense to switch to something like I suggested above, but for now I think it'd be useful to at least get some fix merged.

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Jul 9, 2015

Hi @whitelynx. I believe @tgriesser is currently away but I'm happy to get a fix merged. If someone who knows the code base is able to review and update the PR as necessary, I'm happy to facilitate by merging it into master.

@DenisGorbachev

This comment has been minimized.

DenisGorbachev commented Jul 13, 2015

@tgriesser +1, I use JSONB all over the place :)

@underbluewaters

This comment has been minimized.

underbluewaters commented Jul 22, 2015

This is also a problem with hstore methods.

    ELSIF hstore(NEW) ? 'is_bycatch' THEN
@zacronos

This comment has been minimized.

Contributor

zacronos commented Jul 25, 2015

This is also a problem when using knex.raw on a constant which includes a ?. Below is an oversimplified example that is easy to work around, but in my real situation I can't avoid the knex.raw().

Code:

console.log(knex.table('blah').insert({col1: knex.raw("'http://www.google.com/?q=foobar'"), col2: {a:1}}).toString());

SQL:

insert into "blah" ("col1", "col2") values ('http://www.google.com/'{\"a\":1}'q=foobar', ?)
@whitelynx

This comment has been minimized.

Contributor

whitelynx commented Jul 26, 2015

@zacronos said:

This is also a problem when using knex.raw on a constant which includes a ?. Below is an oversimplified example that is easy to work around, but in my real situation I can't avoid the knex.raw().

I believe the correct way to do that with the current version of knex is to provide it as a parameter (then it is automatically escaped correctly, as well as avoiding this issue):

console.log(knex.table('blah').insert({col1: knex.raw('?', [' http://www.google.com/?q=foobar']), col2: {a:1}}).toString()); 
@zacronos

This comment has been minimized.

Contributor

zacronos commented Jul 27, 2015

@whitelynx, ah, that does seem to work. Thanks!

@eknkc

This comment has been minimized.

eknkc commented Aug 11, 2015

This has been a big issue for us. Have recently started to use jsonb columns. Any update on the PR?

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Aug 11, 2015

@eknkc There is an existing PR. Not sure what the hold up is.

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Aug 11, 2015

Actually looks like it's passing, but I don't know the code base well enough to review it.

@zacronos

This comment has been minimized.

Contributor

zacronos commented Aug 11, 2015

@rhys-vdw, the PR passes tests, but it has merge conflicts.

I took a look myself earlier to see what it would take, and unfortunately the code involved has changed quite a bit since the PR was created. I believe at least one of the modified files no longer even exists. It's possible it would be easier for someone to create a new PR from scratch rather than try to merge the existing one, but I don't know the codebase, so I can't say for sure.

@eknkc

This comment has been minimized.

eknkc commented Aug 11, 2015

The JDBC driver seems to have solved this using ?? as an escape for literal ?: pgjdbc/pgjdbc#227
One could write this in that case

knex.raw("SELECT * FROM items WHERE tags ?? ?", ["tagname"])

for

SELECT * FROM items WHERE tags ? 'tagname'
@underbluewaters

This comment has been minimized.

underbluewaters commented Aug 11, 2015

I would expect a more general purpose escaping scheme like \? or ?? would be better than just special-casing question mark operators.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Aug 13, 2015

+1 for ??

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Aug 14, 2015

Except, looks like ?? is already used for wrapping binding value to be used as an identifier e.g.

raw("??", ['TableName.colName'] => "TableName"."colName"
raw("?", ['TableName.colName'] => 'TableName.colName'

https://github.com/tgriesser/knex/blob/master/test/tape/raw.js#L12

@rhys-vdw

This comment has been minimized.

Collaborator

rhys-vdw commented Aug 16, 2015

Good catch, @elhigu.

Perhaps \? would be more appropriate then?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Aug 24, 2015

Looks like we need to do \? actually with \\? since javascript requires that we escape \ char in string. However I added pull request with implementation, test and documentation. Should I run also npm run build and add commit the generated code as well?

@jamoy

This comment has been minimized.

jamoy commented Nov 12, 2015

I know it's been a couple of months but I am still in this predicament on 0.9

can anyone let me know what is going on with this query?

qb.whereRaw("tags \\?& array['" + Array(tags.length + 1).join("?").split("").join("','") + "']", tags)

the error i get is

select "id" from "tablename" where tags \$1 array['foo','bar'] and "locale" = $2 order by "created_at" desc limit $3 offset $4 - syntax error at or near "\"

It seems the escape sequence doesn't get applied in any case. I would appreciate it if anyone can give light to this.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 12, 2015

I'm getting same kind of results... looks like e.g. when one calls .toString() for query builder, it cannot handle //? escaping correctly.

knex.from('test_table_two').whereRaw("(json_data->'me')::jsonb \\?& array['keyOne', 'keyTwo']").where('id', '>', 1).toString()
select * from "test_table_two" where (json_data->\'me\')::jsonb \\\'1\'& array[\'keyOne\', \'keyTwo\'] and "id" > ?

Normal case when one just executes query it goes a bit different route to pg and ? replacement is never done by knex, but they are just converted to $1, $2 etc bindings of prepared sql statement for postgres.

I'll write test for this case so that //? escaping is handled for .toString() replacements too.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Nov 13, 2015

@jamoy I added pull request to fix your issue

@jamoy

This comment has been minimized.

jamoy commented Nov 16, 2015

thank you very much @elhigu wish I knew the codebase enough to contribute. hoping this would get merged soon 👍

@zacronos

This comment has been minimized.

Contributor

zacronos commented Sep 13, 2017

@SeanCannon, in my codebase I saw this working just fine over a year ago after it was fixed. Chances are very good that if you're seeing a similar issue, it is a new problem, not a continuation of this problem.

I suggest you create a new Issue, complete with the version of knex you're using, example code that demonstrates the problem, and output of the error and/or expected vs actual results.

@qinwei-gong

This comment has been minimized.

qinwei-gong commented Jan 12, 2018

?? works!

@m00dy

This comment has been minimized.

m00dy commented Jul 31, 2018

Be sure you have jsonb field but not json !!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment