From 398c870e16742314ed7768250a3822c905c2940a Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 7 Jul 2026 19:19:06 +0000 Subject: [PATCH 1/2] Fail fast on unregistered types and unmatched predicate members (#23, #22) GetRecordFromTypeId used std::map::operator[], which default-inserts an empty Reflection for an absent key, so an unregistered or misspelled type silently produced a record with an empty table name and no columns instead of an error - only surfacing later as an opaque SQLite prepare failure. GetRecordFromTypeId is dual-use though: the QueryPredicate constructor uses it as a pure lookup (where a miss should fail), but the generated Register() also relies on its default-insert side effect to CREATE the registry entry. Making it throw outright would break every registration. Fix, in order: - Register() (include/reflection.h) now creates its entry directly via the in-scope instance.records[type_id], inside the existing find() guard, instead of going through GetRecordFromTypeId. Registration behavior is unchanged. - GetRecordFromTypeId (src/reflection.cc) is now a pure lookup: find() + throw std::runtime_error naming the type_id on a miss. Its only remaining caller is the QueryPredicate lookup path, so an unregistered type now fails fast at the point of use. Database::GetRecord already used records.at(...), so no other caller depended on create-on-miss. Separately, QueryPredicate's member-matching constructor (include/query_predicates.h) scans member_metadata for an offset match and silently left member_name_ empty (emitting malformed SQL like " = ?") if none matched - e.g. a registered type whose member metadata doesn't include the given pointer-to-member's offset. Track whether a match was found and throw a std::runtime_error naming the record if not, in the templated (fn, value, symbol, retrieval) constructor only - not the (symbol, member_name, value) constructor Clone() uses, and not EmptyPredicate, which doesn't go through this path. Tests: a predicate on a struct that never went through the REFLECTABLE/FIELDS macros throws (#23); a predicate on a type registered by hand with no member metadata - so no offset can match - throws (#22). Reverting the source changes while keeping the tests confirms both fail on the prior code. Full suite: 74/74 tests pass, including every existing Save/Fetch/predicate test, confirming registration and normal predicate construction are unaffected. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK --- include/query_predicates.h | 7 +++++++ include/reflection.h | 5 ++++- src/reflection.cc | 8 ++++++-- tests/query_predicates_test.cc | 35 ++++++++++++++++++++++++++++++++++ 4 files changed, 52 insertions(+), 3 deletions(-) diff --git a/include/query_predicates.h b/include/query_predicates.h index 026616a..e29b275 100644 --- a/include/query_predicates.h +++ b/include/query_predicates.h @@ -24,6 +24,7 @@ #include #include +#include #include #include @@ -82,13 +83,19 @@ class REFLECTION_EXPORT QueryPredicate : public QueryPredicateBase { : symbol_(symbol) { auto record = GetRecordFromTypeId(typeid(T).name()); auto offset = OffsetFromStart(fn); + auto found = false; for (auto i = 0; i < record.member_metadata.size(); ++i) { if (record.member_metadata[i].offset == offset) { member_name_ = record.member_metadata[i].name; value_ = value_retrieval((void*)&value, record.member_metadata[i].storage_class); + found = true; break; } } + if (!found) { + throw std::runtime_error("No registered member of '" + record.name + + "' matches the given pointer-to-member (type id: " + typeid(T).name() + ")"); + } } template diff --git a/include/reflection.h b/include/reflection.h index d0a97c7..6b046de 100644 --- a/include/reflection.h +++ b/include/reflection.h @@ -195,7 +195,10 @@ static std::string CAT(Register, REFLECTABLE)() { ReflectionRegister& instance = *GetReflectionRegisterInstance(); auto isRecordRegisterd = instance.records.find(type_id) != instance.records.end(); if (!isRecordRegisterd) { - auto& reflectable = GetRecordFromTypeId(type_id); + // Create the entry directly (operator[] default-inserts on miss); this is registration's + // own create path, scoped by the find() guard above, and is intentionally not routed + // through GetRecordFromTypeId, which is a pure lookup that throws on a miss + auto& reflectable = instance.records[type_id]; reflectable.name = name; // store member metadata diff --git a/src/reflection.cc b/src/reflection.cc index d1e50b3..20934fa 100644 --- a/src/reflection.cc +++ b/src/reflection.cc @@ -23,6 +23,7 @@ #include "reflection.h" #include +#include static std::unique_ptr p = nullptr; @@ -35,8 +36,11 @@ ReflectionRegister* GetReflectionRegisterInstance() { Reflection& GetRecordFromTypeId(const std::string& type_id) { ReflectionRegister& instance = *GetReflectionRegisterInstance(); - auto& meta_struct = instance.records[type_id]; - return meta_struct; + auto it = instance.records.find(type_id); + if (it == instance.records.end()) { + throw std::runtime_error("Reflection lookup failed: type not registered: " + type_id); + } + return it->second; } char* GetMemberAddress(void* precord, const Reflection& record, const size_t i) { diff --git a/tests/query_predicates_test.cc b/tests/query_predicates_test.cc index f3de274..167c194 100644 --- a/tests/query_predicates_test.cc +++ b/tests/query_predicates_test.cc @@ -207,3 +207,38 @@ TEST(QueryPredicatesTest, LikeInsideAndSurvivesCloneWithEscapeClause) { EXPECT_EQ(R"(%50\%%)", bindings[0].text_value); EXPECT_EQ(30, bindings[1].int_value); } + +namespace { +// A plain struct that is deliberately never run through the REFLECTABLE/FIELDS registration +// macros, so its type id never appears in the reflection registry. +struct UnregisteredRecord { + int64_t id; + int64_t value; +}; + +// A plain struct that is also never run through the registration macros, but is manually and +// incompletely registered below (name only, no member metadata) to exercise the +// registered-but-offset-mismatch guard, as distinct from the unregistered-type guard above. +struct MismatchedRecord { + int64_t id; + int64_t value; +}; +} // namespace + +TEST(QueryPredicatesTest, PredicateConstructionThrowsForUnregisteredType) { + // #23: GetRecordFromTypeId must fail fast for a type that was never registered, instead of + // std::map::operator[] silently default-inserting an empty Reflection (empty table name, no + // columns), which would otherwise surface later as an opaque SQLite prepare error + EXPECT_THROW(Equal(&UnregisteredRecord::value, 42), std::runtime_error); +} + +TEST(QueryPredicatesTest, PredicateConstructionThrowsWhenNoMemberMatches) { + // #22: even for a registered type, if no member_metadata entry's offset matches the + // pointer-to-member (here because the type was registered by hand with no members at all, + // rather than via the FIELDS macro), the QueryPredicate constructor must fail fast instead + // of silently leaving member_name_ empty and emitting malformed SQL like " = ?" + auto& instance = *GetReflectionRegisterInstance(); + instance.records[typeid(MismatchedRecord).name()].name = "MismatchedRecord"; + + EXPECT_THROW(Equal(&MismatchedRecord::value, 42), std::runtime_error); +} From dcc2d175f43fbabc718dc53c8d80381eaade607b Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 7 Jul 2026 19:25:25 +0000 Subject: [PATCH 2/2] Clean up the hand-registered test type after the expectation PredicateConstructionThrowsWhenNoMemberMatches manually inserted a MismatchedRecord entry into the process-wide reflection registry (with no member metadata, to force an offset-match miss) but never removed it. Left in place, Database::Database iterates every registered record on Initialize() and would generate "CREATE TABLE IF NOT EXISTS MismatchedRecord ();" (empty column list) for it - invalid SQL that throws and breaks every later Database::Initialize() call in the same test binary, depending on test execution order. This didn't surface in a normal sequential run, but reliably broke under --gtest_shuffle: 4 of 5 random seeds failed with 6-37 cascading test failures before this fix, and all of 10 random seeds pass after it. Added a small RAII ScopedRegistryCleanup that erases the hand-inserted entry on scope exit. Co-Authored-By: Claude Sonnet 5 Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK --- tests/query_predicates_test.cc | 23 ++++++++++++++++++++++- 1 file changed, 22 insertions(+), 1 deletion(-) diff --git a/tests/query_predicates_test.cc b/tests/query_predicates_test.cc index 167c194..079152f 100644 --- a/tests/query_predicates_test.cc +++ b/tests/query_predicates_test.cc @@ -24,6 +24,9 @@ #include +#include +#include + #include "person.h" #include "pet.h" @@ -223,6 +226,22 @@ struct MismatchedRecord { int64_t id; int64_t value; }; + +// Erases a hand-inserted entry from the process-wide reflection registry on scope exit. Without +// this, a MismatchedRecord-shaped entry with no member metadata would linger in the registry for +// the rest of the test binary: Database::Database iterates every registered record and would +// generate "CREATE TABLE IF NOT EXISTS MismatchedRecord ();" (empty column list) for it, failing +// every later Database::Initialize() call in this process. +class ScopedRegistryCleanup { +public: + explicit ScopedRegistryCleanup(std::string type_id) : type_id_(std::move(type_id)) {} + ~ScopedRegistryCleanup() { + GetReflectionRegisterInstance()->records.erase(type_id_); + } + +private: + std::string type_id_; +}; } // namespace TEST(QueryPredicatesTest, PredicateConstructionThrowsForUnregisteredType) { @@ -237,8 +256,10 @@ TEST(QueryPredicatesTest, PredicateConstructionThrowsWhenNoMemberMatches) { // pointer-to-member (here because the type was registered by hand with no members at all, // rather than via the FIELDS macro), the QueryPredicate constructor must fail fast instead // of silently leaving member_name_ empty and emitting malformed SQL like " = ?" + const std::string type_id = typeid(MismatchedRecord).name(); auto& instance = *GetReflectionRegisterInstance(); - instance.records[typeid(MismatchedRecord).name()].name = "MismatchedRecord"; + instance.records[type_id].name = "MismatchedRecord"; + const ScopedRegistryCleanup cleanup(type_id); EXPECT_THROW(Equal(&MismatchedRecord::value, 42), std::runtime_error); }