Skip to content
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

How to use raw() in returning? #2571

Open
qwang07 opened this issue Apr 16, 2018 · 9 comments
Open

How to use raw() in returning? #2571

qwang07 opened this issue Apr 16, 2018 · 9 comments

Comments

@qwang07
Copy link

qwang07 commented Apr 16, 2018

Environment

Knex version: 0.14.6
Database + version: PostgreSQL 10.0
OS: Debian 9

How should I use knex.raw() in returning() function? The output of those two statements are the same:

knex('users').insert({ id: 1, firstname: 'test', lastname: 'test' }).returning(['id', 'firstname', 'lastname']).toString()
knex('users').insert({ id: 1, firstname: 'test', lastname: 'test' }).returning(knex.raw('"id", "firstname", "lastname"')).toString()

However, the first one can return the data correctly, the second will return [ undefined ]. Because I want to use SQL function concat_ws() in returning statement, I have to use raw() to create the query.

@elhigu
Copy link
Member

elhigu commented Apr 20, 2018

You are using it correctly (except from using ?? bindings for quoting column names) and since output of the queries is the same they do return the same result.

You have some other problem in your code, please provide complete reproduction code for your problem (with db initialization etc.). https://github.com/tgriesser/knex/blob/master/CONTRIBUTING.md#what-is-minimal-code-to-reproduce-bug-and-why-i-have-to-provide-that-when-i-can-just-tell-whats-the-problem-is

@elhigu elhigu closed this as completed Apr 20, 2018
@qwang07
Copy link
Author

qwang07 commented Apr 20, 2018

My .env file:

PGHOST=localhost
PGPORT=5432
PGUSER=qwang
PGPASSWORD=qwang123
PGDATABASE=test_db

ADMIN_USER=db_admin
ADMIN_PASS=Admin123
ADMIN_MOBILE=0000000000
ADMIN_EMAIL=qwang0617@gmail.com

Here is the test file:

const { createHash, createSalt, head, prop } = require('lib/utils')
const db = require('knex')({
  client: 'pg',
  connection: {
    host: process.env.PGHOST,
    user: process.env.PGUSER,
    password: process.env.PGPASSWORD,
    database: process.env.PGDATABASE,
    port: process.env.PGPORT
  },
})

const salt = createSalt()
const user1 = {
  username: 'test',
  hash: createHash('pwd', salt),
  salt,
  type: 1,
  store: null,
  contact: 1,
}

const user2 = {
  username: 'test2',
  hash: createHash('pwd', salt),
  salt,
  type: 1,
  store: null,
  contact: 1,
}

const s = async () => {
  console.log(db('users')
    .insert(user1)
    .returning(db.raw(`??`, [
      [
        'id',
        'username',
        'type',
        'locked',
      ]
    ])).toString())

  const res1 = await db('users')
    .insert(user1)
    .returning(db.raw(`??`, [
      [
        'id',
        'username',
        'type',
        'locked',
      ]
    ]))

  console.log('Result for first one:', res1);


  console.log(db('users')
    .insert(user2)
    .returning([
      'id',
      'username',
      'type',
      'locked',
    ]).toString());

  const res2 = await db('users')
    .insert(user2)
    .returning([
      'id',
      'username',
      'type',
      'locked',
    ])

  console.log('Result for second one:', res2)
}

s().then(console.log).catch(console.error)

That is the output of console:

insert into "users" ("contact", "hash", "salt", "store", "type", "username") values (1, 'de5e35c5632e273e84d1ea86059606624bbb86f2ce2b37cb8d7ee24d64e71c4614306f76b9bf19fa530642a0052c50b25455fbf1ec36237788564b357a789c2d8d94a09b267931bdd18d4db78c371419ac1cede76b2e8fa9475537207be82865287031679b9c195aa2a1f8fc2ad3ef83e9239c197a538f278fb7d2e6ebb03ae8', 'deadb6e1d2ee792d3d99565b', NULL, 1, 'test') returning "id", "username", "type", "locked"
Result for first one: [ undefined ]
insert into "users" ("contact", "hash", "salt", "store", "type", "username") values (1, 'de5e35c5632e273e84d1ea86059606624bbb86f2ce2b37cb8d7ee24d64e71c4614306f76b9bf19fa530642a0052c50b25455fbf1ec36237788564b357a789c2d8d94a09b267931bdd18d4db78c371419ac1cede76b2e8fa9475537207be82865287031679b9c195aa2a1f8fc2ad3ef83e9239c197a538f278fb7d2e6ebb03ae8', 'deadb6e1d2ee792d3d99565b', NULL, 1, 'test2') returning "id", "username", "type", "locked"
Result for second one: [ anonymous { id: 11, username: 'test2', type: '1', locked: false } ]

@elhigu
Copy link
Member

elhigu commented Apr 25, 2018

@qwang07 thanks, this test case seems really interesting. Try to print queries with .toSQL().toNative() instead of .toString() to get the most accurate info about what is being sent to DB driver in each case.

