Fix SaveAutoIncrement to write back a database-assigned, non-reused id (#6)#8
Conversation
SaveAutoIncrement previously copied the input record, set an id computed client-side as MAX(id)+1, and persisted the copy, leaving the caller's object without the generated id and with no way to look the row back up. SaveAutoIncrement now takes a mutable reference and lets SQLite assign the id natively: the INSERT omits the id column (which is an INTEGER PRIMARY KEY / rowid alias) and the value returned by sqlite3_last_insert_rowid is written back into the passed-in model. This is atomic, avoids an extra SELECT MAX per save, and removes the stale "id is not modified" behavior described in the issue. - InsertQuery gains an auto_increment_id mode that drops the id column and its binding from the statement - Database::SaveAutoIncrement(void*, record) returns the assigned id - Public templates updated to T& / vector<T>& and write the id back - Tests assert the id is populated on single and batch saves and that ids continue from the current maximum - README updated to document the new write-back behavior
Plain INTEGER PRIMARY KEY reuses the rowid of a deleted highest row on the next insert. Declaring the id column as INTEGER PRIMARY KEY AUTOINCREMENT makes SQLite track the highest id ever assigned in sqlite_sequence and never hand it out again, which is the expected behavior for stable record identifiers. - CreateTableQuery emits PRIMARY KEY AUTOINCREMENT for the id column - Added a test asserting an id is not reused after the highest row is deleted - README documents the no-reuse guarantee
The windows-latest runner image no longer provides a Visual Studio 2022 (v17) instance that CMake can detect, so configuration failed with "could not find any instance of Visual Studio" for the hardcoded "Visual Studio 17 2022" generator. Pinning the Windows matrix entries to windows-2022 guarantees a matching toolchain is present.
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e78b0cb3c6
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ``` | ||
|
|
||
| Use `SaveAutoIncrement` when you want the library to assign the next available `id` for the saved row. The input object is copied; its `id` member is not modified. | ||
| Use `SaveAutoIncrement` when you want the library to assign the next available `id` for the saved row. The database generates the `id` (via an `AUTOINCREMENT` column, so values are never reused even after rows are deleted) and it is written back into the passed-in object, which must therefore be a mutable (non-`const`) lvalue. |
There was a problem hiding this comment.
When a user opens a database file created by the previous release, this guarantee is not true: CreateTableQuery::PrepareSql() still uses CREATE TABLE IF NOT EXISTS, so existing tables keep their old id INTEGER PRIMARY KEY definition, and the new SaveAutoIncrement path now relies on SQLite's table schema to avoid reusing a deleted highest rowid. Please either document that the never-reused guarantee only applies to newly created/migrated tables or add a migration so upgraded databases get the AUTOINCREMENT behavior too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch. Since the library has no migration layer yet, I've taken the documentation route (as suggested): the README now qualifies the never-reused guarantee, noting it applies to tables created by this version onwards and that a database file created by an earlier release keeps its original id INTEGER PRIMARY KEY schema until recreated or migrated. Done in 91864d5.
Generated by Claude Code
The AUTOINCREMENT guarantee is applied at table creation. Since tables are created with CREATE TABLE IF NOT EXISTS, databases created by an earlier release keep their original id INTEGER PRIMARY KEY schema until recreated or migrated. Document this caveat in the README so the guarantee is not overstated.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91864d5a11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| int64_t Database::SaveAutoIncrement(void* p, const Reflection& record) const { | ||
| InsertQuery query(db_, record, p, true); | ||
| query.Execute(); | ||
| return sqlite3_last_insert_rowid(db_); |
There was a problem hiding this comment.
When two threads share Database::Instance(), query.Execute() completes before this connection-wide lookup, so another thread can insert on the same db_ in that window. Since sqlite3_last_insert_rowid(db_) reports the most recent insert on the connection rather than the row inserted by this call, SaveAutoIncrement can write the other row's id into model.id, causing later fetch/update/delete calls to target the wrong row. Return the id from the insert statement itself or guard the insert and lookup with the same mutex.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Thanks — the analysis is correct, but this is out of scope for the current design. The library exposes a single shared connection via Database::Instance() and has no locking anywhere: Save, Update, Delete, and the batch variants all run unguarded on that one db_, and the previous MAX(id)+1 auto-increment path had the same race. In other words, concurrent multi-threaded writes are not supported today, and under the library's single-threaded usage sqlite3_last_insert_rowid(db_) immediately after the insert returns the correct id.
A truly atomic fix is available (the bundled SQLite is 3.53.0, so INSERT ... RETURNING id would read the id from the same statement and avoid the connection-wide lookup entirely), but adding it for just this one path would imply a thread-safety guarantee the rest of the API doesn't provide. Proper concurrency support belongs in a dedicated change that covers the whole surface, so I'm leaving this as-is for now and treating it as a known limitation rather than addressing it piecemeal in this PR.
Generated by Claude Code
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 91864d5a11
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const std::vector<std::string> columns_without_id(columns.begin() + 1, columns.end()); | ||
| sql += StringUtilities::Join(columns_without_id, ", ") + ") VALUES ("; | ||
| sql += Placeholders(columns_without_id.size()) + ");"; |
There was a problem hiding this comment.
When a reflected record has no user fields besides the implicit id (for example an empty FIELDS macro), columns_without_id is empty here and the generated SQL becomes INSERT INTO Name () VALUES ();, which SQLite rejects with a syntax error. This used to work because SaveAutoIncrement inserted an explicit generated id; the auto-increment path should emit INSERT INTO Name DEFAULT VALUES; (and no bindings) for this case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good find — fixed in 62625ff. The auto-increment path now emits INSERT INTO <name> DEFAULT VALUES; (with no bindings) when the record has no columns besides id, instead of the invalid INSERT INTO <name> () VALUES ();. Added an id-only reflectable test record and a regression test that auto-increment-inserts two of them and verifies the assigned ids and a round-trip fetch.
Generated by Claude Code
When a reflected record has no fields besides the implicit id, the auto-increment insert path produced "INSERT INTO Name () VALUES ()", which SQLite rejects as a syntax error. Emit "INSERT INTO Name DEFAULT VALUES" (with no bindings) for that case so the row is inserted and the rowid is assigned. Adds an id-only reflectable test record and a regression test covering auto-increment insertion and fetch for it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 62625ff9e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| auto values = GetValues(p_); | ||
| if (auto_increment_id_) { | ||
| // The id column is omitted from the statement, so drop its binding too | ||
| values.erase(values.begin()); |
There was a problem hiding this comment.
When SaveAutoIncrement is called with a record whose id member was never initialized, this still materializes every reflected field via GetValues(p_) before erasing the id binding. The new id-only test constructs IdOnlyRecord first;, leaving id indeterminate until SQLite assigns it, so this path reads an indeterminate int64_t even though the value is discarded. Build the binding list starting at the first non-id member instead of reading and erasing the id value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — fixed in c96bd60. GetValues() now takes a start member index, and the auto-increment insert path calls GetValues(p_, 1) to build bindings starting from the first non-id member, so the (possibly uninitialized) id value is never read instead of being read and then erased. For an id-only record this yields an empty binding list, matching the DEFAULT VALUES statement.
Generated by Claude Code
For auto-increment inserts the id column is omitted from the statement, yet Bindings() previously materialized every member via GetValues() and then erased the id binding. When the record's id was never initialized (e.g. an id-only record), this read an indeterminate int64_t before discarding it. GetValues() now accepts a start member index, and the auto-increment insert path builds bindings starting from the first non-id member, so the id value is never read.
|
@codex review |
|
Codex Review: Didn't find any major issues. Bravo. ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
The auto-increment insert path only ever needs to skip the leading id member, so a boolean skip_id expresses the intent more clearly than a numeric start index at the call site.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 859b44470b
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| int64_t Database::SaveAutoIncrement(void* p, const Reflection& record) const { | ||
| InsertQuery query(db_, record, p, true); | ||
| query.Execute(); | ||
| return sqlite3_last_insert_rowid(db_); |
There was a problem hiding this comment.
When two threads share Database::Instance(), query.Execute() commits and returns before this connection-wide value is read, so another Save/SaveAutoIncrement on the same sqlite3* can insert in that gap and make this call return the other row's id; SQLite documents sqlite3_last_insert_rowid() as connection state, not a per-statement result (https://sqlite.org/c3ref/last_insert_rowid.html). In that interleaving the first caller's model gets the wrong id even though its insert succeeded, so the rowid needs to be captured while the insert execution is still serialized with the connection.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
This is the same sqlite3_last_insert_rowid connection-state race already discussed in #8 (comment), and the conclusion there stands: the library has no thread-safety model today (Save/Update/Delete and the batch variants all run unguarded on the single shared connection), so concurrent multi-threaded writes are unsupported and under the library's single-threaded usage the lookup is correct. Proper concurrency support (e.g. INSERT ... RETURNING id, available in the bundled SQLite 3.53.0, or connection-level locking) is intentionally deferred to a dedicated change covering the whole API surface rather than this one path. Treating it as a known limitation for this PR.
Generated by Claude Code
2 participants
Summary
Fixes #6.
SaveAutoIncrementpreviously copied the input record, assigned an id computed client-side, and persisted the copy — leaving the caller's object without the generated id and with no reliable way to look the row back up. This PR makes the database the source of truth for ids and writes the assigned id back into the caller's record.What changed
1.
SaveAutoIncrementwrites the generated id back into the caller's objectT&/std::vector<T>&) instead ofconst T&, so the assigned id can be written back into the argument.model.id(and each element'sidfor the batch overload) holds the value the database assigned.2. The database generates the id natively (not
MAX(id)+1)InsertQuerygained an auto-increment mode that omits theidcolumn from theINSERT. Sinceidis anINTEGER PRIMARY KEY(a rowid alias), SQLite assigns the next value itself.Database::SaveAutoIncrement(void*, record)reads that value back viasqlite3_last_insert_rowid()and returns it to be written into the model.SELECT MAX(id) + 1logic is removed from the save path (FetchMaxIdQueryis retained for the publicGetMaxId<T>()API).3. Ids are never reused
idcolumn is now created asINTEGER PRIMARY KEY AUTOINCREMENT, so SQLite tracks the highest id ever assigned (viasqlite_sequence) and never hands it out again — even after the highest row is deleted.4. CI: pin Windows jobs to
windows-2022windows-latestrunner image no longer exposes a Visual Studio 2022 (v17) instance for the workflow's hardcoded"Visual Studio 17 2022"generator, so configuration failed before any compilation. Pinning the Windows matrix entries towindows-2022restores a matching toolchain. (Unrelated to the code change; needed to get CI green.)Rationale
The native rowid mechanism is the correct way to let the database assign ids, and it is strictly better than the previous
MAX(id)+1approach:MAX(id)+1(before)rowid+last_insert_rowid(after)SELECT MAXbefore every saveAUTOINCREMENT)Adding
AUTOINCREMENTguarantees stable, never-reused identifiers, which is the expected behavior for record ids.API/compatibility note
SaveAutoIncrementnow requires a mutable lvalue, so passing aconstor temporary object no longer compiles. This is inherent to writing the id back into the caller's object.Schema note
CREATE TABLE IF NOT EXISTSdoes not alter existing tables, so theAUTOINCREMENTguarantee applies to newly created databases. Databases created before this change keep their previous schema until migrated (the library has no migration layer yet).Tests
https://claude.ai/code/session_01YLXHcZjrnrdhRZAgqDh6kd
Generated by Claude Code