fix(clp-s): Fix authoritative timestamp parsing bug introduced in #731.#769
Conversation
WalkthroughThis pull request modifies the conditional logic in the Changes
Sequence Diagram(s)sequenceDiagram
participant C as Caller
participant AW as ArchiveWriter::add_node
C->>AW: call add_node(node details)
AW->>AW: Check if m_authoritative_timestamp is not empty and parent_node_id equals cRootNodeId
alt Root node condition met
AW->>AW: Process node logic for root condition
else Not a root node
AW->>AW: Verify m_authoritative_timestamp.size() > (m_matched_timestamp_prefix_length + 1)
alt Size check passes
AW->>AW: Access element at m_matched_timestamp_prefix_length and process further
else Size check fails
AW->>AW: Handle condition without accessing the element
end
end
Possibly related PRs
Suggested reviewers
Tip ⚡🧪 Multi-step agentic review comment chat (experimental)
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🧰 Additional context used📓 Path-based instructions (1)`**/*.{cpp,hpp,java,js,jsx,tpp,ts,tsx}`: - Prefer `false == ...
⏰ Context from checks skipped due to timeout of 90000ms (10)
🔇 Additional comments (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
wraymo
left a comment
There was a problem hiding this comment.
Can you show an example what this PR fixes?
All cases -- the bug prevents timestamp parsing from ever being successful. Consider a simple case of timestamp Adding the namespace to the start of the timestamp token vector can also solve the bug/simplify the code overall, but it requires more code change so I've gone with the change in this PR for now. |
Description
This PR fixes a bug introduced during some refactoring in #731. In particular, that PR adds the condition
m_authoritative_timestamp.size() > (m_matched_timestamp_prefix_length + 1)to an if statement wrapping the rest of the timestamp key resolution logic, which prevented being able to correctly match the root of a given object subtree for keys consisting of a single token.The fix simply involves removing this condition from that outermost if branch and adjusting some other conditions accordingly.
Alternatively we could simplify the matching logic by inserting the namespace into the start of the vector of tokens and use that to remove the special case for matching the root of an object subtree.
Checklist
breaking change.
Validation performed
Summary by CodeRabbit