[ISSUE #1474]⚡Optimize derive RequestHeaderCodec and add required attribute🚀 #1475
Conversation
WalkthroughThe changes introduced in this pull request enhance the Changes
Assessment against linked issues
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
Documentation and Community
|
|
🔊@mxsm 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
rocketmq-rust-bot
added
auto merge
ready to review
waiting-review
rocketmq-rust-robot
added
enhancement⚡️
![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
🧹 Outside diff range and nitpick comments (1)
rocketmq-macros/src/request_header_custom.rs (1)
132-196: Refactor to reduce code duplication in 'FromMap' implementationThe handling of
requiredfields introduces repetitive code blocks for different data types and conditions. Consider refactoring to abstract common logic into helper functions or use macros to eliminate duplication, which will improve maintainability and readability.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (2)
rocketmq-macros/src/lib.rs(1 hunks)rocketmq-macros/src/request_header_custom.rs(2 hunks)
🔇 Additional comments (3)
rocketmq-macros/src/lib.rs (1)
29-29: Correctly added 'required' attribute to 'RequestHeaderCodec' macro
The addition allows the macro to accept the required attribute on struct fields, enabling enhanced control over serialization and deserialization of required fields.
rocketmq-macros/src/request_header_custom.rs (2)
50-59: Properly implemented detection of 'required' attribute on struct fields
The code correctly checks for the presence of the required attribute and sets the required flag accordingly.
132-196: Ensure thorough testing of 'required' attribute handling
Verify that the macro correctly enforces required fields during deserialization and that appropriate errors are raised when required fields are missing or invalid.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1475 +/- ##
==========================================
- Coverage 22.32% 22.31% -0.01%
==========================================
Files 450 450
Lines 58248 58289 +41
==========================================
+ Hits 13003 13007 +4
- Misses 45245 45282 +37 ☔ View full report in Codecov by Sentry. |
|
🔊@mxsm 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
rocketmq-macros/src/request_header_custom.rs (2)
129-162: Reduce code duplication in Option type deserializationThe error handling logic is duplicated between String and non-String types. Consider extracting the common error handling into a helper macro or function.
Example approach:
macro_rules! handle_required_field { ($map:expr, $field:expr, $parser:expr) => { if required { Some($map.get(&cheetah_string::CheetahString::from_static_str(Self::#static_name)) .ok_or(Self::Error::RemotingCommandError( format!("Missing {} field", Self::#static_name), ))? .and_then($parser) .ok_or(Self::Error::RemotingCommandError( format!("Parse {} field error", Self::#static_name) ))?) } else { $map.get(&cheetah_string::CheetahString::from_static_str(Self::#static_name)) .and_then($parser) } }; }🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 133-141: rocketmq-macros/src/request_header_custom.rs#L133-L141
Added lines #L133 - L141 were not covered by tests
[warning] 148-155: rocketmq-macros/src/request_header_custom.rs#L148-L155
Added lines #L148 - L155 were not covered by tests
Line range hint
1-227: Improve documentation and error handlingWhile the implementation is functionally correct, consider these improvements:
- Add documentation explaining the
requiredattribute usage and behavior- Consider including the field type in error messages to aid debugging
- Add examples in doc comments showing common use cases
Example documentation:
/// Derives RequestHeaderCodec for a struct /// /// # Attributes /// - `required`: Mark a field as required. Missing or unparseable required fields /// will result in an error during deserialization. /// /// # Example /// ``` /// #[derive(RequestHeaderCodec)] /// struct MyHeader { /// #[required] /// id: String, /// optional_field: Option<i32>, /// } /// ```🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 53-57: rocketmq-macros/src/request_header_custom.rs#L53-L57
Added lines #L53 - L57 were not covered by tests
[warning] 85-85: rocketmq-macros/src/request_header_custom.rs#L85
Added line #L85 was not covered by tests
[warning] 113-113: rocketmq-macros/src/request_header_custom.rs#L113
Added line #L113 was not covered by tests
[warning] 133-141: rocketmq-macros/src/request_header_custom.rs#L133-L141
Added lines #L133 - L141 were not covered by tests
[warning] 148-155: rocketmq-macros/src/request_header_custom.rs#L148-L155
Added lines #L148 - L155 were not covered by tests
[warning] 167-172: rocketmq-macros/src/request_header_custom.rs#L167-L172
Added lines #L167 - L172 were not covered by tests
[warning] 180-186: rocketmq-macros/src/request_header_custom.rs#L180-L186
Added lines #L180 - L186 were not covered by tests
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
rocketmq-macros/src/request_header_custom.rs(1 hunks)
🧰 Additional context used
🪛 GitHub Check: codecov/patch
rocketmq-macros/src/request_header_custom.rs
[warning] 53-57: rocketmq-macros/src/request_header_custom.rs#L53-L57
Added lines #L53 - L57 were not covered by tests
[warning] 85-85: rocketmq-macros/src/request_header_custom.rs#L85
Added line #L85 was not covered by tests
[warning] 113-113: rocketmq-macros/src/request_header_custom.rs#L113
Added line #L113 was not covered by tests
[warning] 133-141: rocketmq-macros/src/request_header_custom.rs#L133-L141
Added lines #L133 - L141 were not covered by tests
[warning] 148-155: rocketmq-macros/src/request_header_custom.rs#L148-L155
Added lines #L148 - L155 were not covered by tests
[warning] 167-172: rocketmq-macros/src/request_header_custom.rs#L167-L172
Added lines #L167 - L172 were not covered by tests
[warning] 180-186: rocketmq-macros/src/request_header_custom.rs#L180-L186
Added lines #L180 - L186 were not covered by tests
🔇 Additional comments (1)
rocketmq-macros/src/request_header_custom.rs (1)
163-192: Add test coverage for all deserialization paths
The deserialization logic for non-Option types needs test coverage for:
- Required fields with valid/invalid/missing values
- Non-required fields with default values
- Different field types (String, CheetahString, numeric types)
Consider adding tests for:
- Required fields that are missing
- Required fields with invalid values
- Non-required fields defaulting correctly
- String vs numeric type parsing
Would you like me to help generate comprehensive test cases?
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 167-172: rocketmq-macros/src/request_header_custom.rs#L167-L172
Added lines #L167 - L172 were not covered by tests
[warning] 180-186: rocketmq-macros/src/request_header_custom.rs#L180-L186
Added lines #L180 - L186 were not covered by tests
| if let Some(value) = has_option { | ||
| let type_name = get_type_name(value); | ||
| if type_name == "CheetahString" { | ||
| quote! { | ||
| if let Some(ref value) = self.#field_name { | ||
| map.insert ( | ||
| cheetah_string::CheetahString::from_static_str(Self::#static_name), | ||
| value.clone() | ||
| ); | ||
| } | ||
| } | ||
| } else if type_name == "String" { | ||
| quote! { | ||
| } else if type_name == "String" { | ||
| quote! { | ||
| if let Some(ref value) = self.#field_name { | ||
| map.insert ( | ||
| cheetah_string::CheetahString::from_static_str(Self::#static_name), | ||
| cheetah_string::CheetahString::from_string(value.clone()) | ||
| ); | ||
| } | ||
| } | ||
| } else { | ||
| quote! { | ||
| } else { | ||
| quote! { | ||
| if let Some(value) = self.#field_name { | ||
| map.insert ( | ||
| cheetah_string::CheetahString::from_static_str(Self::#static_name), | ||
| cheetah_string::CheetahString::from_string(value.to_string()) | ||
| ); | ||
| } | ||
| } | ||
| } | ||
| } else { | ||
| let type_name = get_type_name(&field.ty); | ||
| if type_name == "CheetahString" { | ||
| quote! { | ||
| } | ||
| } else { | ||
| let type_name = get_type_name(&field.ty); | ||
| if type_name == "CheetahString" { | ||
| quote! { | ||
| map.insert ( | ||
| cheetah_string::CheetahString::from_static_str(Self::#static_name), | ||
| self.#field_name.clone() | ||
| ); | ||
| } | ||
| } else if type_name == "String" { | ||
| quote! { | ||
| } else if type_name == "String" { | ||
| quote! { | ||
| map.insert ( | ||
| cheetah_string::CheetahString::from_static_str(Self::#static_name), | ||
| cheetah_string::CheetahString::from_string(self.#field_name.clone()) | ||
| ); | ||
| } | ||
| } else { | ||
| quote! { | ||
| } else { | ||
| quote! { | ||
| map.insert ( | ||
| cheetah_string::CheetahString::from_static_str(Self::#static_name), | ||
| cheetah_string::CheetahString::from_string(self.#field_name.to_string()) | ||
| ); | ||
| } | ||
| } | ||
| }, | ||
| // build FromMap impl | ||
| if let Some(value) = has_option { | ||
| let type_name = get_type_name(value); | ||
| if type_name == "CheetahString" || type_name == "String" { | ||
| quote! { | ||
| } | ||
| }, |
There was a problem hiding this comment.
Add validation for required fields during serialization
The serialization logic doesn't enforce the required attribute constraints. For Option types, if a field is marked as required and contains None, it should return an error instead of silently skipping the field.
Consider adding validation like this:
if let Some(ref value) = self.#field_name {
map.insert(/*...*/);
+} else if required {
+ return None; // or consider returning Result instead of Option
}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 85-85: rocketmq-macros/src/request_header_custom.rs#L85
Added line #L85 was not covered by tests
[warning] 113-113: rocketmq-macros/src/request_header_custom.rs#L113
Added line #L113 was not covered by tests
| .iter() | ||
| .map(|field| { | ||
| let field_name = field.ident.as_ref().unwrap(); | ||
| let mut required = false; | ||
|
|
||
| for attr in &field.attrs { | ||
| if let Some(ident) = attr.path().get_ident() { | ||
| if ident == "required" { | ||
| required = true; | ||
| } | ||
| } | ||
| } | ||
|
|
||
| //Determining whether it is an Option type or a direct data type | ||
| //This will lead to different ways of processing in the future. | ||
| let has_option = is_option_type(&field.ty); |
There was a problem hiding this comment.
💡 Codebase verification
Missing test coverage for required attribute processing
The codebase lacks test coverage for the new required attribute processing in the RequestHeaderCodec derive macro. This could lead to potential issues with:
- Validation of required fields during serialization/deserialization
- Edge cases in attribute parsing
- Interaction with Option types
Please add test cases to cover:
- Struct fields with
requiredattribute - Fields without the attribute
- Required fields with Option types
- Required fields with non-Option types
Note: The verification revealed no existing tests in the rocketmq-macros crate, making this a critical gap in test coverage for the new functionality.
🔗 Analysis chain
Add test coverage for required attribute processing
The new attribute processing logic needs test coverage to ensure correct behavior with:
- Fields marked with
required - Fields without the attribute
- Invalid attribute usage scenarios
Would you like me to help generate comprehensive test cases for the attribute processing?
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check existing test coverage for attribute processing
fd -e rs . tests/ -x rg -l "required"
Length of output: 131
Script:
#!/bin/bash
# First, let's find all test files in the repository
fd -e rs . -x rg -l "required"
# Then, let's check the macro usage in tests
rg "#\[derive\(.*RequestHeaderCodec.*\)\]" -A 10
# Finally, let's check if there are any test modules or integration tests
rg "mod tests" -A 10
Length of output: 66341
Script:
#!/bin/bash
# Let's check for test files in the macros crate specifically
fd -e rs . rocketmq-macros/tests/
# Let's also check for any test modules within the macro implementation file
rg "^mod tests" rocketmq-macros/src/request_header_custom.rs -A 10
# And check for any test attributes in the macro crate
rg "#\[test\]" rocketmq-macros/src/
Length of output: 252
🧰 Tools
🪛 GitHub Check: codecov/patch
[warning] 53-57: rocketmq-macros/src/request_header_custom.rs#L53-L57
Added lines #L53 - L57 were not covered by tests
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1474
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
New Features
requiredattribute for theRequestHeaderCodecmacro, enhancing its functionality.Bug Fixes