ladder: make the zero-sentinel id constraint explicit and enforced - #260
Open
Yaraslaut wants to merge 2 commits into
Open
ladder: make the zero-sentinel id constraint explicit and enforced#260Yaraslaut wants to merge 2 commits into
Yaraslaut wants to merge 2 commits into
Conversation
polls::OptionId, polls::PollEventId and kanban::BoardEventId use `value == 0` as their "not entered" state, so an id of 0 is unrepresentable: construct one and it reports hasValue() == false and behaves as absent everywhere downstream. Nothing is broken today -- all three come from SQLite row ids, which start at 1 -- but the constraint was implicit, and the failure mode if a 0 ever did arrive (a seeded row, a migrated dataset, an externally supplied key, a sequence reset) is that a real record reads as "no record". #215 offers three options. This takes (2), documented-and-enforced, not (1), convert-to-optional-backed, for two reasons the issue does not account for: - kanban's types.hpp already documents the zero-sentinel shape as a deliberate choice ("it is always looked up already-assigned", citing design spec §7), and PollEventId{} is the natural spelling of "no cursor yet, replay from the beginning" for GetEventsSince. Converting would reverse a recorded decision. - The optional-backed shape changes the empty state's wire form from 0 to null for two shipped rungs. Enforcement is a checked factory, fromRowId(), which every conversion from a stored row id now goes through; it rejects 0 loudly instead of letting it collapse into the empty state one layer below the QML surface, where no conversion helper can restore the distinction. Six conversion sites in poll_model.cpp and board_model.cpp adopt it. One site deliberately does not: decodeVotes() in poll_qml_bridges.cpp builds an OptionId from *QML-supplied* input, where toLongLong() yields 0 for a missing or non-numeric field -- which is precisely the "not entered" state the action's validate() exists to reject, as a clean ValidationError rather than an exception. That distinction now has a comment. Also corrects polls/core/types.hpp's file comment, which claimed these two follow BookmarkId's pattern. They do not -- BookmarkId is optional-backed, and that mismatch is the documentation defect underneath this issue. Verified: polls + kanban ladder suites 198/198, including the new fromRowId cases and their control cases (a factory that rejected everything would pass the rejection test alone). clang-tidy-diff and -Wdocumentation clean. Closes #215
The original run was cancelled by the supersede-obsolete-runs concurrency rule (#257) shortly after it merged, and re-running the cancelled workflows produced attempts that were themselves cancelled within minutes. An empty commit gives the PR a fresh head so its checks run from a clean slate. No content change.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #215.
The defect
polls::OptionId,polls::PollEventIdandkanban::BoardEventIdusevalue == 0as their "not entered" state, so an id of
0is unrepresentable — constructone and it reports
hasValue() == falseand behaves as absent everywheredownstream.
Nothing is broken today: all three come from SQLite row ids, which start at 1.
It matters because the constraint was implicit, and the failure mode if a
0ever does arrive — a seeded row, a migrated dataset, an externally supplied key,
a sequence reset — is that a real record reads as "no record", one layer below
the QML boundary where no conversion helper can restore the distinction.
Why option 2, not option 1
The issue offers three options and notes that (1), converting to the
optional-backed shape, "is the only one that removes the trap rather than
documenting it". I took (2), documented-and-enforced, for two reasons the
issue does not account for:
types.hppsays the shape is chosen because the id "is always looked up already-assigned",
citing design spec §7 — and
PollEventId{}is the natural spelling of "nocursor yet, replay from the beginning" for
GetEventsSince. Converting wouldreverse a recorded decision.
empty state as
nullrather than0, for two shipped rungs.If you'd rather have uniformity with
BookmarkIdacross the ladder, option 1 isstill open — but it's a decision about a documented choice, not a cleanup, which
is why I didn't take it unilaterally.
Enforcement, not just prose
A checked factory,
fromRowId(), which every conversion from a stored row id nowgoes through — six sites in
poll_model.cppandboard_model.cpp. It rejects0loudly instead of letting it collapse into the empty state.One site deliberately does not use it.
decodeVotes()inpoll_qml_bridges.cppbuilds anOptionIdfrom QML-supplied input, wheretoLongLong()yields0for a missing or non-numeric field — precisely the "notentered" state the action's
validate()exists to reject, as a cleanValidationErrorrather than an exception. That distinction now carries acomment, since it's exactly what a future reader would trip on.
A documentation defect underneath
polls/core/types.hppclaimed these two ids followBookmarkId's pattern.They don't —
BookmarkIdis optional-backed, which is the whole point of thisissue. Corrected, with the real constraint and its rationale recorded on all
three types.
Verification
fromRowIdcasesclang-tidy-diff(clang 22, as CI pins)-Wdocumentation -Werroron both changed headersThe last two were run locally because #258 showed CI's
clang-tidy-diffand theWASM job's
-Weverythingcatch things an ordinary local build does not.