@elhigu elhigu reopened this Apr 25, 2018
@qwang07
Copy link
Author

qwang07 commented Apr 26, 2018

The code is as same as above except .toString() replace with .toSQL().toNative(). And the result is as same as before.

{ sql: 'insert into "users" ("contact", "hash", "salt", "store", "type", "username") values ($1, $2, $3, $4, $5, $6) returning "id", "username", "type", "locked"',
  bindings:
   [ 1,
     'c7f444de2bbd0e6fa41e765a81a5123d455d2b09d42468b0dcc2ec11bafe63796a8ec0c664ca4ef1589b2acece09052a066c32a9b0b8fed419618ad678a8b64cc78717d7cd182b8c0dde05737d25ec8549495a1681d500b75598fdc0e3fab70e7988f166dd05ced79ac84e7ae0166c901f5c6e0f274d7a2fd7f2d14d01fead8a',
     '58adb5802643d1ff82985f82',
     null,
     1,
     'test' ] }
Result for first one: [ undefined ]
{ sql: 'insert into "users" ("contact", "hash", "salt", "store", "type", "username") values ($1, $2, $3, $4, $5, $6) returning "id", "username", "type", "locked"',
  bindings:
   [ 1,
     'c7f444de2bbd0e6fa41e765a81a5123d455d2b09d42468b0dcc2ec11bafe63796a8ec0c664ca4ef1589b2acece09052a066c32a9b0b8fed419618ad678a8b64cc78717d7cd182b8c0dde05737d25ec8549495a1681d500b75598fdc0e3fab70e7988f166dd05ced79ac84e7ae0166c901f5c6e0f274d7a2fd7f2d14d01fead8a',
     '58adb5802643d1ff82985f82',
     null,
     1,
     'test2' ] }
Result for second one: [ anonymous { id: 3, username: 'test2', type: '1', locked: false } ]

@elhigu
Copy link
Member

elhigu commented Apr 27, 2018

If anyone has time to debug and see whats happening inside knex that is causing that help would be very welcome. I don't have time to start debugging this right now.

@wubzz
Copy link
Member

wubzz commented Jul 13, 2018

In 0.14.6 I got the same undefined message. In 0.15.0 it instead returns the id (In my text case I made it a serial field) property due to changes in #2642.

insert into "users" ("contact", "hash", "salt", "store", "type", "username") values (1, 'asffasasf', 'asdsasdasda', NULL, 1, 'test') returning "id", "username", "type", "locked"
Result for first one: [ 7 ]
insert into "users" ("contact", "hash", "salt", "store", "type", "username") values (1, 'asdasddassda', 'asdsasdasda', NULL, 1, 'test2') returning "id", "username", "type", "locked"
Result for second one: [ anonymous { id: 8, username: 'test2', type: 1, locked: null } ]

The thing here is that calling any of .toString() / .toSQL() / .toNative() will parse the Raw statement onto the generated SQL string, which in essence is perfectly fine. But in reality the problem occurs when processResponse is called to try and parse this returning variable when it is Raw.

As can be seen here it does not and cannot handle when returning is an instance of Raw. So as per #2642 it instead defaults to what is supposed to be "the only column in the row", eventhough there are more than one.

I think there are two ways of handling this:

  • Always return the whole object
  • Don't support knex.raw in returning

I like the first option better. That way no matter how the returning part of the sql string is generated (strings, arrays, raws, etc) the returned object is what the database actually returns.

So

      var returns = [];
      for (var i = 0, l = resp.rows.length; i < l; i++) {
        var row = resp.rows[i];
        if (returning === '*' || Array.isArray(returning)) {
          returns[i] = row;
        } else {
          // Pluck the only column in the row.
          returns[i] = row[(0, _keys2.default)(row)[0]];
        }
      }

Simply turns into

      var returns = [];
      for (var i = 0, l = resp.rows.length; i < l; i++) {
        var row = resp.rows[i];

        returns[i] = typeof returning === 'string' ? row[returning] : row;
      }

I haven't tested this so maybe I'm missing some edge-case, but essentially whatever is returned from the database is what would be added to the response.

@wubzz
Copy link
Member

wubzz commented Oct 5, 2018

@tgriesser What's your take on this? Do you see any reason why knex should be looking at the returning value from builder once the response from SQL has been received?

My mindset is always postgres, so maybe I'm missing something relating to other dialects. But I would like to think whatever was returned from database using builder's returning value should be correct without knex doing any extra checks..

@axelsomerseth
Copy link

Hello there. I came here because I searched about an open "good first issue, " and it looks like there is a Pull Request merged.

So, this issue is still open or it should be marked as closed/solved?

@milen-softavail
Copy link

I've done it like that:

... .returning(db.raw("\"toEmail\", (xmax::text <> '0') AS updated")) ...

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

No branches or pull requests

5 participants