fix(sqlite): fix returning sqlite and add tests for support sqlite #59
Conversation
Codecov Report
@@ Coverage Diff @@
## master #59 +/- ##
==========================================
- Coverage 99.31% 98.00% -1.32%
==========================================
Files 3 3
Lines 147 150 +3
==========================================
+ Hits 146 147 +1
- Misses 1 3 +2
Continue to review full report at Codecov.
|
|
🎉 This PR is included in version 1.5.3 🎉 The release is available on: Your semantic-release bot 📦🚀 |
| @@ -187,6 +187,12 @@ module.exports = class Repository { | |||
| .returning(fields) | |||
| .insert(payload) | |||
|
|
|||
| //.returning() is not supported by sqlite3 and will not have any effect, so we have to get last inserted row | |||
| if (this.runner().client && this.runner().client.driverName && (this.runner().client.driverName.includes('sqlite3'))) { | |||
There was a problem hiding this comment.
Is there a way to abstract this to a HasReturningRowSupport method or something similar? Does Knex has something like this?
There was a problem hiding this comment.
No there is'not. (https://github.com/knex/knex/blob/49ea2b5fa9068cc15aae94258fae673379127d9d/lib/dialects/sqlite3/index.js#L196
)
But SQLLite now supports returning and should be included in knex soon. (knex/knex#4370)
There was a problem hiding this comment.
#64 now we can remove this line with this update.
There was a problem hiding this comment.
| @@ -187,6 +187,12 @@ module.exports = class Repository { | |||
| .returning(fields) | |||
| .insert(payload) | |||
|
|
|||
| //.returning() is not supported by sqlite3 and will not have any effect, so we have to get last inserted row | |||
| if (this.runner().client && this.runner().client.driverName && (this.runner().client.driverName.includes('sqlite3'))) { | |||
| const lastInsertedID = await this.knex.raw(`SELECT last_insert_rowid();`) | |||
There was a problem hiding this comment.
Given the async nature of the queries (.insert(payload) and knex.raw('SELECT last_insert_rowid();') ) it seems it could happen a race condition here: the returned value for last_insert_rowid() would be from a different insert, from a different event loop.
Event Loop A: Insert X
Event Loop B: Insert Y
Event Loop A: get last_insert_rowid = Y, but should be X
There was a problem hiding this comment.
You're right.
We should isolate this in some kind of transaction (http://knexjs.org/guide/transactions.html). I`ll open an issue about this.
| //.returning() is not supported by sqlite3 and will not have any effect, so we have to get last inserted row | ||
| if (this.runner().client && this.runner().client.driverName && (this.runner().client.driverName.includes('sqlite3'))) { | ||
| const lastInsertedID = await this.knex.raw(`SELECT last_insert_rowid();`) | ||
| return await this.findByID(lastInsertedID[0]["last_insert_rowid()"]) |
There was a problem hiding this comment.
this.findByID expect a id(s) from the entity. A "domain" ID.
last_insert_rowid return the PK only if (1) it exists a PK, (2) if it is a integer [1] and (3) if it is not composed PK. Otherwise it returns a number not related to a "domain" ID.
So, the current implementation seems very error-prone. Is this the only method to return the entity?
Suggestion:
If the insertion does not return a error, use the known IDs of the inserted record to retrieve the record.
[1] from doc: If the table has a column of type INTEGER PRIMARY KEY then that column is another alias for the rowid.
There was a problem hiding this comment.
I'm aware (also working on it) of this possible bug and will fix it ASAP.
fix #58
Fixes #
Readiness Checklist
Author/Contributor
Reviewing Maintainer
breakingif this is a large fundamental changeautomation,bug,documentation,enhancement,infrastructure, orperformance