feat(ffi): Update IR stream protocol version handling in preparation for releasing the kv-pair IR stream format: #573
Conversation
WalkthroughThe pull request introduces significant updates to the Changes
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
| auto validate_protocol_version(std::string_view protocol_version) -> IRProtocolErrorCode { | ||
| // These versions are hardcoded to support the IR protocol version that predates the key-value | ||
| // pair IR format. | ||
| constexpr std::array<std::string_view, 3> cBackwardCompatibleVersions{ |
There was a problem hiding this comment.
Do we want to include any backward compatible beta versions here?
There was a problem hiding this comment.
For old IR format no.
For key-value IR format, we do support 0.1.0-beta.1. I added a util to strip the pre-release and only compare the version core.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (7)
components/core/src/clp/ffi/ir_stream/protocol_constants.hpp (1)
17-18: Consider enhancing version compatibility documentation.The comment about backward compatibility is helpful but could be more detailed. Consider adding documentation that explains:
- The version range that's supported (0.0.2 to 0.1.0)
- The differences between the unstructured and key-value pair IR formats
Here's a suggested enhancement:
-// This is used for IR stream format that predates the key-value pair IR format. +/** + * Versions 0.0.0 through 0.0.2 represent the unstructured IR format. + * Version 0.1.0 introduces the key-value pair IR format. + * Versions between 0.0.2 and 0.1.0 are unsupported as they were never formally released. + */components/core/src/clp/ir/LogEventDeserializer.cpp (1)
Line range hint
19-27: Consider adding detailed error logging.While the error handling is well-structured, adding detailed error logging would help with debugging in production environments. Consider logging the specific error conditions and the actual values that caused them.
Here's a suggested improvement:
if (ffi::ir_stream::IRErrorCode_Success != ir_error_code) { switch (ir_error_code) { case ffi::ir_stream::IRErrorCode_Incomplete_IR: + LOG_ERROR("Incomplete IR detected during deserialization"); return std::errc::result_out_of_range; case ffi::ir_stream::IRErrorCode_Corrupted_IR: default: + LOG_ERROR("Protocol error: IR corruption detected (error_code: {})", ir_error_code); return std::errc::protocol_error; } }components/core/src/clp/ffi/ir_stream/encoding_methods.cpp (1)
101-102: Well-structured approach to version management.The implementation demonstrates good architectural practices:
- Uses flexible JSON metadata structure
- Maintains clear separation between encoding methods
- Follows semantic versioning principles for the IR format protocol
Consider documenting the version compatibility matrix in the codebase to help future maintainers understand the version relationships.
components/core/src/clp/ffi/ir_stream/decoding_methods.hpp (2)
4-4: LGTM with minor naming suggestion!The conversion to
enum classwithuint8_tas the underlying type improves type safety and memory efficiency. The new states effectively support the version differentiation requirements.However, for naming consistency with other enum values, consider renaming
Backward_CompatibletoBackward_CompatibilityorBackwards_Compatible.enum class IRProtocolErrorCode : uint8_t { Supported, - Backward_Compatible, + Backwards_Compatible, Too_Old, Too_New, Invalid, };Also applies to: 24-30
24-30: Consider adding version constants and validation testsTo strengthen the version protocol implementation:
- Consider defining version constants (e.g.,
MIN_SUPPORTED_VERSION,CURRENT_VERSION) to centralize version management- Ensure comprehensive unit tests cover all version validation scenarios, particularly around the
0.1.0boundary- Document the version compatibility matrix in a central location for future reference
Also applies to: 208-209
components/core/src/clp/ffi/ir_stream/decoding_methods.cpp (2)
488-489: Unnecessarystatic_castinstd::regexinitialisation.The
static_cast<char const*>may be unnecessary ifcProtocol::Metadata::VersionRegexis already of typechar const*. Removing the cast can improve code clarity.Apply this diff to simplify the code:
- std::regex const protocol_version_regex{ - static_cast<char const*>(cProtocol::Metadata::VersionRegex) - }; + std::regex const protocol_version_regex{ cProtocol::Metadata::VersionRegex };
501-502: Redundantstatic_castinstd::string_viewinitialisation.Similarly, the
static_cast<char const*>may be redundant ifcProtocol::Metadata::VersionValueis alreadychar const*. Simplifying this can enhance readability.Apply this diff:
- std::string_view const current_build_protocol_version{ - static_cast<char const*>(cProtocol::Metadata::VersionValue) - }; + std::string_view const current_build_protocol_version{ cProtocol::Metadata::VersionValue };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
components/core/src/clp/ffi/ir_stream/Deserializer.hpp(1 hunks)components/core/src/clp/ffi/ir_stream/Serializer.cpp(1 hunks)components/core/src/clp/ffi/ir_stream/decoding_methods.cpp(2 hunks)components/core/src/clp/ffi/ir_stream/decoding_methods.hpp(3 hunks)components/core/src/clp/ffi/ir_stream/encoding_methods.cpp(1 hunks)components/core/src/clp/ffi/ir_stream/protocol_constants.hpp(1 hunks)components/core/src/clp/ir/LogEventDeserializer.cpp(1 hunks)components/core/tests/test-ir_encoding_methods.cpp(4 hunks)
✅ Files skipped from review due to trivial changes (1)
- components/core/src/clp/ffi/ir_stream/Serializer.cpp
🧰 Additional context used
📓 Learnings (1)
components/core/src/clp/ffi/ir_stream/Deserializer.hpp (2)
Learnt from: LinZhihao-723
PR: y-scope/clp#549
File: components/core/src/clp/ffi/ir_stream/Deserializer.hpp:209-210
Timestamp: 2024-10-08T15:52:50.753Z
Learning: Until #486 is merged with customized error code support, introducing a layer from `IRErrorCode` to `std::errc` is unnecessary.
Learnt from: LinZhihao-723
PR: y-scope/clp#549
File: components/core/src/clp/ffi/ir_stream/Deserializer.hpp:209-210
Timestamp: 2024-10-01T07:59:15.290Z
Learning: Until #486 is merged with customized error code support, introducing a layer from `IRErrorCode` to `std::errc` is unnecessary.
🔇 Additional comments (11)
components/core/src/clp/ffi/ir_stream/protocol_constants.hpp (1)
15-18: Version constants align with PR objectives.
The version constants correctly implement the versioning strategy outlined in the PR objectives:
VersionValueset to "0.1.0" for the key-value pair IR formatMinimumSupportedVersionValueset to "0.1.0" to invalidate unreleased versionsLatestBackwardCompatibleVersionValueset to "0.0.2" for unstructured IR format support
Let's verify the version constants are used consistently:
✅ Verification successful
Let me search for the version validation logic to verify the changes.
Version constants are correctly implemented and validated
The version constants align with the implementation:
validate_protocol_version()correctly handles backward compatibility for versions "0.0.1" and "0.0.2"- The new version "0.1.0" represents the key-value pair IR format
- Test cases verify proper validation of version strings
- The version validation logic ensures compatibility between old and new formats
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify version constant usage across the codebase
# Expected: No hardcoded versions that conflict with these constants
# Search for version strings that might conflict with our constants
rg -g '!*.{md,txt}' -A 3 '"0\.[0-9]+\.[0-9]+"'
# Search for version validation logic
ast-grep --pattern 'validate_protocol_version($_)'
Length of output: 85394
Script:
#!/bin/bash
# Search for version validation implementation
rg -A 10 "validate_protocol_version" components/core/src/clp/ffi/ir_stream/
# Search for version-related test cases
rg -A 10 "TEST_CASE.*version" components/core/tests/test-ir_encoding_methods.cpp
Length of output: 3910
components/core/src/clp/ir/LogEventDeserializer.cpp (1)
45-48: LGTM! Verify version handling across the codebase.
The change to use IRProtocolErrorCode::Backward_Compatible aligns well with the PR's objective of maintaining backward compatibility with older IR formats.
Let's verify the version handling implementation:
✅ Verification successful
The shell script results reveal comprehensive version handling across the codebase. Let me verify the specific version constants and validation logic with one more search:
Version handling implementation is correctly aligned with backward compatibility requirements
The verification confirms that:
- The change to use
Backward_Compatibleis properly implemented and aligns with the version handling strategy - Versions v0.0.0, 0.0.1, and 0.0.2 are explicitly defined as backward-compatible versions
- The
IRProtocolErrorCodeenum correctly differentiates betweenSupportedandBackward_Compatiblestates - Comprehensive test coverage exists for all version validation scenarios
- The implementation properly handles version validation across different components
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the protocol version validation implementation
# Expected: Find all version-related constants and validation logic
# Search for version-related constants
echo "Searching for version constants..."
rg -A 2 "0\.0\.[0-2]|0\.1\.0" --type cpp
# Search for version validation logic
echo "Searching for version validation implementations..."
ast-grep --pattern 'validate_protocol_version($_)'
Length of output: 6835
Script:
#!/bin/bash
# Search for the validate_protocol_version implementation
echo "Searching for protocol version validation implementation..."
rg -A 10 "validate_protocol_version" --type cpp
# Search for IRProtocolErrorCode enum
echo "Searching for IRProtocolErrorCode definition..."
rg -A 5 "enum.*IRProtocolErrorCode" --type cpp
Length of output: 16970
components/core/src/clp/ffi/ir_stream/encoding_methods.cpp (2)
101-102: Verify version handling in both encoding methods.
Since both eight-byte and four-byte encoding methods share the same metadata structure, ensure that the version change is handled consistently in both deserialization paths.
Let's verify the deserialization handling:
#!/bin/bash
# Description: Verify version handling in deserialization paths
# Expected: Find corresponding deserialization logic for both encoding methods
# Search for deserialization logic in both encoding methods
echo "Searching for eight-byte encoding deserialization..."
ast-grep --pattern 'namespace eight_byte_encoding {
$$$
deserialize$_($$$) {
$$$
}
$$$
}'
echo "Searching for four-byte encoding deserialization..."
ast-grep --pattern 'namespace four_byte_encoding {
$$$
deserialize$_($$$) {
$$$
}
$$$
}'101-102: The version update aligns with backward compatibility requirements.
The change to use LatestBackwardCompatibleVersionValue instead of VersionValue appropriately implements the version protocol update while maintaining backward compatibility with older IR formats.
Let's verify the version constants and their usage:
✅ Verification successful
Let me gather more specific information about the version constants and their usage.
Let me check the protocol constants header files to understand the version definitions.
Version update is correctly implemented for backward compatibility
The change to use LatestBackwardCompatibleVersionValue ("0.0.2") instead of VersionValue ("0.1.0") in the metadata is correct and properly maintains backward compatibility. The protocol constants header confirms that:
- Current version is "0.1.0"
- Minimum supported version is "0.1.0"
- Latest backward compatible version is "0.0.2"
The implementation properly handles the version transition while maintaining support for older IR formats (v0.0.0 through v0.0.2).
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify version constants and their usage across the codebase
# Expected: Find definitions of version constants and their usage in serialization/deserialization
# Search for version-related constants
echo "Searching for version constants..."
rg "VersionValue|LatestBackwardCompatibleVersionValue" --type cpp
# Search for version validation usage
echo "Searching for version validation..."
rg "validate_protocol_version" --type cpp
Length of output: 434
Script:
#!/bin/bash
# Search for version-related constants in protocol headers
echo "Searching for version constants in protocol headers..."
fd -e hpp -e h "protocol" --exec rg -l "Version|version"
# Search for actual version string definitions
echo "Searching for version string definitions..."
rg "\"0\\.\\d+\\.\\d+\"" --type cpp --type hpp
# Search for metadata version key usage
echo "Searching for metadata version key usage..."
rg "Metadata::VersionKey" --type cpp
Length of output: 1824
Script:
#!/bin/bash
# Check protocol constants headers content
echo "Content of CLP protocol constants header:"
cat ./components/core/src/clp/ffi/ir_stream/protocol_constants.hpp
echo -e "\nContent of GLT protocol constants header:"
cat ./components/core/src/glt/ffi/ir_stream/protocol_constants.hpp
# Check version validation implementations
echo -e "\nSearching for version validation implementations:"
rg -A 5 "validate_protocol_version" --type cpp
Length of output: 15912
components/core/src/clp/ffi/ir_stream/decoding_methods.hpp (2)
198-205: Documentation updates look great!
The documentation clearly describes all possible return values and effectively communicates the distinction between supported versions and backward-compatible formats. This aligns well with the PR's objective of formalizing the version protocol.
208-209: Excellent function signature improvements!
The changes enhance safety and efficiency:
[[nodiscard]]ensures version validation results aren't accidentally ignoredstring_viewparameter optimizes string handling- Trailing return type syntax improves readability
components/core/src/clp/ffi/ir_stream/Deserializer.hpp (1)
157-158: LGTM!
The updated protocol version validation enhances maintainability and correctness.
components/core/src/clp/ffi/ir_stream/decoding_methods.cpp (1)
3-5: Appropriate inclusion of necessary headers.
The addition of <array>, <regex>, and <string_view> is appropriate, as these headers are required for the usage of std::array, std::regex, and std::string_view in the updated code.
components/core/tests/test-ir_encoding_methods.cpp (3)
633-634: LGTM: Version validation test for backward-compatible versions is correct
The test case correctly verifies that validate_protocol_version(version) returns IRProtocolErrorCode::Backward_Compatible for backward-compatible versions.
954-955: LGTM: Version validation check for backward-compatible versions
The test correctly asserts that validate_protocol_version(version) returns IRProtocolErrorCode::Backward_Compatible.
1104-1104: LGTM: Correct use of VersionValue in expected metadata
The expected_metadata is properly populated with cProtocol::Metadata::VersionValue, ensuring the version key matches the protocol's expected value.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/core/src/clp/ffi/ir_stream/decoding_methods.cpp (2)
503-511: LGTM! Comprehensive semver core extraction.The implementation correctly handles all semver components according to the specification. Consider adding unit tests to verify edge cases like versions with both pre-release and build metadata (e.g., "1.0.0-alpha+001").
528-548: Consider adding detailed documentation for minimum version validation.While the logic is correct, the complexity of handling pre-release versions warrants additional documentation explaining the different cases and their rationale.
Add documentation above the minimum version check:
+ // Handle special cases for minimum supported versions: + // 1. If the version core is less than minimum supported, reject it + // 2. If version cores match but the given version has pre-release info: + // a. Reject if minimum supported is a formal release + // b. Compare full versions if minimum supported has pre-release info
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
components/core/src/clp/ffi/ir_stream/decoding_methods.cpp(2 hunks)components/core/src/clp/ffi/ir_stream/protocol_constants.hpp(2 hunks)components/core/tests/test-ir_encoding_methods.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- components/core/src/clp/ffi/ir_stream/protocol_constants.hpp
- components/core/tests/test-ir_encoding_methods.cpp
🔇 Additional comments (4)
components/core/src/clp/ffi/ir_stream/decoding_methods.cpp (4)
3-5: LGTM! Required includes added appropriately.
The new includes support the version validation functionality with standard library components.
476-480: LGTM! Well-structured backward compatibility support.
The constexpr array efficiently defines the supported legacy versions. The implementation aligns with the PR objectives to support unstructured IR format versions.
487-498: LGTM! Robust version validation implementation.
The regex validation with proper type safety and error handling ensures version string correctness.
522-525:
Fix potential version comparison issues.
The current string comparison of major versions could produce incorrect results when comparing versions with different digit counts (e.g., "2" vs "10"). Consider implementing numeric comparison.
Apply this diff:
- if (get_major_version(curr_build_protocol_version_core)
- > get_major_version(protocol_version_core))
+ if (std::stoi(get_major_version(curr_build_protocol_version_core))
+ > std::stoi(get_major_version(protocol_version_core)))Likely invalid or redundant comment.
Co-authored-by: kirkrodrigues <[email protected]>
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
components/core/src/clp/ffi/ir_stream/decoding_methods.cpp(2 hunks)components/core/src/clp/ffi/ir_stream/decoding_methods.hpp(3 hunks)components/core/src/clp/ffi/ir_stream/encoding_methods.cpp(1 hunks)components/core/src/clp/ffi/ir_stream/protocol_constants.hpp(2 hunks)components/core/tests/test-ir_encoding_methods.cpp(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- components/core/src/clp/ffi/ir_stream/decoding_methods.hpp
- components/core/src/clp/ffi/ir_stream/encoding_methods.cpp
- components/core/src/clp/ffi/ir_stream/protocol_constants.hpp
- components/core/tests/test-ir_encoding_methods.cpp
🔇 Additional comments (4)
components/core/src/clp/ffi/ir_stream/decoding_methods.cpp (4)
3-7: LGTM! Header includes are properly organized.
The new includes are necessary for the version validation functionality and follow the standard include ordering convention.
475-475: LGTM! Modern function declaration style.
The trailing return type syntax improves readability and consistency.
502-503: TODO comment is appropriate.
The TODO comment clearly indicates the temporary nature of the hardcoded versions and the planned improvement.
475-514: Add test cases for version comparison edge cases.
To ensure robust version validation, consider adding test cases for:
- Beta versions (e.g., "0.1.0-beta.1")
- Release candidates
- Version with different number of components
- Invalid version formats
There was a problem hiding this comment.
For the PR title & commit body, how about:
feat(ffi): Update IR stream protocol version handling in preparation for releasing the kv-pair IR stream format:
- Bump the IR stream protocol version to 0.1.0 for the kv-pair IR stream format.
- Treat the previous IR stream format's versions as backwards compatible.
- Differentiate between backwards-compatible and supported versions during validation.
LinZhihao-723
changed the title
* ffi: Add support for serializing/deserializing auto-generated and user-generated schema tree node IDs. (y-scope#557) Co-authored-by: kirkrodrigues <[email protected]> * clp: Add missing C++ standard library includes in IR parsing files. (y-scope#561) Co-authored-by: kirkrodrigues <[email protected]> * log-viewer-webui: Update `yscope-log-viewer` to the latest version (which uses `clp-ffi-js`). (y-scope#562) * package: Upgrade dependencies to resolve security issues. (y-scope#536) * clp-s: Implement table packing (y-scope#466) Co-authored-by: wraymo <[email protected]> Co-authored-by: Kirk Rodrigues <[email protected]> Co-authored-by: wraymo <[email protected]> * log-viewer-webui: Update `yscope-log-viewer` to the latest version. (y-scope#565) * ci: Switch GitHub macOS build workflow to use macos-13 (x86) and macos-14 (ARM) runners. (y-scope#566) * core: Add support for user-defined HTTP headers in `NetworkReader`. (y-scope#568) Co-authored-by: Lin Zhihao <[email protected]> Co-authored-by: Xiaochong Wei <[email protected]> * chore: Update to the latest version of yscope-dev-utils. (y-scope#574) * build(core): Upgrade msgpack to v7.0.0. (y-scope#575) * feat(ffi): Update IR stream protocol version handling in preparation for releasing the kv-pair IR stream format: (y-scope#573) - Bump the IR stream protocol version to 0.1.0 for the kv-pair IR stream format. - Treat the previous IR stream format's versions as backwards compatible. - Differentiate between backwards-compatible and supported versions during validation. Co-authored-by: kirkrodrigues <[email protected]> * fix(taskfiles): Trim trailing slash from URL prefix in `download-and-extract-tar` (fixes y-scope#577). (y-scope#578) * fix(ffi): Correct `clp::ffi::ir_stream::Deserializer::deserialize_next_ir_unit`'s return value when failing to read the next IR unit's type tag. (y-scope#579) * fix(taskfiles): Update `yscope-log-viewer` sources in `log-viewer-webui-clients` sources list (fixes y-scope#576). (y-scope#580) * fix(cmake): Add Homebrew path detection for `mariadb-connector-c` to fix macOS build failure. (y-scope#582) Co-authored-by: kirkrodrigues <[email protected]> * refactor(ffi): Make `get_schema_subtree_bitmap` a public method of `KeyValuePairLogEvent`. (y-scope#581) * ci: Schedule GitHub workflows to daily run to detect failures due to upgraded dependencies or environments. (y-scope#583) * docs: Update the required version of task. (y-scope#567) * Add pr check workflow --------- Co-authored-by: kirkrodrigues <[email protected]> Co-authored-by: Junhao Liao <[email protected]> Co-authored-by: Henry8192 <[email protected]> Co-authored-by: Devin Gibson <[email protected]> Co-authored-by: wraymo <[email protected]> Co-authored-by: wraymo <[email protected]> Co-authored-by: Xiaochong(Eddy) Wei <[email protected]> Co-authored-by: Xiaochong Wei <[email protected]> Co-authored-by: haiqi96 <[email protected]>
…for releasing the kv-pair IR stream format: (y-scope#573) - Bump the IR stream protocol version to 0.1.0 for the kv-pair IR stream format. - Treat the previous IR stream format's versions as backwards compatible. - Differentiate between backwards-compatible and supported versions during validation. Co-authored-by: kirkrodrigues <[email protected]>
Description
This PR formally updates the version protocol to mark released versions as
0.1.0for the key-value pair IR format.Also, to support backward compatibility and not break the existing old IR format serialization/deserialization method, we add support for version validation for IR version
v0.0.0,0.0.1and0.0.2, which is the unstructured IR format. It also updates the old serialization methods to stick to the latest compatible version (0.0.2) to differentiate with the key-value pair IR format.This PR also introduces a minimum supported version to invalidate any version larger than
0.0.2and smaller than0.1.0, since we don't want to support any compatibility to these versions we never formally released.However, after realizing the current version validation function is broken since it's doing purely string comprising, we replace the validation function with hardcoded values instead. The proper implementation should be deferred to future PRs.
Validation performed
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Bug Fixes
Tests