Implement nullable reflected fields#38
Conversation
fcpp::optional_t<T> is the nullable-field representation: it aliases std::optional<T> under C++17 and later and falls back to an equivalent C++11 implementation otherwise. Consumed header-only via FetchContent, pinned to tag 1.1.1; SOURCE_SUBDIR points at the header directory (which has no CMakeLists.txt) so only the sources are downloaded, without building functional_cpp's own library and test targets. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
#2) Each field macro gains a _NULLABLE variant declaring the member as fcpp::optional_t of the underlying type (std::optional under C++17+). MemberMetadata carries an orthogonal nullable flag - storage_class keeps describing the underlying contained type - set via the new DEFINE_MEMBER_NULLABLE during registration, so query generation, binding, and hydration can branch on it. No behavior change yet for existing records: the flag defaults to false everywhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
…and predicates (#2, #20) Write side: SqlValue gains an is_null state bound via sqlite3_bind_null; GetValues reads a nullable member through its optional and produces a null SqlValue when it is empty. Read side (the #20 fix): HydrateCurrentRow hydrates a nullable member's SQL NULL into an empty optional instead of skipping it, and a present value into the contained value - an empty TEXT becomes a contained empty string, distinct from NULL, ending the NULL/empty conflation. Non-nullable members keep today's exact skip semantics, including the NULL/BLOB accessor-safety skip. Schema: CreateTableQuery emits NOT NULL for non-nullable columns and omits it for nullable ones (id keeps PRIMARY KEY AUTOINCREMENT, which is implicitly NOT NULL). Only newly created tables are affected, since tables are created with CREATE TABLE IF NOT EXISTS. The pre-existing NULL-skip-semantics test now recreates its table the way a legacy database file would look, which is the very scenario those skip semantics protect. Predicates: new IsNull()/IsNotNull() emit "<column> IS NULL"/"IS NOT NULL" with no bound value and survive Clone() inside And()/Or() compounds. Value predicates on a nullable member compare against the contained value; given an empty optional they throw std::invalid_argument, since "= NULL" never matches in SQL - IsNull()/IsNotNull() is the correct tool there. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
NullableRecord mixes a non-nullable member with a nullable member of every storage class. Covered: unset fields round-trip as SQL NULL (with IS NULL matching at the SQL level as raw proof); set fields round-trip equal; NULL is distinguishable from a stored empty string and stored zero after fetch (the #20 core case); non-nullable columns reject NULL while nullable ones accept it (schema NOT NULL, asserted behaviorally); IsNull/IsNotNull select the right rows, also inside And() compounds; value predicates operate on the contained value and fail fast on an empty optional; a record without nullable members round-trips unchanged. Under C++17+ a static_assert pins that nullable fields are literally std::optional. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
Covers the MEMBER_*_NULLABLE macros, the fcpp::optional_t representation (std::optional under C++17+, C++11 fallback otherwise), NULL round-trip semantics, IsNull/IsNotNull and value-predicate behavior, and the NOT NULL migration caveat for pre-existing database files, mirroring the existing AUTOINCREMENT caveat. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
Apple Clang errors on &IsNull(...) with -Waddress-of-temporary (taking the address of a temporary object), failing both macOS CI jobs; GCC only tolerated it via the project's -fpermissive. Bind each predicate to a named const local first, matching how every other predicate call site in the tests already does it. 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: 0524f43afa
ℹ️ 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".
| functional_cpp | ||
| GIT_REPOSITORY https://github.com/jkalias/functional_cpp.git | ||
| GIT_TAG 1.1.1 | ||
| SOURCE_SUBDIR include |
There was a problem hiding this comment.
This line relies on FetchContent's SOURCE_SUBDIR handling to avoid adding functional_cpp's own CMake project, but I checked cmake --help-module FetchContent and that support is marked versionadded 3.18 while this project still advertises cmake_minimum_required(VERSION 3.14). On CMake 3.14-3.17, configuring this commit won't honor the subdir avoidance and can add functional_cpp's top-level CMakeLists.txt, pulling its src/tests into this build rather than just the headers; either raise the minimum to 3.18 or fetch the headers without SOURCE_SUBDIR.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Good catch — SOURCE_SUBDIR in FetchContent_Declare is indeed 3.18+, while this project declares a 3.14 minimum. Fixed in 3ee9551 without raising the minimum: the population is now version-guarded. On CMake < 3.18 we use FetchContent_GetProperties + FetchContent_Populate, which downloads the sources without ever calling add_subdirectory, so functional_cpp's own src/tests targets stay out of the build; on 3.18+ the original SOURCE_SUBDIR include + FetchContent_MakeAvailable path is kept. (FetchContent_Populate is deprecated in CMake 3.30, but that branch is unreachable there since it's behind the VERSION_LESS 3.18 check.) Verified with a fresh configure + full build + 83/83 tests passing.
Generated by Claude Code
FetchContent's SOURCE_SUBDIR handling was added in CMake 3.18, but the project's minimum is 3.14; on 3.14-3.17, MakeAvailable would have add_subdirectory'd functional_cpp's top-level CMakeLists.txt, pulling its own src/tests into this build. Branch on the CMake version: 3.18+ keeps the SOURCE_SUBDIR MakeAvailable path, older versions populate the sources via FetchContent_Populate, which never calls add_subdirectory at all. The deprecated-in-3.30 Populate call sits behind the VERSION_LESS 3.18 check, so modern CMake never executes it. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3ee9551751
ℹ️ 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".
| if (record.member_metadata[i].nullable) { | ||
| // A value predicate on a nullable member compares against the contained | ||
| // value; an empty optional cannot be expressed as "= ?" in SQL (a bound | ||
| // NULL never matches), so demand IsNull()/IsNotNull() for that instead | ||
| value_address = NullableContainedValue(value_address, record.member_metadata[i].storage_class); |
There was a problem hiding this comment.
When this nullable-member branch is added, it only helps predicate classes whose constructors accept the member's actual fcpp::optional_t<T> type. The range predicates below still expose only int64_t T::* and double T::* overloads, so a natural nullable numeric query such as GreaterThan(&Record::maybe_int, fcpp::optional_t<int64_t>(5)) does not compile even though nullable value predicates are documented as comparing against the contained value. Add optional/generic overloads for the range predicates as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Agreed — Equal/Unequal/Like have a generic R T::* constructor that deduces nullable member types, but the four range predicates only exposed int64_t T::* and double T::* overloads, so a nullable numeric member couldn't be used with them at all. Fixed in c162223: GreaterThan, GreaterThanOrEqual, SmallerThan and SmallerThanOrEqual now each have fcpp::optional_t<int64_t> and fcpp::optional_t<double> overloads that route through the existing QueryPredicate constructor, so they get the same contained-value comparison and the same fail-fast std::invalid_argument on an empty optional. Covered by a new test (RangePredicatesOperateOnTheContainedValue) exercising all four predicates on nullable INT and REAL members, including NULL rows never matching and the empty-optional throw; 84/84 tests pass under both C++11 and C++20. The README nullable-predicate section now mentions the range predicates too.
Generated by Claude Code
GreaterThan, GreaterThanOrEqual, SmallerThan and SmallerThanOrEqual only exposed int64_t and double member-pointer overloads, so they could not be used with nullable numeric members at all, even though value predicates on nullable members are documented to compare against the contained value. Add fcpp::optional_t<int64_t> and fcpp::optional_t<double> overloads that route through the existing QueryPredicate constructor, which already unwraps the optional and fails fast on an empty one. Also fix the README nullness-predicate example, which took the address of a temporary (the pattern Clang and MSVC reject), and mention the range predicates in the nullable value-predicate documentation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NUt3c1wdseRtRSSXfg3MCK
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c162223928
ℹ️ 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".
| Employee e; | ||
| e.name = L"Ada Lovelace"; // middle_name and salary stay unset | ||
| db->Save(e); // unset fields are stored as SQL NULL | ||
|
|
||
| const auto fetched = db->Fetch<Employee>(e.id); |
There was a problem hiding this comment.
When users copy this new example as written, Employee e; leaves the appended int64_t id indeterminate, and Database::Save (unlike SaveAutoIncrement) reads and binds that id without writing one back. The subsequent fetch by e.id therefore uses an uninitialized value; set e.id before Save or use SaveAutoIncrement(e) before fetching.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Right — Save binds the id as given and never writes one back, so the example as written saved an indeterminate id and then fetched by it. Fixed in 2711eb3: the example now uses db->SaveAutoIncrement(e), which lets the database assign the id and writes it back into e, making the follow-up Fetch<Employee>(e.id) correct as copied. This matches the README's own guidance further down ("Use SaveAutoIncrement when you want the library to assign the next available id").
Generated by Claude Code
The example default-constructed an Employee and saved it via Save, which binds the appended int64_t id without ever writing one back - so the id stayed indeterminate and the subsequent fetch by e.id read an uninitialized value. SaveAutoIncrement assigns the id and writes it back into the object, making the example correct as written. 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. Chef's kiss. 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
Closes #2. Closes #20.
These two issues share one root cause: a reflected field had no way to represent the absence of a value. Every field macro expanded to an always-present C++ type, so the read path could only skip a SQL
NULL(leaving the member at its default — #20's NULL/empty/zero conflation), and the model couldn't declare, write, or query nullable columns at all (#2). This PR adds the missing representation and delivers the whole nullable-field feature; the conflation disappears as a consequence.The representation —
fcpp::optional_tfrom functional_cppNullable fields are declared via new
MEMBER_*_NULLABLEmacros (all five storage classes) and typed asfcpp::optional_t<T>, pulled in via FetchContent pinned to tag1.1.1, consumed header-only (itsSOURCE_SUBDIRhas no CMakeLists, so none of its own library/test targets build). Verified in its source: under C++17+optional_t<T>is literallyusing optional_t = std::optional<T>— astatic_assertin the tests pins this — with an equivalent C++11 fallback otherwise (has_value(),value(),operator*/->, assignment). The C++11 minimum is unchanged. Layout-wise anoptionalmember fits the same pattern as the existing non-standard-layoutwstring/TimePointmembers: non-polymorphic value type, default-constructs to empty, works underoffsetof.What changed
MEMBER_INT/REAL/TEXT/BOOL/DATETIME_NULLABLE;MemberMetadatagains an orthogonalnullableflag (storage_classkeeps describing the underlying type).SqlValuegains anis_nullstate bound viasqlite3_bind_null;GetValuesreads nullable members through their optional (empty → NULL).HydrateCurrentRowhydrates SQLNULLin a nullable member to an empty optional instead of skipping; a stored empty TEXT hydrates to a contained empty string — NULL, empty, and zero are now all distinguishable. Non-nullable members keep today's exact skip semantics, including the NULL/BLOB accessor-safety skip.NOT NULL; nullable ones don't (idkeepsPRIMARY KEY AUTOINCREMENT, implicitly NOT NULL). Because tables useCREATE TABLE IF NOT EXISTS, this affects newly created tables only; pre-existing files keep their schema until migrated — documented in the README exactly like theAUTOINCREMENTcaveat.IsNull(&T::field)/IsNotNull(&T::field)emitIS NULL/IS NOT NULLwith no bound value and surviveClone()insideAnd()/Or(). Value predicates on a nullable member compare against the contained value (pass an optional, e.g.Equal(&T::field, fcpp::optional_t<int64_t>(7))); handing them an empty optional throwsstd::invalid_argumentwith a pointer toIsNull/IsNotNull, since= NULLnever matches in SQL.Backward compatibility
Additive: records without nullable macros round-trip identically, byte for byte — the only observable difference is
NOT NULLon freshly created tables. One existing test (FetchPreservesNullAndEmptyStringSkipSemantics) inserted NULLs through omitted columns, which the newNOT NULLschema now correctly rejects; its setup now recreates the table the way a legacy/foreign database file would look (the exact scenario its skip-semantics assertions protect), with all assertions unchanged.Tests
New
NullableRecord(one non-nullable member + a nullable member of every storage class), covering:NULL, withIS NULLmatching at the SQL level as raw proof of storageIsNull/IsNotNullrow selection, including insideAnd()compounds (Clone survival)static_assertthat nullable fields arestd::optionalunder C++17+Full suite: 83/83 pass in both C++11 (fallback optional) and C++20 (
std::optional) locally on Linux/GCC, plus shuffled-order runs for isolation. macOS/Clang and Windows/MSVC verification via CI (note: MSVC's/Zc:__cplusplus, already set in this project, is what makes functional_cpp's C++17 detection work there).Generated by Claude Code