Add validation for type registration in QueryPredicate construction#34
Conversation
…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
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 398c870e16
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // 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"; |
There was a problem hiding this comment.
This inserts a permanent record into the process-wide reflection registry. If this test runs before any database-initializing test in the same unit_tests process, for example with --gtest_shuffle or a local filter that runs DB tests afterward, Database::Database iterates every registry entry and CreateTableQuery::PrepareSql() will try to create MismatchedRecord with an empty column list (CREATE TABLE IF NOT EXISTS MismatchedRecord ();), causing initialization to throw. Use RAII cleanup/erase around this expectation so the test does not leak global state.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Confirmed and fixed in dcc2d17, with a stronger repro than I expected: running the pre-fix suite with --gtest_shuffle across 5 random seeds failed 4 of them (6-37 cascading failures each) exactly as you described - only the unshuffled default order happened to dodge it. Added a small RAII ScopedRegistryCleanup that erases the hand-inserted MismatchedRecord entry on scope exit; reran 10 shuffled seeds post-fix and all pass.
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
|
@codex review |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
2 participants
Summary
This PR adds fail-fast validation to prevent silent failures when constructing query predicates with unregistered types or mismatched member offsets. Previously, these errors would surface later as opaque SQLite errors; now they throw immediately with descriptive messages.
Key Changes
GetRecordFromTypeId()to throwstd::runtime_errorwhen a type is not found in the reflection registry, instead of silently creating an empty entry viaoperator[]QueryPredicateconstructor to ensure the pointer-to-member offset matches a registered member; throws if no match is foundoperator[]for creation (scoped by an existence check), preserving the original behavior for registration while keepingGetRecordFromTypeId()as a pure lookup functionImplementation Details
REFLECTABLE/FIELDSmacroshttps://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK