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

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

Closed
jmarthernandez opened this issue Sep 12, 2016 · 16 comments
Closed
Labels

Comments

@jmarthernandez
Copy link

@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
Copy link
Member

@tgriesser 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
Copy link
Author

@jmarthernandez 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
Copy link
Member

@elhigu 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
Copy link
Contributor

@thetutlage 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
Copy link
Member

@elhigu 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
Copy link
Member

@wubzz 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
Copy link
Member

@elhigu 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 added a commit to wubzz/knex that referenced this issue Feb 15, 2018
@wubzz
Copy link
Member

@wubzz 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
Copy link

@larryprice 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
Copy link

@cvlmtg 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
Copy link

@GeekyDeaks 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
Copy link
Member

@elhigu 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
Copy link

@GeekyDeaks 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
Copy link
Member

@elhigu 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
Copy link

@GeekyDeaks 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
Copy link

@chaim1221 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
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
9 participants