Skip to content

Commit 11812e4

Browse files
jkaliasclaude
andauthored
Add validation for type registration in QueryPredicate construction (#34)
* 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<T>() 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<T>() (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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK * 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 08dcf6a commit 11812e4

4 files changed

Lines changed: 73 additions & 3 deletions

File tree

include/query_predicates.h

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,7 @@
2424

2525
#include <functional>
2626
#include <memory>
27+
#include <stdexcept>
2728
#include <string>
2829
#include <vector>
2930

@@ -82,13 +83,19 @@ class REFLECTION_EXPORT QueryPredicate : public QueryPredicateBase {
8283
: symbol_(symbol) {
8384
auto record = GetRecordFromTypeId(typeid(T).name());
8485
auto offset = OffsetFromStart(fn);
86+
auto found = false;
8587
for (auto i = 0; i < record.member_metadata.size(); ++i) {
8688
if (record.member_metadata[i].offset == offset) {
8789
member_name_ = record.member_metadata[i].name;
8890
value_ = value_retrieval((void*)&value, record.member_metadata[i].storage_class);
91+
found = true;
8992
break;
9093
}
9194
}
95+
if (!found) {
96+
throw std::runtime_error("No registered member of '" + record.name +
97+
"' matches the given pointer-to-member (type id: " + typeid(T).name() + ")");
98+
}
9299
}
93100

94101
template <typename T, typename R>

include/reflection.h

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -195,7 +195,10 @@ static std::string CAT(Register, REFLECTABLE)() {
195195
ReflectionRegister& instance = *GetReflectionRegisterInstance();
196196
auto isRecordRegisterd = instance.records.find(type_id) != instance.records.end();
197197
if (!isRecordRegisterd) {
198-
auto& reflectable = GetRecordFromTypeId(type_id);
198+
// Create the entry directly (operator[] default-inserts on miss); this is registration's
199+
// own create path, scoped by the find() guard above, and is intentionally not routed
200+
// through GetRecordFromTypeId, which is a pure lookup that throws on a miss
201+
auto& reflectable = instance.records[type_id];
199202
reflectable.name = name;
200203

201204
// store member metadata

src/reflection.cc

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,7 @@
2323
#include "reflection.h"
2424

2525
#include <memory>
26+
#include <stdexcept>
2627

2728
static std::unique_ptr<ReflectionRegister> p = nullptr;
2829

@@ -35,8 +36,11 @@ ReflectionRegister* GetReflectionRegisterInstance() {
3536

3637
Reflection& GetRecordFromTypeId(const std::string& type_id) {
3738
ReflectionRegister& instance = *GetReflectionRegisterInstance();
38-
auto& meta_struct = instance.records[type_id];
39-
return meta_struct;
39+
auto it = instance.records.find(type_id);
40+
if (it == instance.records.end()) {
41+
throw std::runtime_error("Reflection lookup failed: type not registered: " + type_id);
42+
}
43+
return it->second;
4044
}
4145

4246
char* GetMemberAddress(void* precord, const Reflection& record, const size_t i) {

tests/query_predicates_test.cc

Lines changed: 56 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,9 @@
2424

2525
#include <gtest/gtest.h>
2626

27+
#include <string>
28+
#include <utility>
29+
2730
#include "person.h"
2831
#include "pet.h"
2932

@@ -207,3 +210,56 @@ TEST(QueryPredicatesTest, LikeInsideAndSurvivesCloneWithEscapeClause) {
207210
EXPECT_EQ(R"(%50\%%)", bindings[0].text_value);
208211
EXPECT_EQ(30, bindings[1].int_value);
209212
}
213+
214+
namespace {
215+
// A plain struct that is deliberately never run through the REFLECTABLE/FIELDS registration
216+
// macros, so its type id never appears in the reflection registry.
217+
struct UnregisteredRecord {
218+
int64_t id;
219+
int64_t value;
220+
};
221+
222+
// A plain struct that is also never run through the registration macros, but is manually and
223+
// incompletely registered below (name only, no member metadata) to exercise the
224+
// registered-but-offset-mismatch guard, as distinct from the unregistered-type guard above.
225+
struct MismatchedRecord {
226+
int64_t id;
227+
int64_t value;
228+
};
229+
230+
// Erases a hand-inserted entry from the process-wide reflection registry on scope exit. Without
231+
// this, a MismatchedRecord-shaped entry with no member metadata would linger in the registry for
232+
// the rest of the test binary: Database::Database iterates every registered record and would
233+
// generate "CREATE TABLE IF NOT EXISTS MismatchedRecord ();" (empty column list) for it, failing
234+
// every later Database::Initialize() call in this process.
235+
class ScopedRegistryCleanup {
236+
public:
237+
explicit ScopedRegistryCleanup(std::string type_id) : type_id_(std::move(type_id)) {}
238+
~ScopedRegistryCleanup() {
239+
GetReflectionRegisterInstance()->records.erase(type_id_);
240+
}
241+
242+
private:
243+
std::string type_id_;
244+
};
245+
} // namespace
246+
247+
TEST(QueryPredicatesTest, PredicateConstructionThrowsForUnregisteredType) {
248+
// #23: GetRecordFromTypeId must fail fast for a type that was never registered, instead of
249+
// std::map::operator[] silently default-inserting an empty Reflection (empty table name, no
250+
// columns), which would otherwise surface later as an opaque SQLite prepare error
251+
EXPECT_THROW(Equal(&UnregisteredRecord::value, 42), std::runtime_error);
252+
}
253+
254+
TEST(QueryPredicatesTest, PredicateConstructionThrowsWhenNoMemberMatches) {
255+
// #22: even for a registered type, if no member_metadata entry's offset matches the
256+
// pointer-to-member (here because the type was registered by hand with no members at all,
257+
// rather than via the FIELDS macro), the QueryPredicate constructor must fail fast instead
258+
// of silently leaving member_name_ empty and emitting malformed SQL like " = ?"
259+
const std::string type_id = typeid(MismatchedRecord).name();
260+
auto& instance = *GetReflectionRegisterInstance();
261+
instance.records[type_id].name = "MismatchedRecord";
262+
const ScopedRegistryCleanup cleanup(type_id);
263+
264+
EXPECT_THROW(Equal(&MismatchedRecord::value, 42), std::runtime_error);
265+
}

0 commit comments

Comments
 (0)