ffi: Fix Serializer to invalidate msgpack maps with non-string keys and msgpack arrays with unsupported types.
#570
Conversation
WalkthroughThe pull request introduces enhancements to the serialization logic for MessagePack data structures in the Changes
Possibly related PRs
Suggested reviewers
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 (
|
| if (msgpack::type::STR != key.type) { | ||
| // A map containing non-string keys is not serializable | ||
| return false; | ||
| } |
There was a problem hiding this comment.
This function is split from the previous serialize_msgpack_map to resolve clang-tidy cognitive complexity issue. The only major change is this if-statement.
Some other changes:
- Using
TransactionManagerto handle failure recovery - Rewrite the code in a way that it has less
elseblock
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (1)
components/core/src/clp/ffi/ir_stream/Serializer.hpp (1)
119-126: Enhance method documentation with more details.The documentation for
serialize_msgpack_map_using_dfsshould be expanded to include:
- The DFS traversal strategy and its benefits
- Error handling behaviour
- Validation rules for map keys
- Any transaction management details
Here's a suggested documentation improvement:
/** * Serializes the given msgpack map using depth-first search (DFS). + * + * Uses a depth-first search approach to traverse the msgpack map structure, which: + * - Validates that all keys are strings + * - Handles nested maps and arrays + * - Supports transaction-based error handling + * * @param msgpack_map The MessagePack map to serialize + * @throws May throw if memory allocation fails during serialization * @return Whether the serialization succeeded. */
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
components/core/src/clp/ffi/ir_stream/Serializer.cpp(6 hunks)components/core/src/clp/ffi/ir_stream/Serializer.hpp(1 hunks)components/core/tests/test-ir_encoding_methods.cpp(4 hunks)
🧰 Additional context used
🪛 cppcheck
components/core/src/clp/ffi/ir_stream/Serializer.cpp
[error] 246-246: Syntax Error
(internalAstError)
🔇 Additional comments (8)
components/core/src/clp/ffi/ir_stream/Serializer.hpp (1)
119-126: Consider architectural implications of DFS implementation.
The DFS approach for map serialization suggests a potentially deep recursion stack. Consider:
- Adding a maximum depth limit to prevent stack overflow
- Documenting memory usage characteristics
- Adding stack depth tracking for debugging
Let's check if there are any existing depth limits in the codebase:
components/core/tests/test-ir_encoding_methods.cpp (1)
214-226: Declaration of unpack_and_assert_serialization_failure is correct
The function declaration is clear, well-documented, and follows proper template syntax.
components/core/src/clp/ffi/ir_stream/Serializer.cpp (6)
19-19: Include TransactionManager.hpp is appropriate
The addition of #include "../../TransactionManager.hpp" is necessary for transaction management in the new serialization logic.
149-157: Well-documented declaration of is_msgpack_array_serializable
The function is_msgpack_array_serializable is clearly declared with comprehensive documentation, enhancing code readability and maintainability.
239-241: Proper validation added for MessagePack arrays in serialize_value_array
The check if (false == is_msgpack_array_serializable(val)) ensures that only serializable arrays are processed, preventing potential runtime errors with unsupported types.
345-347: Refactored map serialization logic enhances modularity
The refactoring to use serialize_msgpack_map_using_dfs improves code organisation and modularity, making the serialization process more maintainable.
469-554: Effective implementation of DFS in serialize_msgpack_map_using_dfs
The depth-first search approach for serializing MessagePack maps is well-implemented, with appropriate transaction management via TransactionManager. The checks for non-string keys and unsupported types enhance the robustness of the serialization process.
595-600: Correct addition of explicit template instantiations
The explicit template instantiations for serialize_msgpack_map_using_dfs ensure that the method is properly instantiated for both eight_byte_encoded_variable_t and four_byte_encoded_variable_t, avoiding potential linker errors.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
components/core/tests/test-ir_encoding_methods.cpp (2)
214-225: Consider enhancing function documentation.The documentation could be more specific about the expected behavior and return value conditions.
Add more details to the documentation:
/** * Unpacks and asserts the serialization of the msgpack bytes fails. * @tparam encoded_variable_t * @param buffer A buffer containing msgpack byte sequence. * @param serializer - * @return Whether serialization failed with the underlying IR buffer being empty. + * @return true if: + * 1. The input is a valid msgpack map + * 2. Serialization fails as expected + * 3. The IR buffer is empty after the failure + * false otherwise */
1348-1382: Comprehensive test coverage for invalid msgpack scenarios.The test cases effectively cover:
- Maps with non-string keys
- Nested invalid maps
- Arrays with invalid types
- Nested arrays with invalid elements
Consider adding test cases for:
- Empty maps with non-string keys
- Maps with mixed valid and invalid keys
Would you like me to help generate additional test cases for these scenarios?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
components/core/tests/test-ir_encoding_methods.cpp(4 hunks)
🧰 Additional context used
📓 Learnings (1)
components/core/tests/test-ir_encoding_methods.cpp (1)
Learnt from: LinZhihao-723
PR: y-scope/clp#570
File: components/core/tests/test-ir_encoding_methods.cpp:376-399
Timestamp: 2024-11-01T03:26:26.386Z
Learning: In the test code (`components/core/tests/test-ir_encoding_methods.cpp`), exception handling for `msgpack::unpack` can be omitted because the Catch2 testing framework captures exceptions if they occur.
🔇 Additional comments (2)
components/core/tests/test-ir_encoding_methods.cpp (2)
376-399: Implementation looks good!
The function correctly validates serialization failures and buffer state.
1328-1347: Well-structured test setup with reusable validation.
The test case is well-organized with a reusable lambda for common validation logic.
| auto const& as_array{curr->via.array}; | ||
| for (auto const& obj : span{as_array.ptr, as_array.size}) { | ||
| switch (obj.type) { | ||
| case msgpack::type::BIN: |
There was a problem hiding this comment.
Not necessarily but can we treat it as an Array<number>?
Description
msgpack allows users to use non-string types for keys in a map (key-value pairs). However, in our key-value pair IR format, we do not allow non-string key types. Before this PR, we don't have checks in our serializer but assuming the inputs keys are always strings. This PR adds validations to the key types to validate only serializable msgpack objects.
Similarly, before this PR, we need to validate whether the msgpack array contains unsupported types such as binary objects. This PR adds checks to invalidate arrays with unsupported types, or any submap with non-string keys. This ensures the serialized unstructured array can always be serialized as a JSON string. Ideally, the array check can be added to PR #465
Validation performed
Summary by CodeRabbit
New Features
Bug Fixes
Tests