knex
Documentation for returning functionality in MySQL is not clear enough in all cases
Environment
Knex version: 0.16.5 Database + version: MySQL 5.7
Bug
db.insert({data}, 'an_id').into('foobar')
That sort of statement is giving returning() is not supported by mysql and will not have any effect. Note that this statement is not using returning() but is relying on Knex's internal version of it for MySQL.
Can confirm, due to this change obv: https://github.com/tgriesser/knex/pull/3039
@jsumners Why are you passing second param to insert? It is literally a returning argument.
All it does is if (!isEmpty(returning)) this.returning(returning);, so it's pretty much same thing.
According to what I understand, that is the only way to get a result from an insert in mysql dialect (since returning(...) syntax doesnt work?)
No, it's exactly same thing and is unlikely to have any effect either, warning is correct.
await db.connection.default("User").insert({})
[ 202 ]
Oh wow I guess not... so the default is to return an array of ids inserted?
If what I'm seeing in builder tests is correct, unfortunately no, just an array of last id inserted. Likely because LAST_INSERT_ID() is used somewhere (can't find reference in knex, could be mysql driver thing). MySQL is not a great DB.
@jsumners Documentation is not MySQL-specific. Also it says: " It's a shortcut for returning method". And documentation for returning method says this: "Utilized by PostgreSQL, MSSQL, and Oracle databases".
Not a bug, works as designed.
Wow.
- Yes, the documentation is MySQL specific. I highlighted the dropdown that indicates it is so.
- It shouldn't matter what the documentation behind the "returning method" says because this documentation is clearly indicating that Knex will do the right thing for MySQL when given this syntax.
@jsumners OK, documentation is misleading here and needs to be clarified. In MySQL case it is actually likely to return 3 and not 2, and also regardless of returning param. This is definitely not communicated properly.
documentation is clearly indicating that Knex will do the right thing for MySQL when given this syntax. - not really. Fact that it only returns you single ID on MySQL is already a strong hint that it's not doing the right thing, but yes, that should have been explained in much less ambiguous terms.
ugh I feel for you guys, yea very difficult to explain, but thanks for the clarification!
FYI for the curious, the implementation looks to be in https://github.com/mysqljs/mysql#getting-the-id-of-an-inserted-row (though a quick search didnt help me understand the exact spot).
Along those lines, it looks like this is race condition safe (which was my immediate next concern)
I miss postgres 😢
@chaffeqa Thanks for investigating this further! Totally share your feelings towards PostgreSQL 😂
Afaik that dropdown in upper corner only selects dialect that is used to generate output from example builders... so rest of the docs are static and the same for all the dialects (and that is intentional to highlight for everyone those parts of docs where builders are not generation cross db compatible queries). If anyone is up to make this part more clear in docs please do.
This issue could be also closed and moved to docs repo.
@chaffeqa it looks like this is where the returning in happening
https://github.com/tgriesser/knex/blob/a2ab754eedcc1d8fe6328cb40adf0e31b2b3576a/src/dialects/mysql/index.js#L153-L154
But I do not see where it is appending the set ?.
Can we got an ability for disable this warning? Because I use knex with mysql and with returning feature as well. And returning only first inserted id is enough for me.
By the way, why we can just fix the mysql knex dialect with changes below?
Just change this line: https://github.com/tgriesser/knex/blob/a2ab754eedcc1d8fe6328cb40adf0e31b2b3576a/src/dialects/mysql/index.js#L154 to this:
return Array.from({ length: rows.affectedRows }, (v, k) => k + rows.insertId);
@ziggi that warning is there for a reason. You should not use .returning() with mysql. It does not do anything . There is a separate issue for adding crossdb compatible insert to knex https://github.com/tgriesser/knex/issues/2682 ... that could be place to talk about that change. I have to say that I don't remember reasons why knex was never changed to generate those "missing" ids (anyways this thread is not the place to discuss about it).
It does not do anything .
But it's do, with .returning() I can receive first inserted id.
After this feature I can't update knex, I stuck on 0.16.3. It would be nice to have ability for disabling this warning manually.
@ziggi you will get that id also without adding any ‘.returning()’ call there. Check from ‘.insert()’ documentation what it returns by default... as I said, ‘.returning()’ is not supported at all but mysql server and now knex emits a warning because you are trying to use it with wrong dialect.
@elhigu is there any way to suppress that warning? My app is database driver agnostic, and so far, inserts have been the only painpoint where I haven't been able to properly make it database driver agnostic :(
MySQL returns primary keys (but only if they're auto incremented it seems), but others (Postgres) returns a "Result" object coming from Knex. My solution was to do something like
const keys = await knex.insert({ ... }).into(...).returning('id');
let key;
if (typeof keys[0] === number) {
key = keys[0];
} else {
key = keys[0].id;
}
but that keeps throwing warnings about MySQL not supporting returning
@rijkvanzanten probably the easiest and fastest way for you is to write knex plugin, which implements crossdb insert that works in your case.
Something like:
const Knex = require('knex');
Knex.QueryBuilder.extend('insertAndReturnId', function(value) {
const builder = this.insert(value);
if (this.client.driverName === 'pg' || this.client.driverName === 'oracledb') {
builder.returning('id');
}
return builder;
});
const id= await knex('accounts').insertAndeturnId({});
https://github.com/knex/knex/search?q=.returning%28%29+is+not+supported+by+mysql+and+will+not+have+any+effect.
Here is my migration, I want it to run silently. Please if someone can make this warning shut up :pray:
'use strict';
import * as Knex from 'knex';
exports.up = function (knex: Knex) {
return knex
.insert([
{
idType: 1,
nomType: 'Midi',
},
{
idType: 2,
nomType: 'Soir',
},
])
.into('Types');
};
exports.down = function (knex: Knex) {
return knex.delete('*').from('Types');
};
Using environment: test
knex:tx trx2: Starting top level transaction +0ms
knex:tx trx2: releasing connection +104ms
knex:tx trx3: Starting top level transaction +139ms
.returning() is not supported by mysql and will not have any effect.
knex:tx trx3: releasing connection +149ms
Batch 2 rolled back the following migrations:
20181110160827-data-types.ts
The patch/hack
development: {
[...]
log: {
warn: (message) => {
// Shut up https://github.com/knex/knex/issues/3158
if (message === '.returning() is not supported by mysql and will not have any effect.') {
return;
}
console.warn(message);
},
},
},
@williamdes That migration should not emit that message, since you have not called .returning(). Please open separate bug report if that keeps happening. Sounds like, migration system might be internally using .returning() which triggers the error.
Good morning guys I need a way to remove the warnning logs, how do I do this?
Tip: If you're mutating a record in the database, and you have the input data, just return that directly upon a successful mutation.
ExpressJS example:
app.post('/api/v1/users', async (req, res, next) => {
const someBookData = req.body
try {
const arrayOfPrimaryKeysSuccessfullyInserted = await knex('books').insert(someBookData) // So this could be like [40222] where 40222 is the PK value in your users table
res.status(200).send({
id: arrayOfPrimaryKeysSuccessfullyInserted[0], // [40222][0] = 40222
...someBookData // the original book data
})
} catch(err) {
next(err)
}
})
I think this is correct, but someone feel free to correct me.
