Make Database mutators non-const and Instance() return a writable handle#36
Merged
Conversation
…dle (#26) Instance() returned std::shared_ptr<const Database>, yet Save, SaveAutoIncrement, Update, Delete, and UnsafeSql were all declared const - so a handle typed as pointer-to-const could freely mutate persistent state. It compiled because the connection is a raw sqlite3* (const does not propagate through the pointer) and db_mutex_ is mutable. The API misrepresented intent: const looked read-only but wasn't. Make const mean read-only: - Instance() now returns std::shared_ptr<Database>. - The write methods (Save/SaveAutoIncrement/Update/Delete overloads, UnsafeSql) and their private void* worker helpers are now non-const. - The genuine reads (FetchAll, Fetch, private FetchRecords) stay const, and db_mutex_ stays mutable so they can still lock it. The ownership/lifecycle model is unchanged: the alive-across-Finalize() guarantee comes from Instance() returning a strong shared_ptr, not from the pointee being const. Serialized access via db_mutex_, the retired_ reinitialization guard, and all CRUD semantics are untouched. A read-only view is still available via the implicit shared_ptr<Database> -> shared_ptr<const Database> conversion, through which only the const fetch methods compile; README updated accordingly. Existing call sites of the form `const auto db = Database::Instance()` still compile (const handle, non-const pointee). Tests: compile-time SFINAE checks in database_test.cc assert the new contract (Save is NOT invocable through const Database, FetchAll IS), with the trait machinery verified to detect the positive case on non-const Database so the negative assert isn't vacuous; plus a runtime test that a shared_ptr<const Database> view can still fetch. Full suite: 76/76 pass in both C++11 and C++20 (including the concurrency/lifecycle tests). Note this is an ABI change (method const-ness and Instance()'s return type alter mangled names) - acceptable for a source library. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #26.
The smell:
Database::Instance()returnedstd::shared_ptr<const Database>, yet every mutating operation —Save,SaveAutoIncrement,Update,Delete(all overloads), andUnsafeSql— was declaredconst. A handle typed as pointer-to-constcould freely write to the database. It compiled because the connection is a rawsqlite3* db_(constdoesn't propagate through the pointer to the SQLite handle) anddb_mutex_ismutable. The API misrepresented intent:constlooked read-only but wasn't, which can mislead callers reasoning about thread-safety and aliasing.Fix
Make
constmean read-only:Instance()now returnsstd::shared_ptr<Database>.Save/SaveAutoIncrement/Update/Deleteoverloads,UnsafeSql) and their privatevoid*worker helpers are now non-const.FetchAll,Fetch, privateFetchRecords) stayconst, anddb_mutex_staysmutableso they can still lock it.Ownership/lifecycle model unchanged. The alive-across-
Finalize()guarantee comes fromInstance()returning a strongshared_ptr, not from the pointee beingconst— serialized access viadb_mutex_, theretired_reinitialization guard, and all CRUD semantics are untouched. The concurrency/lifecycle tests pass unchanged.Read-only views still available. The implicit
shared_ptr<Database>→shared_ptr<const Database>conversion yields a view through which only theconstfetch methods compile; the README's ownership section documents this. Existing call sites of the formconst auto db = Database::Instance(); db->Save(...)still compile (const handle, non-const pointee), so no call-site churn.Tests
tests/database_test.ccassert the new contract:Saveis not invocable throughconst Database,FetchAllis. The trait machinery was separately verified to detect the positive case on non-constDatabase, so the negative assert isn't vacuously passing.ReadOnlyHandleCanStillFetch: ashared_ptr<const Database>view can still fetch.concurrency_test.cc(lifecycle/threading guarantees) and all ofdatabase_test.ccunchanged.ABI note
This is an ABI change: method
const-ness andInstance()'s return type alter mangled names. Acceptable for a source library, but downstream binaries built against the old headers need a rebuild.Scope
Const-correctness only — no behavior change to CRUD semantics, and no other open design issues (#2 nullable fields, #14 identifier quoting, etc.) folded in.
Generated by Claude Code