feat(ffi): Add type checking to only accept JSON objects as user-defined stream-level metadata for the new IR format. #691
Conversation
WalkthroughThis pull request introduces enhanced error handling for the Changes
Sequence DiagramsequenceDiagram
participant Client
participant Serializer
participant Metadata
Client->>Serializer: create(optional_user_defined_metadata)
Serializer->>Metadata: Validate metadata type
alt Metadata is JSON object
Serializer-->>Client: Serializer instance
else Metadata is not JSON object
Serializer-->>Client: Error (std::errc::protocol_not_supported)
end
Possibly related PRs
Suggested reviewers
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms (12)
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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
components/core/src/clp/ffi/ir_stream/Serializer.cpp (1)
553-555: Consider including diagnostic details on invalid JSON metadata
While returningstd::errc::protocol_not_supportedis correct, adding context about which field is invalid (or logging an explanation) can help developers quickly pinpoint issues.if (false == optional_user_defined_metadata.value().is_object()) { + // Optional: Provide helpful logging/diagnostics about invalid metadata + // e.g. LOG_WARNING("User-defined metadata is not a JSON object: " + // << optional_user_defined_metadata.value().dump()); return std::errc::protocol_not_supported; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
components/core/src/clp/ffi/ir_stream/Serializer.cpp(1 hunks)components/core/src/clp/ffi/ir_stream/Serializer.hpp(1 hunks)components/core/tests/test-ir_encoding_methods.cpp(1 hunks)
🧰 Additional context used
📓 Path-based instructions (3)
components/core/src/clp/ffi/ir_stream/Serializer.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/src/clp/ffi/ir_stream/Serializer.hpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
components/core/tests/test-ir_encoding_methods.cpp (1)
Pattern **/*.{cpp,hpp,java,js,jsx,ts,tsx}: - Prefer false == <expression> rather than !<expression>.
⏰ Context from checks skipped due to timeout of 90000ms (12)
- GitHub Check: ubuntu-jammy-static-linked-bins
- GitHub Check: centos-stream-9-static-linked-bins
- GitHub Check: centos-stream-9-dynamic-linked-bins
- GitHub Check: ubuntu-jammy-dynamic-linked-bins
- GitHub Check: ubuntu-focal-static-linked-bins
- GitHub Check: ubuntu-focal-dynamic-linked-bins
- GitHub Check: build-macos (macos-14, false)
- GitHub Check: build-macos (macos-14, true)
- GitHub Check: lint-check (ubuntu-latest)
- GitHub Check: build-macos (macos-13, false)
- GitHub Check: lint-check (macos-latest)
- GitHub Check: build-macos (macos-13, true)
🔇 Additional comments (2)
components/core/src/clp/ffi/ir_stream/Serializer.hpp (1)
44-48: Documentation clarifies object-only input for metadata
The docstring precisely documents thatoptional_user_defined_metadatamust be a JSON object. This ensures consistency between the intended design and the actual validation logic. Good job!components/core/tests/test-ir_encoding_methods.cpp (1)
1497-1516: Comprehensive coverage of invalid user-defined metadata
These tests thoroughly validate all major cases for invalid inputs (string, integer, float, boolean, null, array). This ensures that only JSON objects are permitted and is aligned with the PR objective. Great work!
Co-authored-by: kirkrodrigues <[email protected]>
Description
As introduced in #677, we added support for serializing/deserializing user-defined stream-level metadata. However, we didn't add any type checking in this implementation, meaning that user can give any valid
nlohmann::jsoninstances, including arrays and primitive type values.After an offline discussion with @kirkrodrigues, we decided to constrain the metadata type to only accept JSON objects (key-value pair dictionaries) to avoid undefined behaviors in our ffi libraries. This PR implements this new requirement.
Ideally, the way to implement this PR is to constrain the type of the input to be
nlohmann::json::object_t, so that other types of inputs can't be compiled. However, after testing I realized the compiler doesn't prevent an implicit conversion fromnlohmann::jsontonlohmann::json::object_t, meaning that you could still pass in anynlohmann::jsonobject to anlohmann::json::object_ttyped parameter, and the error will be raised happens during the runtime through a nlohmann::json exception. To prevent the use of exceptions, we add explicit type checking before serializing the user-defined metadata, and returnstd::errc::protocol_not_supportedwhen seeing a non-JSON-object input.Validation performed
Summary by CodeRabbit
Release Notes
New Features
Bug Fixes
Tests
The update improves the reliability of metadata processing by ensuring only valid JSON objects are accepted during serialization, preventing potential data integrity issues.