Skip to content

feat(sqlite): navigational write (append/set/flush/delete) for the SQLite backend#122

Closed
Admnwk wants to merge 1 commit into
FiveTechSoft:mainfrom
Admnwk:feat/sqlite-native-write-nav
Closed

feat(sqlite): navigational write (append/set/flush/delete) for the SQLite backend#122
Admnwk wants to merge 1 commit into
FiveTechSoft:mainfrom
Admnwk:feat/sqlite-native-write-nav

Conversation

@Admnwk

@Admnwk Admnwk commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Summary

The SQLite backend supported read + SQL passthrough only; the navigational
write path (AdsAppendRecord / AdsSetString / AdsWriteRecord /
AdsDeleteRecord) returned an error, while the MariaDB / PostgreSQL /
Firebird / ODBC backends already implement it. This closes that gap so the
in-process SQLite driver matches the other SQL backends' write contract.

Changes

  • SqliteTable: add a staging buffer (staging_row/staging_nulls,
    pending_append, row_dirty), mirroring MariaTable/FirebirdTable.
  • SqliteConnection: implement append_blank / set_field /
    flush_record / delete_record. flush_record emits a parameterized
    INSERT (pending append) or a rowid-keyed UPDATE; delete_record a
    rowid-keyed DELETE. SQLite's implicit rowid is the key, so no
    multi-column PK snapshot is needed; values are bound via sqlite3_bind_*
    (no string escaping), and freshly-inserted rows are located with
    sqlite3_last_insert_rowid + a rowid-list reload.
  • ace_exports.cpp: dispatch the four ABI write entry points to the
    SQLite backend, following the existing per-backend dispatch pattern.
  • New self-contained test (abi_plus_sqlite_write_test): seeds a temp
    .db via the sqlite3 C API, then drives append/update/delete purely
    through the ACE ABI — no external server required.

Coordination

This touches src/abi/ace_exports.cpp (the ABI write dispatch). The additions
are additive and mirror the existing per-backend blocks — happy to adjust
placement or style to your preference.

Testing

  • Full unit suite: 876/876 pass, /WX (warnings-as-errors) clean, MSVC x64.
  • New SQLite write test: 23/23 assertions (append + update + delete round-trip).

🤖 Generated with Claude Code (Opus 4.8)

…Lite backend

The SQLite backend exposed read + SQL passthrough only; navigational write
(AdsAppendRecord / AdsSetString / AdsWriteRecord / AdsDeleteRecord) returned
an error, while MariaDB/PostgreSQL/Firebird/ODBC already supported it. This
closes that gap so the in-process SQLite driver matches the other SQL
backends' write contract.

- SqliteTable: add staging buffer (staging_row/nulls, pending_append,
  row_dirty), mirroring MariaTable/FirebirdTable.
- SqliteConnection: implement append_blank / set_field / flush_record /
  delete_record. flush emits a parameterized INSERT (pending_append) or a
  rowid-keyed UPDATE; delete a rowid-keyed DELETE. SQLite rowid is the
  implicit key, so no multi-column PK snapshot is needed; values bound via
  sqlite3_bind_* (no string escaping), new rows located via
  sqlite3_last_insert_rowid + rowid-list reload.
- ace_exports: dispatch the four ABI write entry points to the SQLite
  backend, following the existing per-backend pattern.
- Self-contained doctest (abi_plus_sqlite_write_test): seeds a temp .db via
  the sqlite3 C API, then drives append/update/delete purely through the ACE
  ABI. No external server needed.

Suite: 876/876 pass, /WX (-Werror) clean, MSVC x64.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Code Review

This pull request implements navigational write operations (append, set field, flush, and delete) for the SQLite backend, aligning it with other supported database backends. It adds write staging fields to SqliteTable, implements the corresponding execution logic in SqliteConnection, and includes unit tests. Feedback on the changes highlights a critical issue in SqliteConnection::delete_record where deleting a record can leave the cursor in an inconsistent state, potentially causing stale data reads or out-of-bounds access if the deleted record was the last one.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment on lines +829 to +831
tbl->row_dirty = false;
tbl->pending_append = false;
return load_rowids(db, tbl);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

high

After deleting the current record, the cursor state is left in an inconsistent and potentially dangerous state:

  1. Stale Data Read: tbl->row_valid remains true and tbl->current_row is not cleared, meaning subsequent read operations will return stale data from the deleted record.
  2. Out-of-Bounds Access: Since the record is deleted, the list of rowids returned by load_rowids will shrink by 1. If the deleted record was the last record in the table, tbl->pos will now be equal to tbl->rowids.size(). Because tbl->row_valid is still true, any subsequent operation attempting to access tbl->rowids[tbl->pos] will result in an out-of-bounds access and potential crash.

Setting tbl->row_valid = false and checking if tbl->pos is out of bounds to clear tbl->positioned resolves these issues.

    tbl->row_dirty      = false;
    tbl->pending_append = false;
    tbl->row_valid      = false;
    if (auto r = load_rowids(db, tbl); !r) return r.error();
    if (tbl->pos >= tbl->rowids.size()) {
        tbl->positioned = false;
    }
    return util::Result<void>{};

@Admnwk

Admnwk commented Jun 27, 2026

Copy link
Copy Markdown
Contributor Author

Withdrawing to re-verify against real-world data before resubmitting.

@Admnwk Admnwk closed this Jun 27, 2026
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.

1 participant