From b07490659f70af613449581e8bd1406e714a8422 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 18 Jul 2026 19:27:54 +0000 Subject: [PATCH] Make Database mutators non-const and Instance() return a writable handle (#26) Instance() returned std::shared_ptr, 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. - 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 -> shared_ptr 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 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 Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK --- README.md | 13 ++++++++++- include/database.h | 34 +++++++++++++++-------------- src/database.cc | 12 +++++------ tests/database_test.cc | 49 ++++++++++++++++++++++++++++++++++++++++++ 4 files changed, 85 insertions(+), 23 deletions(-) diff --git a/README.md b/README.md index cfca1bc..c09c8cc 100644 --- a/README.md +++ b/README.md @@ -180,7 +180,7 @@ SQLite's serialized mode and all operations are serialized internally, so concur `Save`/`Fetch`/`Update`/`Delete` calls are safe (they execute one at a time rather than in parallel). `Initialize`, `Finalize`, and `Instance` are also safe to call concurrently. -**Ownership model.** `Instance()` returns a `std::shared_ptr` rather than a +**Ownership model.** `Instance()` returns a `std::shared_ptr` rather than a reference. Each user holds a strong handle, so the database stays alive for as long as anyone is using it: an operation in progress on one thread is never torn down by a concurrent `Finalize()` on another. `Finalize()` only drops the singleton's own handle; the connection is @@ -197,6 +197,17 @@ std::thread worker([db] { worker.join(); ``` +The write methods (`Save`, `SaveAutoIncrement`, `Update`, `Delete`, `UnsafeSql`) are +non-`const`, and the fetch methods are `const`. To hand a component a read-only view, assign +the handle to a `std::shared_ptr` (the conversion is implicit) — through it, +only the fetch methods compile: + +```c++ +std::shared_ptr reader = Database::Instance(); +auto people = reader->FetchAll(); // ok +// reader->Save(person); // does not compile: Save is not const +``` + Writes (and reads) are serialized, so they are safe but not concurrent; true write parallelism is tracked as future work. diff --git a/include/database.h b/include/database.h index 0c5b9a5..7006260 100644 --- a/include/database.h +++ b/include/database.h @@ -53,8 +53,10 @@ class REFLECTION_EXPORT Database { /// Retrieves the database singleton wrapper for further operations. The returned shared /// handle keeps the database alive for as long as the caller holds it, so an operation in - /// progress is never torn down by a concurrent Finalize(). - static std::shared_ptr Instance(); + /// progress is never torn down by a concurrent Finalize(). The handle is writable; for a + /// read-only view, assign it to a std::shared_ptr (the conversion is + /// implicit), through which only the const fetch methods are callable. + static std::shared_ptr Instance(); ~Database(); @@ -99,7 +101,7 @@ class REFLECTION_EXPORT Database { /// Saves a given record in the database. /// This corresponds to an INSERT query in the SQL syntax template - void Save(const T& model) const { + void Save(const T& model) { const auto type_id = typeid(T).name(); const auto& record = GetRecord(type_id); Save((void*)&model, record); @@ -109,7 +111,7 @@ class REFLECTION_EXPORT Database { /// The newly generated id is written back into the passed-in model. /// This corresponds to an INSERT query in the SQL syntax template - void SaveAutoIncrement(T& model) const { + void SaveAutoIncrement(T& model) { const auto type_id = typeid(T).name(); const auto& record = GetRecord(type_id); model.id = SaveAutoIncrement((void*)&model, record); @@ -118,7 +120,7 @@ class REFLECTION_EXPORT Database { /// Saves multiple records iteratively in the database. /// This corresponds to an INSERT query in the SQL syntax template - void Save(const std::vector& models) const { + void Save(const std::vector& models) { const auto type_id = typeid(T).name(); const auto& record = GetRecord(type_id); for (const auto& model : models) { @@ -130,7 +132,7 @@ class REFLECTION_EXPORT Database { /// The newly generated ids are written back into the passed-in models. /// This corresponds to an INSERT query in the SQL syntax template - void SaveAutoIncrement(std::vector& models) const { + void SaveAutoIncrement(std::vector& models) { const auto type_id = typeid(T).name(); const auto& record = GetRecord(type_id); for (auto& model : models) { @@ -141,7 +143,7 @@ class REFLECTION_EXPORT Database { /// Updates a given record in the database. /// This corresponds to an UPDATE query in the SQL syntax template - void Update(const T& model) const { + void Update(const T& model) { const auto type_id = typeid(T).name(); const auto& record = GetRecord(type_id); Update((void*)&model, record); @@ -150,7 +152,7 @@ class REFLECTION_EXPORT Database { /// Updates multiple records iteratively in the database. /// This corresponds to an UPDATE query in the SQL syntax template - void Update(const std::vector& models) const { + void Update(const std::vector& models) { const auto type_id = typeid(T).name(); const auto& record = GetRecord(type_id); for (const auto& model : models) { @@ -161,7 +163,7 @@ class REFLECTION_EXPORT Database { /// Deletes a given record from the database. /// This corresponds to an DELETE query in the SQL syntax template - void Delete(const T& model) const { + void Delete(const T& model) { const auto type_id = typeid(T).name(); const auto& record = GetRecord(type_id); const auto equal_id_predicate = Equal(&T::id, model.id); @@ -171,7 +173,7 @@ class REFLECTION_EXPORT Database { /// Deletes a given record from the database, which matches a given id. /// This corresponds to an DELETE query in the SQL syntax template - void Delete(int64_t id) const { + void Delete(int64_t id) { const auto type_id = typeid(T).name(); const auto& record = GetRecord(type_id); const auto equal_id_predicate = Equal(&T::id, id); @@ -181,7 +183,7 @@ class REFLECTION_EXPORT Database { /// Deletes multiple records of a given type from the database, which match a given predicate. /// This corresponds to an DELETE query in the SQL syntax, with an additional WHERE clause template - void Delete(const QueryPredicateBase* predicate) const { + void Delete(const QueryPredicateBase* predicate) { const auto type_id = typeid(T).name(); const auto& record = GetRecord(type_id); Delete(record, predicate); @@ -189,7 +191,7 @@ class REFLECTION_EXPORT Database { /// Executes a raw SQL query. A trailing semicolon is added if needed. /// Prefer the type-safe CRUD APIs for user-provided values. - void UnsafeSql(const std::string& raw_sql_query) const; + void UnsafeSql(const std::string& raw_sql_query); private: explicit Database(const char* path); @@ -214,17 +216,17 @@ class REFLECTION_EXPORT Database { } /// Saves a single record in the database - void Save(void* p, const Reflection& record) const; + void Save(void* p, const Reflection& record); /// Saves a single record in the database, letting SQLite assign its id, /// and returns the id that was generated for the inserted row - int64_t SaveAutoIncrement(void* p, const Reflection& record) const; + int64_t SaveAutoIncrement(void* p, const Reflection& record); /// Updates a single record in the database - void Update(void* p, const Reflection& record) const; + void Update(void* p, const Reflection& record); /// Deletes a single record from the database - void Delete(const Reflection& record, const QueryPredicateBase* predicate) const; + void Delete(const Reflection& record, const QueryPredicateBase* predicate); static std::shared_ptr instance_; diff --git a/src/database.cc b/src/database.cc index 1e8387e..2e504e7 100644 --- a/src/database.cc +++ b/src/database.cc @@ -105,7 +105,7 @@ Database::Database(const char* path) : db_(nullptr) { } } -std::shared_ptr Database::Instance() { +std::shared_ptr Database::Instance() { std::lock_guard lock(instance_mutex_); if (instance_ == nullptr) { throw std::runtime_error("Database has not been initialized; call Database::Initialize() first"); @@ -117,13 +117,13 @@ const Reflection& Database::GetRecord(const std::string& type_id) { return GetReflectionRegister().records.at(type_id); } -void Database::Save(void* p, const Reflection& record) const { +void Database::Save(void* p, const Reflection& record) { std::lock_guard lock(db_mutex_); InsertQuery query(db_, record, p); query.Execute(); } -int64_t Database::SaveAutoIncrement(void* p, const Reflection& record) const { +int64_t Database::SaveAutoIncrement(void* p, const Reflection& record) { // The lock spans both the insert and the rowid read so that, on the shared connection, // sqlite3_last_insert_rowid() reflects this insert and not one from another thread. std::lock_guard lock(db_mutex_); @@ -132,19 +132,19 @@ int64_t Database::SaveAutoIncrement(void* p, const Reflection& record) const { return sqlite3_last_insert_rowid(db_); } -void Database::Update(void* p, const Reflection& record) const { +void Database::Update(void* p, const Reflection& record) { std::lock_guard lock(db_mutex_); UpdateQuery query(db_, record, p); query.Execute(); } -void Database::Delete(const Reflection& record, const QueryPredicateBase* predicate) const { +void Database::Delete(const Reflection& record, const QueryPredicateBase* predicate) { std::lock_guard lock(db_mutex_); DeleteQuery query(db_, record, predicate); query.Execute(); } -void Database::UnsafeSql(const std::string& raw_sql_query) const { +void Database::UnsafeSql(const std::string& raw_sql_query) { std::lock_guard lock(db_mutex_); SqlQuery sql(db_, raw_sql_query); sql.Execute(); diff --git a/tests/database_test.cc b/tests/database_test.cc index c4f3f80..29cc875 100644 --- a/tests/database_test.cc +++ b/tests/database_test.cc @@ -24,6 +24,10 @@ #include +#include +#include +#include + #include "company.h" #include "id_only_record.h" #include "person.h" @@ -31,6 +35,38 @@ using namespace sqlite_reflection; +namespace { +// C++11 stand-in for std::void_t (C++17) +template +struct MakeVoid { + typedef void type; +}; + +// Detects whether Save(const T&) is invocable through a const Database +template +struct CanSaveThroughConst : std::false_type {}; + +template +struct CanSaveThroughConst< + T, typename MakeVoid().Save(std::declval()))>::type> + : std::true_type {}; + +// Detects whether FetchAll() is invocable through a const Database +template +struct CanFetchAllThroughConst : std::false_type {}; + +template +struct CanFetchAllThroughConst().FetchAll())>::type> + : std::true_type {}; +} // namespace + +// The const-correctness contract (#26): const means read-only. A handle to a const Database +// can fetch but not mutate; the write methods are only invocable through a non-const handle. +static_assert(!CanSaveThroughConst::value, + "Save must not be invocable through a const Database - write methods are non-const"); +static_assert(CanFetchAllThroughConst::value, + "FetchAll must remain invocable through a const Database - read methods are const"); + class DatabaseTest : public ::testing::Test { void SetUp() override { Database::Initialize(""); @@ -41,6 +77,19 @@ class DatabaseTest : public ::testing::Test { } }; +TEST_F(DatabaseTest, ReadOnlyHandleCanStillFetch) { + const auto db = Database::Instance(); + const Person p{L"ada", L"lovelace", 36, true, 1}; + db->Save(p); + + // The implicit shared_ptr -> shared_ptr conversion yields a + // read-only view, through which the const fetch methods keep working + std::shared_ptr reader = Database::Instance(); + const auto all = reader->FetchAll(); + ASSERT_EQ(1, all.size()); + EXPECT_EQ(p.first_name, all[0].first_name); +} + TEST_F(DatabaseTest, Initialization) { const auto db = Database::Instance();