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

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

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

HugoPoi
Copy link

@HugoPoi HugoPoi commented May 30, 2018

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

  • New tests added or existing tests modified to cover all changes
  • Code conforms with the style
    guide

@gms1
Copy link

gms1 commented Aug 2, 2018

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

HugoPoi commented Aug 2, 2018

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

@raymondfeng

@jannyHou
Copy link

@slnode test please

test/sqlite.test.js Outdated Show resolved Hide resolved
@FeelyChau
Copy link

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

HugoPoi commented Nov 30, 2020

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 😛

lib/sqlite3.js Outdated Show resolved Hide resolved
@FeelyChau
Copy link

Could this PR be merged now? @jannyHou

Copy link

@FeelyChau FeelyChau left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.

@FeelyChau
Copy link

Ping @jannyHou

@jacko
Copy link

jacko commented Aug 11, 2021

@jannyHou Could you please merge this PR?

@achrinza
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants