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

SQLite: Primary key property and value not populated on insert of object with auto-incrementing primary key #323

Open
jim-wilson-kt opened this issue Aug 14, 2017 · 5 comments

Comments

@jim-wilson-kt
Copy link

jim-wilson-kt commented Aug 14, 2017

I tracked the problem down to the fact that the code doesn't reach the else block in Massive.Shared.PerformInsert, which retrieves the identity value. This is because the _primaryKeyFieldSequence value is an empty string. The source of that value is _defaultSeqencyName in Massive.Sqlite. Setting that value to any non-empty string results in the primary key property being set on the inserted object as expected. From my perspective this is a work-around and probably even a hack, but I don't know what _primaryKeyFieldSequence is supposed to do (despite code comments that explain it). I also don't know if this functionality is covered by unit tests. I'll check when I further investigate the table Schema-property issue I recently posted. I just wanted to post this issue and work-around right away.

@jim-wilson-kt jim-wilson-kt changed the title SQLite: Primary Key Value not Populated on Insert SQLite: Primary key property and value not populated on insert of object with auto-incrementing primary key Aug 14, 2017
@FransBouma
Copy link
Owner

Thanks Jim :)

the primaryKeySequence is the sequence name used to create identity values when a row is inserted. In SQL Server 'SCOPE_IDENTITY()' is used, on oracle the name of a custom sequence is used, but on sqlite I don't think it supports sequences, but I'll check. Your issue suggests it does and there's a bug :) Hopefully I have more news later today :)

@jim-wilson-kt
Copy link
Author

Here are the two most informative pages I have found on the topic:

It is the call to the last_insert_rowid() SQL function mentioned in the second of the two references above that is executed in the else block I explained in my original post.

@FransBouma
Copy link
Owner

If you look at the test for Sqlite:
https://github.com/FransBouma/Massive/blob/v2.0/tests/Sqlite/TableClasses/Playlist.cs#L19

here I initialize the 'sequence' with 'last_insert_rowid()'

Indeed, if i instead don't pass the last 2 parameters, but instead set the default sequence for Sqlite to 'last_insert_rowid()' it works as well.

You can set the sequence as default also in the config file, using default_seq in the AppSettings part of your app/web.config file.

I don't know Sqlite usage patterns well enough to determine what's best here: change the default (if most tables on sqlite have an autoincrement value) or leave it as-is and you use either the app/webconfig settings way or specify the sequence in the constructor call of the DynamicModel constructor.

Thing is, changing this to the default will mean the value is obtained always for the table, even if you don't specify it currently, which might not what you want. (I know it's not the best of designs, I'll blame RobConery for that ;)). The current design is such that if no sequence is specified in the constructor call, the default is assumed, and that's used. If you specify "", it will assume no sequence and won't obtain an autonumber value for the PK field. So current Sqlite code uses this setup: if no sequence is specified, the default is used, which is "" so no sequence is assumed by default. When the default changes, this code will break, so I'm a little reluctant to do that. OTOH, if every table in sqlite has an autoincrement pk anyway, it won't break as it's always assumed to be there. Hopefully this makes sense :)

So any info you can give me in this case what to do would be great :)

@jim-wilson-kt
Copy link
Author

I think I understand your explanation, notably your point about breaking code. As you might expect for a project that uses SQLite, my application's scope is very narrow, the database is used exclusively by the application, and I have complete control over the database schema. Consequently, I don't think I'm qualified to comment on SQLite usage patterns. That said, I will check out the app.config-setting approach. If that works for me, then I will use it rather than modify the default value in code.

One question: In looking into this matter, did you conclude whether the default-sequence value matters (other than being non null and not empty) for retrieving the PK for auto-incrementing PKs? In other words, can it just be "abc" or "foo"? (I suggested as much in my original post.)

In any event, this should be documented, and I'm happy to do it. Shall I create a SQLite Implementation Notes page and submit a pull request on it?

@FransBouma
Copy link
Owner

@jim-wilson-kt A note to the readme.md is enough I think, as that's the documentation there is currently. Thanks :)

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

No branches or pull requests

2 participants