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

.returning('foo') returns primary key instead of 'foo' column in sqlite3 #1660

Closed
jmarthernandez opened this Issue Sep 12, 2016 · 16 comments

Comments

Projects
None yet
9 participants
@jmarthernandez

jmarthernandez commented Sep 12, 2016

I have a node app that I switch back and forth between sqlite3 and postgres.

When I my npm script for using sqlite3 I get back a primary key rather than the column I specified.
Postgres is returning the correct column.

No matter what I put in for .returning('*'/['uuid']/'name' .....) I seem to be getting the primary key in sqlite3.

This is how the docs describe the intended behavior.

// Returns [ { id: 1, title: 'Slaughterhouse Five' } ]
knex('books')
  .returning(['id','title'])
  .insert({title: 'Slaughterhouse Five'})

// Returns [ { id: 1, title: 'Slaughterhouse Five' } ]
knex('books')
  .returning(['id','title'])
  .insert({title: 'Slaughterhouse Five'})

The Query in Question

add: function (d, results) {
    return db('output')
      .returning('uuid')
      .insert({
        classifiers: JSON.stringify(d.classifiers),
        created: now(),
        datasource: d.datasource,
        files: JSON.stringify(d.files),
        name: d.name,
        results: results,
        uuid: uuid.v4()
      })
      .then((ret) => ret[0])
  },

Screenshot of data
screen shot 2016-09-12 at 2 20 08 pm

Config File

var prod = process.env.PROD_CONN_STRING,
  dev = process.env.DEV_CONN_STRING,
  test = process.env.TEST_CONN_STRING;

module.exports = {

  test: {
    client: 'pg',
    connection: test,
    migrations: {
      tableName: 'knex_migrations'
    }
  },

  demo: {
    client: 'sqlite3',
    connection: {
      filename: './tcu.sqlite'
    }
  },

  dev: {
    client: 'pg',
    connection: dev,
    migrations: {
      tableName: 'knex_migrations'
    }
  },


  prod: {
    client: 'pg',
    connection: prod,
    pool: {
      min: 2,
      max: 10
    },
    migrations: {
      tableName: 'knex_migrations'
    }
  }
};

This might be a bug or perhaps the docs are lacking. Either way I can try to take a look at it if a collaborator can point me in the right direction.

@tgriesser

This comment has been minimized.

Owner

tgriesser commented Sep 12, 2016

Sorry for the confusion. At the beginning of returning docs, the first line reads "Utilized by PostgreSQL, MSSQL, and Oracle databases, the returning method specifies which column should be returned by the insert and update methods."

I guess we could be a little clearer in the wording there, but the implication is that this feature isn't supported by sqlite3.

We could also look into throwing an error to fail when trying to use this feature on an unsupported client. Thoughts?

@tgriesser tgriesser added the question label Sep 12, 2016

@jmarthernandez

This comment has been minimized.

jmarthernandez commented Sep 12, 2016

Ah I see that makes sense. I think an error might be helpful for people in the future. This would simply trigger the .catch?

I actually miscopied what is in the docs. It actually says:

// Returns [2] in "mysql", "sqlite"; [2, 3] in "postgresql"
knex('books')
  .returning('id')
  .insert([{title: 'Great Gatsby'}, {title: 'Fahrenheit 451'}])


Outputs:
insert into `books` (`title`) values ('Great Gatsby'), ('Fahrenheit 451')

So the sqlite mention confused me. Removing that part might be more obvious.

Thanks for the quick response!

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Sep 14, 2016

An error would be nice to have. This is not the first time that people has been confused with features, which are supported only by some db engines.

@thetutlage

This comment has been minimized.

thetutlage commented Jun 16, 2017

Not sure if error should be really be thrown, since it makes hard to generic libraries/wrappers over knex. The wrapper implementation will have to check the connection config before chaining the returning method.

Also people get confused, since they ignore the documentation text above the code blocks 😄

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Jun 20, 2017

@thetutlage I don't see how creating erroneus query is a better choice than throwing and error and yes generic wrapper should make sure that underlying DB supports the .returning statement before calling that builder method or otherwise it wont return correct results.

If wrapper wants to be really generic, it won't use .returning feature at all and just emulates it by fetching the date with select or some other portable way (or some specialised implementation for each dialect).

I'm not a big fan of the historical-JS habit of allowing users to do invalid code with silent errors / noop functionality.

@wubzz

This comment has been minimized.

Collaborator

wubzz commented Feb 15, 2018

Have we reached a conclusion to this issue? Throwing an error, silently ignorning, and printing a warning during query compilation are all valid options imo.

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 15, 2018

I would propose following:

  1. add warning for all of these old places where people are using query builder methods which are just not supported by used dialect.
  2. Afterwards we can consider changing all those warnings to errors and do bigger break to backwards compatibility at once.
  3. For new APIs error should be throws for the unsupported dialects (I've been requiring this in PRs already for a while).
@wubzz

This comment has been minimized.

Collaborator

wubzz commented Feb 15, 2018

Aside from the issue at hand I don't know methods by hand that aren't supported by specific dialects, so in the spirit of not escalating the issue any further, I will only add a warning for .returning to sqlite.

Knex:warning - .returning() is not supported by sqlite3 and will not have any effect.

#2471

@wubzz wubzz closed this in f1faaf9 Feb 15, 2018

@larryprice

This comment has been minimized.

larryprice commented Feb 20, 2018

As of 0.14.4, the warning is printed every time I run migrate() in test even though the returning function is never used in my code?

@cvlmtg

This comment has been minimized.

cvlmtg commented Feb 21, 2018

I have an app using a psql database and some unit tests which use mock-knex and thus set sqlite as the backend. Since my upgrade to knex 0.14.4 the unit tests display this output:

    ✓ obtains a lock and process a book (54ms)
Knex:warning - .returning() is not supported by sqlite3 and will not have any effect.     
Knex:warning - .returning() is not supported by sqlite3 and will not have any effect.     
Knex:warning - .returning() is not supported by sqlite3 and will not have any effect.     

I know that returning() doesn't work with sqlite (it's written in the doc) and my tests already take that into account (even if actually it doesn't matter, since I'm mocking the queries) so it would be nice to have a way to suppress the warning if possible (couldn't find anything in the docs). thanks!

@GeekyDeaks

This comment has been minimized.

GeekyDeaks commented Feb 26, 2018

Hey chaps. This is fine, but I found that the mssql dialect does not return the primary key when you leave off the .returning() during an insert, so I was using it to get the primary key across both DB types, even though it was technical not supported by sqlite3. Is the lack of primary key returned with an insert in mssql by design or is it ok to add it?

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 26, 2018

@GeekyDeaks Also postgresql does that... Goog to know that mssql does that also. I've been meaning to make proposal about changing .insert({}) functionality in a way that it would be cross db compatible somehow (by adding some implicit returns for postgresql and mssql or somtehing like that).

Anyways I need to discuss about it with other contributors first. Currently I've been just fixing other mssql/tedious driver stability problems.

@GeekyDeaks

This comment has been minimized.

GeekyDeaks commented Feb 26, 2018

Thanks @elhigu - would you implement it in the processReponse() of the dialect? i.e. like in dialect/sqlite3/index.js

      case 'insert':
        return [ctx.lastID];

If so, I might take a look at this over the weekend and submit a PR. I'm only just getting up to speed with knex though, so it might take me a little longer 😄

@elhigu

This comment has been minimized.

Collaborator

elhigu commented Feb 26, 2018

Process response is not enough, since also built query needs to be modified, I don't think this can be implemented before it has been properly discussed first. There are some benefits/drawback that needs to be considered.

@GeekyDeaks

This comment has been minimized.

GeekyDeaks commented Feb 26, 2018

ah, yeah - was poking around under the debugger and noticed that. I can workaround by checking the client dialect for now.

@chaim1221

This comment has been minimized.

chaim1221 commented May 1, 2018

That's great; but how do you get the ID of a created, updated, or deleted record in Sqlite3?

...Never mind. It's in the docs.

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