You signed in with another tab or window. Reload to refresh your session.You signed out in another tab or window. Reload to refresh your session.You switched accounts on another tab or window. Reload to refresh your session.Dismiss alert
{{ message }}
Repository navigation
Evaluate std::optional for Token row identifiers and kInvalidRowId consistency #833
Evaluate whether Token should represent its optional row identifier with std::optional<std::uint64_t>.
This follow-up is intentionally out of scope for PR #830.
Required evaluation
Evaluate replacing Token::m_id and Token::m_hasId with std::optional<std::uint64_t>.
Evaluate the related public API change. This includes whether to replace the id() and hasId() accessor pair with an optional-returning accessor.
Evaluate whether form/storage/istorage.hpp should stop using the write-chain sentinel kInvalidRowId in the same change.
Keep the Token representation and the write-chain representation consistent. Document the selected contract.
Rationale
Reviewers requested discussion of the std::optional representation. The current design uses an explicit validity flag in Token and a sentinel in the write chain. A single follow-up should decide whether those representations remain separate or change together.
Affected areas
form/core/token.hpp
form/core/token.cpp
form/storage/istorage.hpp
Write implementations that return row identifiers
Read and write call sites and tests for Token
Acceptance criteria
The issue records a decision for the Token identifier representation.
The issue records a decision for kInvalidRowId and explains how the write-chain and Token contracts remain consistent.
If an implementation change is selected, the follow-up updates the API documentation and tests for identified and non-addressable rows.
The design decision for this issue is now clear, and the code has moved in a way that settles it.
Decision: keep invalid_row_id (form/storage/istorage.hpp) as the storage-layer indication that a backend does not address rows, and keep tokens produced by the write path always identified. The row ID does not need to become std::optional.
Why the two contracts stay consistent: persistence_writer::register_write now rejects invalid_row_id and throws rather than returning a token for it, on the grounds that such a token could not locate the product on read. The sentinel therefore never reaches a token — it lives entirely in the storage layer, and token is identified by construction. There is no second representation to reconcile.
On has_id() and the placement-only constructor: these now have no callers outside tests. The only production use of token::id() is on the read path in storage_reader.cpp, and it does not guard on has_id(). Removing them is a small cleanup to fold into the next PR that touches token, not part of this decision. (The members were also renamed from m_id/m_hasId to id_/has_id_ by #831.)
No implementation is required for this issue, so I'm closing it.
Activity
wwuoneway commentedon Oct 1, 2026
The design decision for this issue is now clear, and the code has moved in a way that settles it.
Decision: keep
invalid_row_id(form/storage/istorage.hpp) as the storage-layer indication that a backend does not address rows, and keep tokens produced by the write path always identified. The row ID does not need to becomestd::optional.Why the two contracts stay consistent:
persistence_writer::register_writenow rejectsinvalid_row_idand throws rather than returning a token for it, on the grounds that such a token could not locate the product on read. The sentinel therefore never reaches atoken— it lives entirely in the storage layer, andtokenis identified by construction. There is no second representation to reconcile.On
has_id()and the placement-only constructor: these now have no callers outside tests. The only production use oftoken::id()is on the read path instorage_reader.cpp, and it does not guard onhas_id(). Removing them is a small cleanup to fold into the next PR that touches token, not part of this decision. (The members were also renamed fromm_id/m_hasIdtoid_/has_id_by #831.)No implementation is required for this issue, so I'm closing it.