search

loopbackio / loopback-connector-sqlite3

SQLite3 Connector for LoopBack
Other
13 stars 22 forks source link

Don't return lastID when primary key is TEXT type solve #13 #38

Open HugoPoi opened 6 years ago

HugoPoi commented 6 years ago

Description

This PR fix the issue #13 returning wrong id when the primary column is not a INTEGER type. Sqlite3 has a built-in feature for retrieving the lastID on INSERT statement. It's use here https://github.com/strongloop-community/loopback-connector-sqlite3/blob/master/lib/sqlite3.js#L202 But in the documentation of Sqlite3 https://sqlite.org/c3ref/last_insert_rowid.html the lastID contains the last rowid inserted but when your primary key is not integer you have two primary key : the primary key define in schema and the sqlite3 internal rowid. So when we have on INSERT the lastID=an integer and the real id the token which is override by the dao https://github.com/strongloop/loopback-datasource-juggler/blob/master/lib/dao.js#L410.

Solution

Add SQLite3.prototype._isPrimaryKeyText returning true when the primary key is TEXT. Edit SQLite3.prototype.getInsertedId for returning undefined when primary key is TEXT.

Related issues

Checklist

gms1 commented 6 years ago

I just stumbled upon this PR, may I say something about it? In Sqlite3, only a column with the type "INTEGER PRIMARY KEY" is an alias for the RowID. Therefore, 'getInsertedId' should only return the lastID (=RowID) if the primary key consists of exactly one INTEGER column. Instead of querying for 'NOT-TEXT', you may therefore want to ask for "INTEGER and NOT composite-ID"

HugoPoi commented 6 years ago

@gms1 I'm not an expert of sqlite, I greatly appreciate any suggestion or review. Ok so I need to change my schema analyse for INTEGER and NOT COMPOSITE ID. Also change the name of _isPrimaryKeyText to _isIntPrimaryKey.

rafaneri commented 5 years ago

@raymondfeng

jannyHou commented 3 years ago

@slnode test please

FeelyChau commented 3 years ago

I just stumbled upon this PR, may I say something about it? In Sqlite3, only a column with the type "INTEGER PRIMARY KEY" is an alias for the RowID. Therefore, 'getInsertedId' should only return the lastID (=RowID) if the primary key consists of exactly one INTEGER column. Instead of querying for 'NOT-TEXT', you may therefore want to ask for "INTEGER and NOT composite-ID"

+1, I think _isPrimaryKeyInteger is better.

HugoPoi commented 3 years ago

I run the tests locally, preserves empty values from the database when "applyDefaultsOnReads" is false is broken but it's not me, but I run the test under v15.2.1 :stuck_out_tongue:

FeelyChau commented 3 years ago

Could this PR be merged now? @jannyHou

FeelyChau commented 3 years ago

Ping @jannyHou

jacko commented 3 years ago

@jannyHou Could you please merge this PR?

achrinza commented 3 years ago

Hi @jacko, apologies for the delayed reply. We're in the midst of migrating the CI pipelines over to public infrastructure as part of LoopBack 4 becoming an OpenJS Foundation project.

I'll see if I can get this merged after the CI pipeline migration is completed.