Implement nullable reflected fields#37
Conversation
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5806a127c9
ℹ️ 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".
| return is_id ? name + " PRIMARY KEY AUTOINCREMENT" | ||
| : (record_.member_metadata[index].nullable ? name : name + " NOT NULL"); |
There was a problem hiding this comment.
Update legacy NULL insert coverage before enforcing NOT NULL
When a fresh test database is initialized, this now creates existing non-nullable records such as Company with name/address declared NOT NULL. The existing FetchPreservesNullAndEmptyStringSkipSemantics test still uses UnsafeSql to omit those columns (INSERT INTO Company (id, name, age, salary) ... and INSERT INTO Company (id, age, address, salary) ...), so sqlite3_exec will fail on the constraint before the fetch behavior is exercised. Unless that coverage is migrated to a pre-existing/legacy schema or nullable fields, CI will fail on a clean database.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6296389c75
ℹ️ 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".
| SqlValue QueryPredicate::GetOptionalSqlValue(void* v, SqliteStorageClass storage_class) const { | ||
| switch (storage_class) { | ||
| case SqliteStorageClass::kInt: | ||
| return GetSqlValue(&reinterpret_cast<fcpp::optional_t<int64_t>*>(v)->value(), storage_class); |
There was a problem hiding this comment.
Use const-safe extraction for C++11 optional values
In C++11 builds that use functional_cpp's fallback optional, value() returns const T&, so &...value() is a const T*. Passing that to GetSqlValue(void*) is rejected by Clang/MSVC in the supported C++11 workflow jobs (GCC only masks it with -fpermissive -w), so nullable predicate support breaks those builds. Either make the value extraction const-safe or avoid taking a mutable pointer here.
Useful? React with 👍 / 👎.
| template <typename T, typename R> | ||
| explicit Like(fcpp::optional_t<R> T::* fn, R value) | ||
| : QueryPredicate(fn, fcpp::optional_t<R>(value), "LIKE", [&](void* v, SqliteStorageClass storage_class) { | ||
| return GetOptionalSqlValue(v, storage_class); |
There was a problem hiding this comment.
Apply LIKE binding semantics to nullable fields
When a Like predicate is created for a nullable field, this path unwraps the optional through the base helper and never applies Like::GetSqlValue, which is the code that converts the binding to text, wraps it in %...%, and escapes user-supplied %/_. In C++17/GCC builds where this compiles, Like(&T::optional_text, std::wstring(L"%")) binds raw % and matches every non-NULL row instead of a literal percent, while ordinary substring searches become exact matches.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: aefc4f9482
ℹ️ 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".
| explicit Equal(R T::* fn, R value) : QueryPredicate(fn, value, "=") {} | ||
|
|
||
| template <typename T, typename R> | ||
| explicit Equal(fcpp::optional_t<R> T::* fn, R value) : QueryPredicate(fn, value, "=") {} |
There was a problem hiding this comment.
Reject optional control values for nullable predicates
When callers pass an fcpp::optional_t<T> control value to a nullable field, e.g. Equal(&NullableRecord::optional_text, fcpp::optional_t<std::wstring>{}), overload resolution bypasses this contained-value overload and selects the generic Equal(R T::*, R) overload instead. That path treats the optional object's storage as the contained T, so empty optionals can bind garbage or throw rather than performing a null-aware comparison; either delete optional-valued controls or route them to IS NULL/contained-value handling.
Useful? React with 👍 / 👎.
|
Fixed by PR #38 |
Motivation
NULLwas indistinguishable from default/empty values and dropped on fetch.optionalimplementation fromfunctional_cppso the type aliases tostd::optionalon C++17+ toolchains while keeping C++11 compatibility.Description
functional_cppas a FetchContent dependency (pinned to tag1.1.1) and expose it to thesrctarget so public headers can usefcpp::optional_t<T>.nullableflag and introduceMEMBER_INT_NULLABLE,MEMBER_REAL_NULLABLE,MEMBER_TEXT_NULLABLE,MEMBER_DATETIME_NULLABLE, andMEMBER_BOOL_NULLABLEmacros that declarefcpp::optional_t<T>members and register them as nullable.NOT NULLfor non-nullable reflected columns and omitNOT NULLfor nullable columns (theidcolumn still receivesPRIMARY KEY AUTOINCREMENT).NULL(sqlite3_bind_null), non-empty optionals bind their contained value, andHydrateCurrentRowsets an optional to empty onSQLITE_NULLwhile preserving the existing NULL/BLOB safety path for non-nullable members.IsNull(&T::field)andIsNotNull(&T::field)predicates (no bindings) and extend value predicates to accept member-pointers to nullable fields (they operate on the contained value); provide anGetOptionalSqlValuehelper to extract contained values for bindings.Defining recordssection to document the nullable macros, round-trip semantics,NOT NULLmigration caveat, and thatfcpp::optional_t<T>aliasesstd::optional<T>under C++17+.tests/nullable_record.hplus tests updates) that cover round-trip null vs default/empty distinction across all storage classes, schemaNOT NULLbehavior,IsNull/IsNotNullpredicates, nullable value predicates, and a C++17static_assertthat the optional type aliasesstd::optionalwhen available.Testing
cmake -S . -B buildcould not complete because the environment proxy returned HTTP 403 while FetchContent attempted to download external archives (GoogleTest/FetchContent step), so a full build/test run was not possible in this environment (network error).g++ -std=c++11 -Iinclude -Isrc -I/tmp/fcpp -fsyntax-only src/queries.cc src/query_predicates.ccsucceeded (checked binding/hydration/predicate codepaths).gtestand the temporaryfunctional_cppheader) succeeded fortests/database_test.ccandtests/query_predicates_test.ccunder both C++11 and C++17 compile-time checks, including thestatic_assertforstd::optionalaliasing under C++17.git diff --checkand basic repository checks; no whitespace/diff-check errors reported.Closes #2
Closes #20
Codex Task