[ISSUE #1470]♻️FromMap trait from mehtod return type changed from Option<Self::Target> to Result<Self::Target,Self::Error>🚀 #1473
Conversation
…ion<Self::Target> to Result<Self::Target,Self::Error>🚀
|
🔊@mxsm 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
|
Caution Review failedThe pull request is closed. WalkthroughThe changes in this pull request primarily focus on enhancing error handling across multiple components of the RocketMQ codebase. The Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Rate limit exceeded@rocketmq-rust-bot has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 19 minutes and 18 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. 📜 Recent review detailsConfiguration used: .coderabbit.yaml 📒 Files selected for processing (47)
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
|
rocketmq-rust-bot
added
AI review first
rocketmq-rust-bot
added
approved
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 29
🧹 Outside diff range and nitpick comments (51)
rocketmq-remoting/src/protocol/header/get_all_topic_config_response_header.rs (1)
39-40: Consider adding documentation for the empty implementation.While the implementation is correct for this empty struct, it would be helpful to add a doc comment explaining why this header doesn't require any map parsing. This would make the intention clearer for future maintainers.
Consider adding documentation like this:
+ /// Creates a new instance from a property map. + /// This header doesn't require any properties, so it always returns an empty instance. fn from(_map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { Ok(Self {}) }rocketmq-remoting/src/rpc/rpc_request_header.rs (1)
Line range hint
69-79: Handle parsing errors explicitly when converting strings to booleansCurrently, parsing errors for
namespacedandonewayfields are silently ignored usingand_then(|s| s.parse::<bool>().ok()). Since thefrommethod now returns aResult, it's better to handle parsing errors explicitly and return appropriate errors.Apply this diff to handle parsing errors:
namespaced: map .get(&CheetahString::from_static_str( RpcRequestHeader::NAMESPACED, )) - .and_then(|s| s.parse::<bool>().ok()), + .map(|s| s.parse::<bool>()) + .transpose()?, broker_name: map .get(&CheetahString::from_static_str( RpcRequestHeader::BROKER_NAME, )) .cloned(), oneway: map .get(&CheetahString::from_static_str(RpcRequestHeader::ONEWAY)) - .and_then(|s| s.parse::<bool>().ok()), + .map(|s| s.parse::<bool>()) + .transpose()?,This change ensures that parsing errors are properly propagated rather than being silently ignored.
rocketmq-remoting/src/protocol/header/check_transaction_state_request_header.rs (1)
Line range hint
93-117: Consider propagating parsing errors instead of defaulting to default valuesIn the
frommethod, when parsing numerical values from the map, usingunwrap_or_default()can mask potential parsing errors and may result in silent failures or unexpected default values. It's better to propagate parsing errors using the?operator, allowing for more robust error handling.Apply this diff to improve error handling:
- tran_state_table_offset: map - .get(&CheetahString::from_static_str(Self::TRAN_STATE_TABLE_OFFSET)) - .and_then(|v| v.parse().ok()) - .unwrap_or_default(), + tran_state_table_offset: map + .get(&CheetahString::from_static_str(Self::TRAN_STATE_TABLE_OFFSET)) + .ok_or_else(|| crate::remoting_error::RemotingError::ValueNotFound(Self::TRAN_STATE_TABLE_OFFSET.to_string()))? + .parse::<i64>() + .map_err(|e| crate::remoting_error::RemotingError::ParseError(e.to_string()))?,Repeat similar changes for other numerical fields like
commit_log_offsetto ensure all parsing errors are properly handled.rocketmq-remoting/src/protocol/header/namesrv/query_data_version_header.rs (2)
Line range hint
84-97: Avoid usingunwrap(); handle missing or invalidbroker_idappropriatelyUsing
unwrap()on the optional parsing result ofbroker_idcan cause a panic if the key is missing or the value cannot be parsed. To improve robustness, consider returning an error whenbroker_idis missing or invalid.Apply this diff to handle errors appropriately:
- broker_id: map - .get(&CheetahString::from_static_str( - QueryDataVersionRequestHeader::BROKER_ID, - )) - .and_then(|s| s.parse::<u64>().ok()) - .unwrap(), + broker_id: map + .get(&CheetahString::from_static_str( + Self::BROKER_ID, + )) + .ok_or_else(|| crate::remoting_error::RemotingError::ValueNotFound(Self::BROKER_ID.to_string()))? + .parse::<u64>() + .map_err(|e| crate::remoting_error::RemotingError::ParseError(e.to_string()))?,Similarly, consider handling potential missing or invalid values for
broker_name,broker_addr, andcluster_nameby returning errors instead of defaulting to empty strings.
Line range hint
141-152: Ensure proper error handling for thechangedfieldIn the
frommethod ofQueryDataVersionResponseHeader, usingunwrap_or(false)can hide parsing errors and lead to unintended behavior. To enhance error visibility, propagate parsing errors to the caller.Apply this diff to handle parsing errors:
- changed: map - .get(&CheetahString::from_static_str( - QueryDataVersionResponseHeader::CHANGED, - )) - .and_then(|s| s.parse::<bool>().ok()) - .unwrap_or(false), + changed: map + .get(&CheetahString::from_static_str( + Self::CHANGED, + )) + .ok_or_else(|| crate::remoting_error::RemotingError::ValueNotFound(Self::CHANGED.to_string()))? + .parse::<bool>() + .map_err(|e| crate::remoting_error::RemotingError::ParseError(e.to_string()))?,This change ensures that any issues during parsing are reported back to the caller for appropriate handling.
rocketmq-remoting/src/protocol/header/broker/broker_heartbeat_request_header.rs (1)
Line range hint
89-110: Improve error handling by propagating parsing errorsIn the
frommethod, usingunwrap_or_default()or default values can mask missing or invalid inputs for required fields, potentially leading to silent failures or unintended behavior. To enhance robustness, consider returning errors when required fields are missing or parsing fails.Apply this diff to handle errors properly:
- cluster_name: map - .get(&CheetahString::from_static_str( - BrokerHeartbeatRequestHeader::CLUSTER_NAME, - )) - .cloned() - .unwrap_or_default(), + cluster_name: map + .get(&CheetahString::from_static_str( + Self::CLUSTER_NAME, + )) + .cloned() + .ok_or_else(|| crate::remoting_error::RemotingError::ValueNotFound(Self::CLUSTER_NAME.to_string()))?, - broker_addr: map - .get(&CheetahString::from_static_str( - BrokerHeartbeatRequestHeader::BROKER_ADDR, - )) - .cloned() - .unwrap_or_default(), + broker_addr: map + .get(&CheetahString::from_static_str( + Self::BROKER_ADDR, + )) + .cloned() + .ok_or_else(|| crate::remoting_error::RemotingError::ValueNotFound(Self::BROKER_ADDR.to_string()))?, - broker_name: map - .get(&CheetahString::from_static_str( - BrokerHeartbeatRequestHeader::BROKER_NAME, - )) - .cloned() - .unwrap_or_default(), + broker_name: map + .get(&CheetahString::from_static_str( + Self::BROKER_NAME, + )) + .cloned() + .ok_or_else(|| crate::remoting_error::RemotingError::ValueNotFound(Self::BROKER_NAME.to_string()))?,For numerical fields like
broker_id,epoch, and others, consider using similar error propagation to handle parsing failures.rocketmq-remoting/src/protocol/header/namesrv/topic_operation_header.rs (1)
204-209: Handle optional fields appropriately in 'from' implementationsIn the
frommethod forTopicRequestHeader, therpcfield, which is anOption, is initialized withSome(<RpcRequestHeader as FromMap>::from(map)?). This means it will return an error ifRpcRequestHeader::from(map)fails, potentially forcing the presence ofrpc.Consider handling the optional nature of the
rpcfield more gracefully:let rpc = match <RpcRequestHeader as FromMap>::from(map) { Ok(rpc_header) => Some(rpc_header), Err(_) => None, }; Ok(TopicRequestHeader { lo: map .get(&CheetahString::from_static_str(Self::LO)) .and_then(|s| s.parse::<bool>().ok()), rpc, })Alternatively, if
rpcis required, consider updating the struct to reflect this by removing theOptionwrapper.rocketmq-remoting/src/protocol/header/namesrv/kv_config_header.rs (4)
90-92: Correct grammatical errors in error messagesThe error messages use "Miss" instead of "Missing". For clarity, please update the messages.
Apply this diff to correct the error messages:
-ok_or(Self::Error::RemotingCommandError( - "Miss namespace field".to_string(), +ok_or(Self::Error::RemotingCommandError( + "Missing 'namespace' field".to_string(),Similarly, update the messages for the
keyandvaluefields:-"Miss key field" +"Missing 'key' field" -"Miss value field" +"Missing 'value' field"Also applies to: 97-100, 105-108
159-160: Correct grammatical errors in error messagesThe error messages in
GetKVConfigRequestHeaderuse "Miss" instead of "Missing". Please update them for accuracy.[duplicate_comment from lines 90-92]
Update the error messages as previously described.
Also applies to: 165-168
260-261: Correct grammatical errors in error messagesIn
DeleteKVConfigRequestHeader, the error messages should use "Missing" instead of "Miss".[duplicate_comment from lines 90-92]
Ensure that all error messages are grammatically correct for better clarity.
Also applies to: 268-269
311-312: Correct grammatical errors in error messagesThe error message in
GetKVListByNamespaceRequestHeadershould be updated from "Miss namespace field" to "Missing 'namespace' field".[duplicate_comment from lines 90-92]
Please apply the correction to maintain consistency across all error messages.
rocketmq-remoting/src/protocol/header/namesrv/register_broker_header.rs (1)
287-288: Implement proper error handling forRegisterBrokerResponseHeaderEven though
ha_server_addrandmaster_addrare optional, consider handling cases where they might have invalid data. Provide meaningful error messages if parsing fails.rocketmq-broker/src/out_api/broker_outer_api.rs (1)
Line range hint
277-281: Handle errors fromregister_broker_resultto prevent silent failuresCurrently, if
register_broker_resultis anErr, the error is silently ignored, which can lead to unnoticed failures. Consider handling theErrvariant to log the error and take appropriate action.Apply this diff to handle the error case:
if let Ok(header) = register_broker_result { result.ha_server_addr = header .ha_server_addr .clone() .unwrap_or(CheetahString::empty()); result.master_addr = header.master_addr.clone().unwrap_or(CheetahString::empty()); + } else { + error!( + "Failed to decode RegisterBrokerResponseHeader: {:?}", + register_broker_result.unwrap_err() + ); + // You might want to handle the error further or return it }rocketmq-broker/src/processor/send_message_processor.rs (1)
1005-1007: Implement theconsumer_send_msg_backmethod.The
consumer_send_msg_backfunction currently contains atodo!()macro, indicating it is not yet implemented. Providing an implementation will enable proper handling of consumer message callbacks.Would you like assistance in implementing the
consumer_send_msg_backmethod? I can help draft the code based on the expected functionality.rocketmq-remoting/src/protocol/header/query_topics_by_consumer_request_header.rs (1)
74-74: Consider handling missingrpc_request_headergracefullyWhile wrapping
RpcRequestHeader::from(map)?withSome(...)is appropriate, if there is a possibility that therpc_request_headermight be absent, consider handling theNonecase explicitly to prevent potentialNoneError.Apply this diff to handle the absence of
rpc_request_header:rpc_request_header: Some(<RpcRequestHeader as FromMap>::from(map)?), + // If RpcRequestHeader::from(map) returns an error due to missing keys, handle it accordinglyrocketmq-remoting/src/protocol/header/update_consumer_offset_header.rs (1)
Line range hint
87-111: Enhance error handling consistency across all field conversionsWhile the change to Result is good, the implementation could better utilize error handling:
- Required fields (consumer_group, topic) use unwrap_or_default() which silently handles missing fields
- Numeric parsing (queue_id, commit_offset) silently ignores parsing errors with ok()
- Only topic_request_header propagates errors with ?
Consider this more robust implementation:
fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { - Ok(UpdateConsumerOffsetRequestHeader { - consumer_group: map - .get(&CheetahString::from_static_str( - UpdateConsumerOffsetRequestHeader::CONSUMER_GROUP, - )) - .cloned() - .unwrap_or_default(), + let consumer_group = map + .get(&CheetahString::from_static_str( + UpdateConsumerOffsetRequestHeader::CONSUMER_GROUP, + )) + .cloned() + .ok_or(RemotingError::MissingField("consumer_group"))?; + + let topic = map + .get(&CheetahString::from_static_str( + UpdateConsumerOffsetRequestHeader::TOPIC, + )) + .cloned() + .ok_or(RemotingError::MissingField("topic"))?; + + let queue_id = if let Some(v) = map + .get(&CheetahString::from_static_str( + UpdateConsumerOffsetRequestHeader::QUEUE_ID, + )) { + Some(v.parse().map_err(|_| RemotingError::InvalidField("queue_id"))?) + } else { + None + }; + + let commit_offset = if let Some(v) = map + .get(&CheetahString::from_static_str( + UpdateConsumerOffsetRequestHeader::COMMIT_OFFSET, + )) { + Some(v.parse().map_err(|_| RemotingError::InvalidField("commit_offset"))?) + } else { + None + }; + + Ok(UpdateConsumerOffsetRequestHeader { + consumer_group, + topic, + queue_id, + commit_offset, topic_request_header: Some(<TopicRequestHeader as FromMap>::from(map)?), })This implementation:
- Properly validates required fields
- Provides specific error messages for missing or invalid fields
- Maintains consistent error handling across all field conversions
rocketmq-remoting/src/protocol/header/get_min_offset_request_header.rs (1)
Line range hint
69-83: Critical: Remove unsafe unwrap() calls in parse operationsThe current implementation has multiple
unwrap()calls that could panic at runtime. This defeats the purpose of moving to Result-based error handling.- queue_id: map - .get(&CheetahString::from_static_str( - GetMinOffsetRequestHeader::QUEUE_ID, - )) - .map(|s| s.parse().unwrap()) - .unwrap_or_default(), + queue_id: map + .get(&CheetahString::from_static_str( + GetMinOffsetRequestHeader::QUEUE_ID, + )) + .map(|s| s.parse()) + .transpose() + .map_err(|e| RemotingError::ParseError(format!("Failed to parse queue_id: {}", e)))? + .unwrap_or_default(),rocketmq-remoting/src/protocol/header/get_max_offset_request_header.rs (2)
Line range hint
87-107: Critical: Remove unsafe unwrap() calls in parse operationsThe implementation contains multiple
unwrap()calls that could panic at runtime, which contradicts the PR's goal of improving error handling.- queue_id: map - .get(&CheetahString::from_static_str( - GetMaxOffsetRequestHeader::QUEUE_ID, - )) - .map(|s| s.parse().unwrap()) - .unwrap_or_default(), - committed: map - .get(&CheetahString::from_static_str( - GetMaxOffsetRequestHeader::COMMITTED, - )) - .map(|s| s.parse().unwrap()) - .unwrap_or(true), + queue_id: map + .get(&CheetahString::from_static_str( + GetMaxOffsetRequestHeader::QUEUE_ID, + )) + .map(|s| s.parse()) + .transpose() + .map_err(|e| RemotingError::ParseError(format!("Failed to parse queue_id: {}", e)))? + .unwrap_or_default(), + committed: map + .get(&CheetahString::from_static_str( + GetMaxOffsetRequestHeader::COMMITTED, + )) + .map(|s| s.parse()) + .transpose() + .map_err(|e| RemotingError::ParseError(format!("Failed to parse committed: {}", e)))? + .unwrap_or(true),
Line range hint
112-207: Consider refactoring TopicRequestHeaderTrait implementation to avoid unwrap()The trait implementation makes extensive use of
unwrap()which could lead to runtime panics. Consider refactoring to useok_or_else()or similar error handling patterns.Example refactor for one method:
- fn broker_name(&self) -> Option<&CheetahString> { - self.topic_request_header - .as_ref() - .unwrap() - .rpc_request_header - .as_ref() - .unwrap() - .broker_name - .as_ref() - } + fn broker_name(&self) -> Option<&CheetahString> { + self.topic_request_header + .as_ref() + .and_then(|h| h.rpc_request_header.as_ref()) + .and_then(|h| h.broker_name.as_ref()) + }rocketmq-remoting/src/protocol/header/namesrv/brokerid_change_request_header.rs (2)
Line range hint
124-131: Remove duplicate ha_broker_addr handlingThe
ha_broker_addrfield is being processed twice in theto_mapmethod. The second block is redundant and should be removed.- if let Some(ref ha_broker_addr) = self.ha_broker_addr { - map.insert( - CheetahString::from_static_str( - NotifyMinBrokerIdChangeRequestHeader::HA_BROKER_ADDR, - ), - ha_broker_addr.clone(), - ); - }
Line range hint
85-87: Implement check_fields methodThe
check_fieldsmethod is currently unimplemented. Given this PR's focus on improved error handling, it's crucial to implement proper field validation to ensure data integrity.Consider implementing validation for:
- Required fields presence
- Broker ID format validation
- Address format validation
Would you like assistance in implementing this method?
rocketmq-remoting/src/protocol/header/unlock_batch_mq_request_header.rs (1)
51-55: LGTM! Consider documenting error conditions.The implementation correctly handles error propagation and aligns with Rust's error handling patterns.
Consider adding documentation to describe possible error conditions:
+ /// Converts a HashMap into UnlockBatchMqRequestHeader + /// + /// # Errors + /// + /// Returns a RemotingError if: + /// - Required RPC header fields are missing from the map + /// - Field values cannot be properly parsed fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> {rocketmq-remoting/src/protocol/header/lock_batch_mq_request_header.rs (1)
51-55: LGTM! Consider adding documentationThe implementation correctly handles error propagation using the
?operator and aligns with the PR's objective of transitioning to Result-based error handling.Consider adding a comment explaining why
rpc_request_headeris wrapped inSomewhen it's already validated through theFromMapconversion.rocketmq-remoting/src/protocol/header/get_consumer_listby_group_request_header.rs (1)
Line range hint
77-89: Add unit tests for error casesThe current unit tests only cover the successful scenario where
consumer_groupis present. With the change to return aResult, it's important to add tests for error cases, such as whenconsumer_groupis missing from the map, to ensure proper error handling.Would you like assistance in writing the unit tests for these error cases?
rocketmq-remoting/src/protocol/header/get_topic_stats_request_header.rs (1)
Line range hint
38-52: Consider updating CommandCustomHeader trait for consistencyWhile we're improving error handling in
FromMap::from(), theCommandCustomHeader::to_map()method still returnsOption. Consider updating both traits to useResultfor consistency in error handling across the codebase.rocketmq-remoting/src/protocol/header/heartbeat_request_header.rs (1)
51-54: LGTM! Consider documenting error conditionsThe implementation correctly handles error propagation using the
?operator and maintains backward compatibility withOption<RpcRequestHeader>.Consider adding documentation to describe possible error conditions:
+/// Converts a HashMap into HeartbeatRequestHeader +/// +/// # Errors +/// +/// Returns a RemotingError if: +/// - RpcRequestHeader conversion fails fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> {rocketmq-remoting/src/protocol/header/query_consumer_offset_request_header.rs (1)
Line range hint
81-98: Critical: Improve error handling consistency and robustnessThe current implementation has several potential issues:
- Unsafe parsing operations using
unwrap()could cause panics- Inconsistent error handling patterns (mixing
unwrap,unwrap_or_default, andResult)- No validation for required fields
Consider applying these improvements:
fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { + // Validate required fields + let consumer_group = map + .get(&CheetahString::from_static_str(Self::CONSUMER_GROUP)) + .ok_or_else(|| Self::Error::new("Missing required field: consumerGroup"))? + .clone(); + + let topic = map + .get(&CheetahString::from_static_str(Self::TOPIC)) + .ok_or_else(|| Self::Error::new("Missing required field: topic"))? + .clone(); + + let queue_id = map + .get(&CheetahString::from_static_str(Self::QUEUE_ID)) + .map(|value| value.parse::<i32>()) + .transpose() + .map_err(|e| Self::Error::new(&format!("Invalid queue_id: {}", e)))? + .unwrap_or(0); + Ok(QueryConsumerOffsetRequestHeader { - consumer_group: map - .get(&CheetahString::from_static_str(Self::CONSUMER_GROUP)) - .cloned() - .unwrap_or_default(), - topic: map - .get(&CheetahString::from_static_str(Self::TOPIC)) - .cloned() - .unwrap_or_default(), - queue_id: map - .get(&CheetahString::from_static_str(Self::QUEUE_ID)) - .map_or(0, |value| value.parse::<i32>().unwrap()), + consumer_group, + topic, + queue_id, set_zero_if_not_found: map .get(&CheetahString::from_static_str(Self::SET_ZERO_IF_NOT_FOUND)) - .and_then(|value| value.parse::<bool>().ok()), + .map(|value| value.parse::<bool>()) + .transpose() + .map_err(|e| Self::Error::new(&format!("Invalid set_zero_if_not_found: {}", e)))?, topic_request_header: Some(<TopicRequestHeader as FromMap>::from(map)?), }) }These changes:
- Validate required fields (consumer_group, topic)
- Handle parsing errors properly
- Use consistent error handling patterns
- Provide meaningful error messages
rocketmq-remoting/src/protocol/header/query_message_request_header.rs (2)
114-115: Improve clarity of parse error messagesConsider rephrasing the parse error messages for better clarity. For example, "Parse maxNum field error" could be "Failed to parse maxNum field."
Apply the following diffs to improve the error messages:
For parsing the "maxNum" field:
- Self::Error::RemotingCommandError("Parse maxNum field error".to_string()) + Self::Error::RemotingCommandError("Failed to parse maxNum field".to_string())For parsing the "beginTimestamp" field:
- Self::Error::RemotingCommandError( - "Parse beginTimestamp field error".to_string(), - ) + Self::Error::RemotingCommandError( + "Failed to parse beginTimestamp field".to_string(), + )For parsing the "endTimestamp" field:
- Self::Error::RemotingCommandError("Parse endTimestamp field error".to_string()) + Self::Error::RemotingCommandError("Failed to parse endTimestamp field".to_string())Also applies to: 124-127, 136-137
192-192: Rename test function to reflect expected resultThe test function
creating_from_map_with_invalid_number_fields_returns_noneshould be renamed to reflect that it now expects an error due to the change fromOptiontoResult.Rename the test function for clarity:
- fn creating_from_map_with_invalid_number_fields_returns_none() { + fn creating_from_map_with_invalid_number_fields_returns_error() {rocketmq-remoting/src/protocol/header/delete_topic_request_header.rs (1)
Line range hint
93-146: Tests need to be updated for error handlingThe test cases don't verify error scenarios that could now be properly handled with the Result type.
Consider adding these test cases:
#[test] fn delete_topic_request_header_from_map_with_invalid_topic_request_header() { let mut map = HashMap::new(); // Add invalid data that would cause TopicRequestHeader::from to return an error let result = <DeleteTopicRequestHeader as FromMap>::from(&map); assert!(result.is_err()); }rocketmq-remoting/src/protocol/header/create_topic_request_header.rs (2)
Line range hint
142-180: Improve error handling for numeric field parsingThe current implementation silently ignores parsing errors using
and_then(|v| v.parse().ok()). This should be changed to propagate parsing errors.Consider this improvement:
- read_queue_nums: map - .get(&CheetahString::from_static_str(Self::READ_QUEUE_NUMS)) - .and_then(|v| v.parse().ok()) - .unwrap_or_default(), + read_queue_nums: map + .get(&CheetahString::from_static_str(Self::READ_QUEUE_NUMS)) + .map(|v| v.parse()) + .transpose() + .map_err(|e| RemotingError::ParseError(format!("Invalid read_queue_nums: {}", e)))? + .unwrap_or_default(),Apply similar changes to other numeric fields (write_queue_nums, perm, topic_sys_flag).
Line range hint
185-334: Enhance test coverage for error scenariosThe test suite lacks coverage for error cases that could occur with invalid numeric values.
Add these test cases:
#[test] fn create_topic_request_header_from_map_with_invalid_numeric_values() { let mut map = HashMap::new(); map.insert( CheetahString::from_static_str(CreateTopicRequestHeader::READ_QUEUE_NUMS), CheetahString::from("invalid_number"), ); let result = <CreateTopicRequestHeader as FromMap>::from(&map); assert!(result.is_err()); assert!(result.unwrap_err().to_string().contains("Invalid read_queue_nums")); }rocketmq-remoting/src/protocol/header/pull_message_response_header.rs (2)
Line range hint
102-180: Consider similar error handling improvements in decode_fast methodWhile outside the scope of this PR, the
decode_fastmethod contains similarunwrap()calls that could benefit from proper error handling in a future improvement.Consider creating a follow-up ticket to address error handling in the
decode_fastmethod using a similar approach to what's suggested for thefrommethod.
Critical: Multiple instances of unsafe unwrap() found in FromMap implementations
The search results confirm that unsafe
unwrap()patterns exist in multiple FromMap implementations across the codebase, not just inPullMessageResponseHeader. Here are the key files with unsafe unwrap usage:
get_min_offset_request_header.rsget_max_offset_request_header.rspull_message_response_header.rspull_message_request_header.rsThe pattern of using
unwrap()on parse results is a systemic issue that needs to be addressed consistently across all these implementations to prevent potential runtime panics.Consider using proper error handling with
map_errand descriptive error messages, as suggested in the original review. For example:.map(|v| v.parse().map_err(|e| RemotingError::ParseError(format!("Failed to parse {}: {}", field_name, e))))?This change should be applied consistently across all FromMap implementations to:
- Provide better error messages
- Prevent runtime panics
- Maintain consistency in error handling
🔗 Analysis chain
Line range hint
211-220: Critical: Replace unwrap() calls with proper error handlingThe current implementation uses
unwrap()on allparse()calls, which can cause runtime panics if parsing fails. This defeats the purpose of changing toResultfor better error handling.Consider replacing the unwrap calls with proper error handling:
- Ok(PullMessageResponseHeader { - suggest_which_broker_id: suggest_which_broker_id.map(|v| v.parse().unwrap()), - next_begin_offset: next_begin_offset.map(|v| v.parse().unwrap()), - min_offset: min_offset.map(|v| v.parse().unwrap()), - max_offset: max_offset.map(|v| v.parse().unwrap()), - offset_delta: offset_delta.map(|v| v.parse().unwrap()), - topic_sys_flag: topic_sys_flag.map(|v| v.parse().unwrap()), - group_sys_flag: group_sys_flag.map(|v| v.parse().unwrap()), - forbidden_type: forbidden_type.map(|v| v.parse().unwrap()), + Ok(PullMessageResponseHeader { + suggest_which_broker_id: match suggest_which_broker_id { + Some(v) => Some(v.parse().map_err(|e| RemotingError::ParseError(format!("Failed to parse suggest_which_broker_id: {}", e)))?), + None => None, + }, + next_begin_offset: match next_begin_offset { + Some(v) => Some(v.parse().map_err(|e| RemotingError::ParseError(format!("Failed to parse next_begin_offset: {}", e)))?), + None => None, + }, + min_offset: match min_offset { + Some(v) => Some(v.parse().map_err(|e| RemotingError::ParseError(format!("Failed to parse min_offset: {}", e)))?), + None => None, + }, + max_offset: match max_offset { + Some(v) => Some(v.parse().map_err(|e| RemotingError::ParseError(format!("Failed to parse max_offset: {}", e)))?), + None => None, + }, + offset_delta: match offset_delta { + Some(v) => Some(v.parse().map_err(|e| RemotingError::ParseError(format!("Failed to parse offset_delta: {}", e)))?), + None => None, + }, + topic_sys_flag: match topic_sys_flag { + Some(v) => Some(v.parse().map_err(|e| RemotingError::ParseError(format!("Failed to parse topic_sys_flag: {}", e)))?), + None => None, + }, + group_sys_flag: match group_sys_flag { + Some(v) => Some(v.parse().map_err(|e| RemotingError::ParseError(format!("Failed to parse group_sys_flag: {}", e)))?), + None => None, + }, + forbidden_type: match forbidden_type { + Some(v) => Some(v.parse().map_err(|e| RemotingError::ParseError(format!("Failed to parse forbidden_type: {}", e)))?), + None => None, + }, })This change:
- Properly handles parse errors using
map_err- Provides descriptive error messages
- Propagates errors using the
?operator- Maintains the Option wrapping for optional fields
Let's verify if similar unwrap patterns exist in other FromMap implementations:
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Search for potentially unsafe unwrap calls in FromMap implementations ast-grep --pattern 'impl FromMap for $_ { $$$ parse().unwrap() $$$ }'Length of output: 77
Script:
#!/bin/bash # Let's try a more specific search for unwrap patterns in FromMap implementations rg -U "impl FromMap.*?\{.*?parse\(\)\.unwrap\(\)" --multiline # Also search for similar patterns with map and unwrap rg -U "\.map\(.*?parse\(\)\.unwrap\(\)" --multiline # Let's also check for other FromMap implementations to compare patterns rg -U "impl FromMap for.*?\{" -A 20 --multilineLength of output: 101406
rocketmq-remoting/src/protocol/header/message_operation_header/send_message_request_header_v2.rs (1)
213-221: Improve error message consistency and clarity.The error messages for required fields should be more descriptive and follow a consistent format.
- Self::Error::RemotingCommandError("Miss a field".to_string()), + Self::Error::RemotingCommandError("Missing required field 'producerGroup' (a)".to_string()), - Self::Error::RemotingCommandError("Miss b field".to_string()), + Self::Error::RemotingCommandError("Missing required field 'topic' (b)".to_string()), - Self::Error::RemotingCommandError("Miss c field".to_string()), + Self::Error::RemotingCommandError("Missing required field 'defaultTopic' (c)".to_string()),rocketmq-remoting/src/protocol/header/get_consume_stats_request_header.rs (1)
Line range hint
80-92: Improve error handling for required fieldsThe current implementation has several issues that could lead to runtime problems:
- Using
unwrap_or_default()for potentially required fields (consumer_groupandtopic) silently masks missing data errors- The
topic_request_headeris always set asSomeeven though it'sOption<TopicRequestHeader>in the struct- No validation of required fields before creating the struct
Consider this implementation instead:
fn from( map: &std::collections::HashMap<CheetahString, CheetahString>, ) -> Result<Self::Target, Self::Error> { + let consumer_group = map + .get(&CheetahString::from_static_str(Self::CONSUMER_GROUP)) + .cloned() + .ok_or_else(|| RemotingError::new("Missing required field: consumerGroup"))?; + + let topic = map + .get(&CheetahString::from_static_str(Self::TOPIC)) + .cloned() + .ok_or_else(|| RemotingError::new("Missing required field: topic"))?; + + let topic_request_header = if map.contains_key(&CheetahString::from_static_str(TopicRequestHeader::BROKER_NAME)) { + Some(<TopicRequestHeader as FromMap>::from(map)?) + } else { + None + }; + Ok(GetConsumeStatsRequestHeader { - consumer_group: map - .get(&CheetahString::from_static_str(Self::CONSUMER_GROUP)) - .cloned() - .unwrap_or_default(), - topic: map - .get(&CheetahString::from_static_str(Self::TOPIC)) - .cloned() - .unwrap_or_default(), - topic_request_header: Some(<TopicRequestHeader as FromMap>::from(map)?), + consumer_group, + topic, + topic_request_header, }) }This implementation:
- Returns proper errors for missing required fields
- Only creates TopicRequestHeader when relevant fields are present
- Maintains consistency with the struct definition
rocketmq-remoting/src/protocol/header/unregister_client_request_header.rs (2)
Line range hint
49-66: Handle missing client_id more explicitlyThe current implementation uses
unwrap_or_default()forclient_id, which silently handles missing values. Sinceclient_idappears to be a required field (it's not Option), this could lead to silent failures.Consider this improvement:
- client_id: map - .get(&CheetahString::from_static_str( - UnregisterClientRequestHeader::CLIENT_ID, - )) - .cloned() - .unwrap_or_default(), + client_id: map + .get(&CheetahString::from_static_str( + UnregisterClientRequestHeader::CLIENT_ID, + )) + .cloned() + .ok_or_else(|| RemotingError::new("Missing required field: clientID"))?,
67-67: Consider more explicit error handling for RpcRequestHeaderWhile the error propagation using
?is correct, consider making the error handling more explicit and descriptive.Consider this improvement:
- rpc_request_header: Some(<RpcRequestHeader as FromMap>::from(map)?), + rpc_request_header: Some(<RpcRequestHeader as FromMap>::from(map) + .map_err(|e| RemotingError::new(&format!("Failed to parse RpcRequestHeader: {}", e)))?),rocketmq-remoting/src/protocol/header/pull_message_request_header.rs (3)
Line range hint
331-381: Critical: Properly handle parsing errors infrommethodThe current implementation defaults to zero or default values when parsing fields fails, which can mask errors and lead to unintended behavior. Instead of using
unwrap_or_default()and defaulting to zeros, return an error when parsing fails for required fields. This ensures that any issues with missing or invalid data are promptly reported.Consider updating the
frommethod to propagate parsing errors appropriately. For example:fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { Ok(Self { consumer_group: map .get(&CheetahString::from_static_str(Self::CONSUMER_GROUP)) .cloned() - .unwrap_or_default(), + .ok_or_else(|| RemotingError::MissingField("consumerGroup"))?, topic: map .get(&CheetahString::from_static_str(Self::TOPIC)) .cloned() - .unwrap_or_default(), + .ok_or_else(|| RemotingError::MissingField("topic"))?, queue_id: map .get(&CheetahString::from_static_str(Self::QUEUE_ID)) - .and_then(|value| value.parse::<i32>().ok()), + .map(|value| value.parse::<i32>().map_err(|_| RemotingError::ParseError("queueId"))) + .transpose()?, queue_offset: map .get(&CheetahString::from_static_str(Self::QUEUE_OFFSET)) - .map_or(0, |value| value.parse().unwrap_or_default()), + .ok_or_else(|| RemotingError::MissingField("queueOffset"))? + .parse() + .map_err(|_| RemotingError::ParseError("queueOffset"))?, // Apply similar changes for other required fields...This change ensures that missing or invalid required fields result in an error, enhancing the robustness and reliability of the parsing logic.
Line range hint
332-381: Refactor: Reduce repetition in field parsingThe
frommethod contains repetitive code patterns when extracting and parsing fields from themap. Consider refactoring this logic to reduce duplication and improve readability.For example, you can create a helper function or macro to handle field extraction and parsing:
fn get_required_field<T: std::str::FromStr>( map: &HashMap<CheetahString, CheetahString>, key: &str, ) -> Result<T, RemotingError> { map.get(&CheetahString::from_static_str(key)) .ok_or_else(|| RemotingError::MissingField(key)) .and_then(|value| value.parse::<T>().map_err(|_| RemotingError::ParseError(key))) }Then, use it in your
frommethod:consumer_group: map .get(&CheetahString::from_static_str(Self::CONSUMER_GROUP)) .cloned() .ok_or_else(|| RemotingError::MissingField("consumerGroup"))?, queue_offset: get_required_field(map, Self::QUEUE_OFFSET)?,This refactoring will make the code cleaner and easier to maintain.
Line range hint
331-381: Suggestion: Add unit tests for error handling infrommethodWith the enhanced error handling in the
frommethod, it's important to add unit tests to verify that parsing errors are correctly detected and that appropriate errors are returned when required fields are missing or invalid.Consider adding tests that cover scenarios such as:
- Missing required fields.
- Invalid data types for fields.
- Successful parsing with all required and optional fields present.
rocketmq-remoting/src/protocol/header/consumer_send_msg_back_request_header.rs (2)
135-139: Consider returning an error when parsingunit_modefailsCurrently, if parsing
unit_modefails, the code defaults tofalsewithout signaling an error. For consistency and better error reporting, consider returning an error when parsing fails.Apply this diff to handle parsing errors:
unit_mode: map .get(&CheetahString::from_static_str(Self::UNIT_MODE)) .cloned() .unwrap_or(CheetahString::from_static_str("false")) .parse() - .unwrap_or(false), + .map_err(|_| Self::Error::RemotingCommandError("Invalid unit_mode".to_string()))?,
141-142: Handle parsing errors formax_reconsume_timesWhen parsing
max_reconsume_times, parsing failures are ignored, andNoneis returned. To enhance error visibility, consider returning an error if the parsing fails.Apply this diff to handle parsing errors:
max_reconsume_times: map .get(&CheetahString::from_static_str(Self::MAX_RECONSUME_TIMES)) - .and_then(|value| value.parse().ok()), + .map_or(Ok(None), |value| { + value.parse().map(Some).map_err(|_| { + Self::Error::RemotingCommandError("Invalid max_reconsume_times".to_string()) + }) + })?,rocketmq-remoting/src/protocol/header/namesrv/perm_broker_header.rs (4)
52-57: Consider adding broker name validationWhile the implementation safely handles missing values, it might silently accept invalid (empty) broker names. Consider adding validation to return an error for empty broker names, as they might cause issues downstream.
fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { - Ok(WipeWritePermOfBrokerRequestHeader { - broker_name: map - .get(&CheetahString::from_static_str( - WipeWritePermOfBrokerRequestHeader::BROKER_NAME, - )) - .cloned() - .unwrap_or_default(), - }) + let broker_name = map + .get(&CheetahString::from_static_str( + WipeWritePermOfBrokerRequestHeader::BROKER_NAME, + )) + .cloned() + .unwrap_or_default(); + + if broker_name.is_empty() { + return Err(RemotingError::InvalidHeader("broker_name cannot be empty".into())); + } + + Ok(WipeWritePermOfBrokerRequestHeader { broker_name }) }
134-139: Consider extracting common broker name validationThis implementation shares validation needs with
WipeWritePermOfBrokerRequestHeader. Consider extracting the broker name validation into a shared helper function.+impl AddWritePermOfBrokerRequestHeader { + fn validate_broker_name(name: &CheetahString) -> Result<(), RemotingError> { + if name.is_empty() { + return Err(RemotingError::InvalidHeader("broker_name cannot be empty".into())); + } + Ok(()) + } +} + impl FromMap for AddWritePermOfBrokerRequestHeader { type Error = crate::remoting_error::RemotingError; type Target = Self; fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { - Ok(AddWritePermOfBrokerRequestHeader { - broker_name: map - .get(&CheetahString::from_static_str( - AddWritePermOfBrokerRequestHeader::BROKER_NAME, - )) - .cloned() - .unwrap_or_default(), - }) + let broker_name = map + .get(&CheetahString::from_static_str(Self::BROKER_NAME)) + .cloned() + .unwrap_or_default(); + + Self::validate_broker_name(&broker_name)?; + Ok(Self { broker_name }) } }
174-179: Extract common topic count validation logicThis implementation shares validation needs with
WipeWritePermOfBrokerResponseHeader. Consider extracting the topic count parsing and validation into a shared trait or helper function.+trait TopicCountValidation { + fn parse_and_validate_topic_count( + map: &HashMap<CheetahString, CheetahString>, + key: &str + ) -> Result<i32, RemotingError> { + let count = map + .get(&CheetahString::from_static_str(key)) + .map(|s| s.parse::<i32>()) + .transpose() + .map_err(|_| RemotingError::InvalidHeader("invalid topic count format".into()))? + .unwrap_or(0); + + if count < 0 { + return Err(RemotingError::InvalidHeader("topic count cannot be negative".into())); + } + Ok(count) + } +} + +impl TopicCountValidation for AddWritePermOfBrokerResponseHeader {} + impl FromMap for AddWritePermOfBrokerResponseHeader { type Error = crate::remoting_error::RemotingError; type Target = Self; fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { - Ok(AddWritePermOfBrokerResponseHeader { - add_topic_count: map - .get(&CheetahString::from_static_str( - AddWritePermOfBrokerResponseHeader::ADD_TOPIC_COUNT, - )) - .and_then(|s| s.parse::<i32>().ok()) - .unwrap_or(0), - }) + let add_topic_count = Self::parse_and_validate_topic_count(map, Self::ADD_TOPIC_COUNT)?; + Ok(Self { add_topic_count }) } }
Line range hint
52-179: Overall implementation feedbackThe changes successfully transition from
OptiontoResultreturn types, improving error handling capabilities. However, there are opportunities for enhancement:
- The implementations could benefit from more robust error handling instead of silently falling back to default values
- Common validation logic could be extracted into shared traits or helper functions
- Consider adding comprehensive unit tests for the new error handling paths
Would you like me to help create a comprehensive test suite for these changes?
rocketmq-remoting/src/protocol/header/end_transaction_request_header.rs (1)
Line range hint
121-172: Critical: Improve error handling in the FromMap implementation.The current implementation doesn't take advantage of the new
Resulttype and silently handles failures:
- All field parsing uses
unwrap_or_default()which masks potential errors- The
RpcRequestHeaderconversion errors are ignored- Required fields (if any) should return errors instead of defaults
Consider this improved implementation:
fn from( map: &std::collections::HashMap<CheetahString, CheetahString>, ) -> Result<Self::Target, Self::Error> { + // Helper closure to extract and parse required fields + let parse_required_field = |key: &str| -> Result<CheetahString, Self::Error> { + map.get(&CheetahString::from_static_str(key)) + .cloned() + .ok_or_else(|| Self::Error::new(format!("Missing required field: {}", key))) + }; + + // Helper closure to parse numeric fields + let parse_numeric = |s: &CheetahString, field: &str| -> Result<u64, Self::Error> { + s.parse::<u64>().map_err(|_| { + Self::Error::new(format!("Invalid numeric value for field: {}", field)) + }) + }; + + // Extract required fields first + let topic = parse_required_field(Self::TOPIC)?; + let producer_group = parse_required_field(Self::PRODUCER_GROUP)?; + let msg_id = parse_required_field(Self::MSG_ID)?; + + // Parse numeric fields with proper error handling + let tran_state_table_offset = map + .get(&CheetahString::from_static_str(Self::TRAN_STATE_TABLE_OFFSET)) + .ok_or_else(|| Self::Error::new("Missing tran_state_table_offset")) + .and_then(|s| parse_numeric(s, Self::TRAN_STATE_TABLE_OFFSET))?; + Ok(EndTransactionRequestHeader { - topic: map - .get(&CheetahString::from_static_str( - EndTransactionRequestHeader::TOPIC, - )) - .cloned() - .unwrap_or_default(), + topic, - producer_group: map - .get(&CheetahString::from_static_str( - EndTransactionRequestHeader::PRODUCER_GROUP, - )) - .cloned() - .unwrap_or_default(), + producer_group, // ... similar changes for other fields ... - rpc_request_header: <RpcRequestHeader as FromMap>::from(map).unwrap_or_default(), + rpc_request_header: <RpcRequestHeader as FromMap>::from(map) + .map_err(|e| Self::Error::new(format!("RPC header error: {}", e)))?, }) }This implementation:
- Properly handles required fields
- Provides meaningful error messages
- Propagates errors from RpcRequestHeader conversion
- Uses helper functions to reduce code duplication
rocketmq-remoting/src/protocol/header/message_operation_header/send_message_request_header.rs (3)
154-156: Correct the grammar in error messages.The error messages for missing fields should use proper grammar to enhance readability and professionalism. Replace "Miss <field> field" with "Missing <field> field".
Apply this diff to correct the error messages:
- .ok_or(Self::Error::RemotingCommandError( - "Miss producerGroup field".to_string(), - ))? + .ok_or(Self::Error::RemotingCommandError( + "Missing producerGroup field".to_string(), + ))? - .ok_or(Self::Error::RemotingCommandError( - "Miss topic field".to_string(), - ))? + .ok_or(Self::Error::RemotingCommandError( + "Missing topic field".to_string(), + ))? - .ok_or(Self::Error::RemotingCommandError( - "Miss defaultTopic field".to_string(), - ))? + .ok_or(Self::Error::RemotingCommandError( + "Missing defaultTopic field".to_string(), + ))? - .ok_or(Self::Error::RemotingCommandError( - "Miss defaultTopicQueueNums field".to_string(), - ))? + .ok_or(Self::Error::RemotingCommandError( + "Missing defaultTopicQueueNums field".to_string(), + ))? - .ok_or(Self::Error::RemotingCommandError( - "Miss queueId field".to_string(), - ))? + .ok_or(Self::Error::RemotingCommandError( + "Missing queueId field".to_string(), + ))? - .ok_or(Self::Error::RemotingCommandError( - "Miss sysFlag field".to_string(), - ))? + .ok_or(Self::Error::RemotingCommandError( + "Missing sysFlag field".to_string(), + ))? - .ok_or(Self::Error::RemotingCommandError( - "Miss bornTimestamp field".to_string(), - ))? + .ok_or(Self::Error::RemotingCommandError( + "Missing bornTimestamp field".to_string(), + ))? - .ok_or(Self::Error::RemotingCommandError( - "Miss flag field".to_string(), - ))? + .ok_or(Self::Error::RemotingCommandError( + "Missing flag field".to_string(), + ))?Also applies to: 160-162, 166-168, 174-175, 184-188, 197-199, 207-209, 217-219
178-182: Enhance parsing error messages with field values.Including the problematic value in parsing error messages can aid in debugging by showing exactly what failed to parse.
Apply this change to include the invalid value in the error messages:
- .map_err(|_| { - Self::Error::RemotingCommandError( - "Parse defaultTopicQueueNums field error".to_string(), - ) - })? + .map_err(|e| { + Self::Error::RemotingCommandError(format!( + "Failed to parse defaultTopicQueueNums field: {}", + e + )) + })? // Apply similar changes to other parsing errors.Repeat this pattern for the parsing errors of
queueId,sysFlag,bornTimestamp, andflagfields.Also applies to: 190-192, 201-203, 211-213, 221-223
Line range hint
223-239: Correct handling of optional fields with parsing.For optional fields like
reconsume_times,unit_mode,batch, andmax_reconsume_times, consider handling parsing errors that may occur if the field is present but contains invalid data.Modify the code to handle parsing errors:
- reconsume_times: map - .get(&CheetahString::from_static_str(Self::RECONSUME_TIMES)) - .and_then(|v| v.parse().ok()), + reconsume_times: map + .get(&CheetahString::from_static_str(Self::RECONSUME_TIMES)) + .map(|v| { + v.parse().map_err(|e| { + Self::Error::RemotingCommandError(format!("Failed to parse reconsumeTimes field: {}", e)) + }) + }) + .transpose()?, // Apply similar changes to `unit_mode`, `batch`, and `max_reconsume_times`.This way, if the field is present but parsing fails, an error is returned instead of silently ignoring the issue.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (47)
rocketmq-broker/src/out_api/broker_outer_api.rs(1 hunks)rocketmq-broker/src/processor/admin_broker_processor/offset_request_handler.rs(2 hunks)rocketmq-broker/src/processor/reply_message_processor.rs(2 hunks)rocketmq-broker/src/processor/send_message_processor.rs(3 hunks)rocketmq-macros/src/request_header_custom.rs(1 hunks)rocketmq-remoting/src/protocol/command_custom_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/broker/broker_heartbeat_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/check_transaction_state_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/client_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/consumer_send_msg_back_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/create_topic_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/delete_topic_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/end_transaction_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/get_all_topic_config_response_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/get_consume_stats_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/get_consumer_connection_list_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/get_consumer_listby_group_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/get_consumer_listby_group_response_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/get_max_offset_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/get_min_offset_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/get_topic_config_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/get_topic_stats_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/heartbeat_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/lock_batch_mq_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/message_operation_header/send_message_request_header.rs(3 hunks)rocketmq-remoting/src/protocol/header/message_operation_header/send_message_request_header_v2.rs(2 hunks)rocketmq-remoting/src/protocol/header/namesrv/broker_request.rs(3 hunks)rocketmq-remoting/src/protocol/header/namesrv/brokerid_change_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/namesrv/kv_config_header.rs(5 hunks)rocketmq-remoting/src/protocol/header/namesrv/perm_broker_header.rs(4 hunks)rocketmq-remoting/src/protocol/header/namesrv/query_data_version_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/namesrv/register_broker_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/namesrv/topic_operation_header.rs(4 hunks)rocketmq-remoting/src/protocol/header/notify_consumer_ids_changed_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/pull_message_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/pull_message_response_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/query_consumer_offset_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/query_message_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/query_topic_consume_by_who_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/query_topics_by_consumer_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/reply_message_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/unlock_batch_mq_request_header.rs(1 hunks)rocketmq-remoting/src/protocol/header/unregister_client_request_header.rs(2 hunks)rocketmq-remoting/src/protocol/header/update_consumer_offset_header.rs(2 hunks)rocketmq-remoting/src/protocol/remoting_command.rs(1 hunks)rocketmq-remoting/src/rpc/rpc_request_header.rs(2 hunks)rocketmq-remoting/src/rpc/topic_request_header.rs(1 hunks)
🔇 Additional comments (56)
rocketmq-remoting/src/protocol/header/get_all_topic_config_response_header.rs (2)
35-36: LGTM! Good error type selection.
The addition of RemotingError as the associated error type aligns well with Rust's error handling patterns and the crate's error handling strategy.
39-40: Verify FromMap trait implementation consistency.
Let's ensure this change is consistently applied across all implementations of the FromMap trait.
✅ Verification successful
FromMap trait implementation is consistent across the codebase
All implementations of the FromMap trait across the codebase consistently use Result<Self::Target, Self::Error> as the return type, and there are no remaining Option return types. The trait definition and all implementations have been updated to use Result.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for any remaining Option<Self::Target> implementations of FromMap
# and verify all implementations have been updated to use Result
# Search for FromMap implementations
echo "Checking FromMap implementations..."
ast-grep --pattern 'impl FromMap for $type {
$$$
}'
# Search for any remaining Option return types in FromMap implementations
echo "Checking for any remaining Option return types..."
rg "fn from.*->.*Option<.*>" --type rust
Length of output: 288
Script:
#!/bin/bash
# Let's try a different approach to find FromMap implementations and their return types
# First, let's find all files that might contain FromMap implementations
echo "Files containing FromMap trait:"
rg "impl.*FromMap" -l
# Then, let's see the actual implementations with context
echo -e "\nFromMap implementations with context:"
rg "impl.*FromMap" -B1 -A5
# Let's also check the trait definition itself
echo -e "\nFromMap trait definition:"
rg "trait FromMap" -B1 -A5
Length of output: 43223
rocketmq-remoting/src/rpc/rpc_request_header.rs (2)
26-26: Importing RemotingError for enhanced error handling
The import of RemotingError is necessary to define the Error associated type in the FromMap implementation, enabling more explicit error reporting.
66-66: Defining the Error associated type in FromMap
Adding type Error = RemotingError; specifies the error type for the FromMap trait implementation, which facilitates robust error handling.
rocketmq-remoting/src/protocol/command_custom_header.rs (3)
22-22: Importing RemotingError for error constraint
The import of RemotingError is necessary to define constraints on the Error associated type in the FromMap trait, enhancing error handling capabilities.
104-104: Defining Error associated type with constraint
Adding type Error: From<RemotingError>; in the FromMap trait specifies that the error type must implement From<RemotingError>, allowing for flexible and consistent error handling in implementations.
110-110: Updating the from method to return a Result
Changing the from method's return type to Result<Self::Target, Self::Error> enhances error handling by enabling implementations to return detailed errors.
rocketmq-remoting/src/protocol/header/client_request_header.rs (3)
53-53: Defining the Error associated type for FromMap
Adding type Error = crate::remoting_error::RemotingError; specifies the error type returned by the from method, improving error reporting and consistency.
57-57: Updating from method to return Result
By changing the from method to return Result<Self::Target, Self::Error>, the implementation can now propagate errors encountered during parsing, enhancing robustness.
70-70: Using the ? operator to handle errors in topic_request_header
The line topic_request_header: Some(<TopicRequestHeader as FromMap>::from(map)?), properly uses the ? operator to propagate any errors from TopicRequestHeader::from, ensuring that parsing failures are not silently ignored.
rocketmq-remoting/src/protocol/header/namesrv/register_broker_header.rs (2)
126-127: [Duplicate] Implement proper error handling for missing mandatory fields
This comment applies similarly to the from method starting at line 126. Ensure that all required fields are validated, and meaningful errors are returned if they are missing.
291-292: [Duplicate] Validate fields and handle errors in from method
Ensure that parsing of the fields in the from method returns appropriate errors when parsing fails, enhancing the overall error handling in the method.
rocketmq-remoting/src/protocol/header/namesrv/broker_request.rs (2)
217-218: [Duplicate] Validate mandatory fields and provide meaningful error messages
The same approach should be applied to BrokerHeartbeatRequestHeader. Validate required fields and return errors if they are missing or invalid, improving error handling in the method.
297-298: [Duplicate] Validate mandatory fields and provide meaningful error messages
Similarly, in GetBrokerMemberGroupRequestHeader, ensure that mandatory fields are validated, and meaningful errors are returned when necessary.
rocketmq-remoting/src/protocol/remoting_command.rs (2)
583-590: Improved error handling in decode_command_custom_header
The change to return an error when ext_fields is None enhances error reporting and prevents silent failures.
595-608: Enhanced error handling in decode_command_custom_header_fast
Switching to Result allows for better error propagation, and handling missing ext_fields explicitly improves code robustness.
rocketmq-broker/src/processor/send_message_processor.rs (1)
Line range hint 985-991: Confirm proper error handling in execute_send_message_hook_after.
The use of if let Ok(ref header) appropriately handles the potential error from decode_command_custom_header. This ensures that the context is only updated when header decoding is successful.
rocketmq-remoting/src/protocol/header/query_topics_by_consumer_request_header.rs (2)
62-63: Updating return type enhances error handling
Defining the Error associated type and changing the return type to Result<Self::Target, Self::Error> improves error propagation and provides more informative error messages.
68-74: Ensure consistent error propagation in from method
The use of the ? operator with RpcRequestHeader::from(map) correctly propagates errors. Ensure that all implementations of FromMap within the codebase follow this pattern for consistency.
Run the following script to verify consistency across FromMap implementations:
✅ Verification successful
Based on the output, I can now generate the final response since I have sufficient information about the FromMap implementations across the codebase.
Error propagation is consistently implemented across all FromMap implementations
All FromMap implementations in the codebase follow a consistent pattern of error propagation:
- They all return
Result<Self::Target, Self::Error>with proper error types - They use the
?operator when calling otherFromMapimplementations (e.g.,<RpcRequestHeader as FromMap>::from(map)?) - Error handling is consistent across all implementations, using either
?for propagation or providing default values withunwrap_or_default()
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check that all `FromMap` implementations return `Result<Self::Target, Self::Error>`.
# Find all implementations of `FromMap` and check their return types.
ast-grep --pattern $'impl FromMap for $_ {
type Error = $_;
type Target = $_;
fn from($_) -> Result<$_, $_> {
$$$
}
}'
Length of output: 100733
rocketmq-remoting/src/protocol/header/update_consumer_offset_header.rs (2)
83-84: LGTM: Error type addition aligns with improved error handling
The addition of RemotingError as the associated Error type is appropriate for the FromMap trait implementation and consistent with the remoting module's error handling approach.
Line range hint 87-111: Verify error handling in dependent code
Since this is a breaking change in error handling, we should verify how errors are handled in code that calls this implementation.
rocketmq-remoting/src/rpc/topic_request_header.rs (1)
53-54: LGTM: Error type definition aligns with the new FromMap trait design
The addition of the Error type is consistent with the PR's objective to improve error handling.
rocketmq-remoting/src/protocol/header/get_min_offset_request_header.rs (1)
65-66: LGTM: Error type definition is consistent
The Error type definition aligns with the project's error handling strategy.
rocketmq-remoting/src/protocol/header/get_max_offset_request_header.rs (1)
83-84: LGTM: Error type definition is consistent
The Error type definition aligns with the project's error handling strategy.
rocketmq-remoting/src/protocol/header/unlock_batch_mq_request_header.rs (2)
47-47: LGTM! Appropriate error type selection.
The addition of RemotingError as the associated Error type is consistent with the crate's error handling patterns and supports the transition to Result-based error handling.
51-55: Verify consistent error handling across FromMap implementations.
The changes look good, but let's ensure consistency across the codebase.
✅ Verification successful
FromMap implementations consistently use Result type across the codebase
Based on the comprehensive search results, all FromMap trait implementations in the codebase consistently:
- Use
Result<Self::Target, Self::Error>as the return type - Define
RemotingErroras the error type - Follow the same pattern of error handling
The changes in unlock_batch_mq_request_header.rs align with this consistent pattern across the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for other FromMap implementations and their error handling patterns
# Find all FromMap implementations
echo "Checking FromMap implementations:"
ast-grep --pattern 'impl FromMap for $type {
$$$
}'
# Check for any remaining Option return types in FromMap implementations
echo -e "\nChecking for any remaining Option return types in FromMap implementations:"
rg "fn from.*->.*Option<" --type rust
Length of output: 337
Script:
#!/bin/bash
# Let's try a different approach to find FromMap implementations
# First, find files that might contain FromMap implementations
echo "Files containing FromMap trait usage:"
rg "FromMap" --type rust -l
# Then look for specific implementations with context
echo -e "\nFromMap implementations with context:"
rg "impl FromMap" --type rust -A 5
# Check for Result usage in from methods
echo -e "\nChecking from method implementations with Result:"
rg "fn from.*Result<" --type rust -A 3
Length of output: 61977
rocketmq-remoting/src/protocol/header/lock_batch_mq_request_header.rs (2)
47-47: LGTM! Appropriate error type selection
The addition of RemotingError as the associated Error type is appropriate for this remoting component and aligns with the PR's objective of enhancing error handling.
51-55: Verify consistent error handling pattern
Let's verify that this error handling pattern is consistently implemented across other FromMap implementations in the codebase.
✅ Verification successful
Error handling pattern is consistently implemented across FromMap implementations
After analyzing all FromMap implementations in the codebase, I can confirm that the error handling pattern is consistent. All implementations:
- Use
Result<Self::Target, Self::Error>as the return type - Define
type Error = RemotingError - Handle errors using the
?operator when calling other FromMap implementations - Return
Ok(...)with the constructed instance
The implementation in lock_batch_mq_request_header.rs follows this consistent pattern:
fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> {
let rpc_request_header = <RpcRequestHeader as FromMap>::from(map)?;
Ok(LockBatchMqRequestHeader {
rpc_request_header: Some(rpc_request_header),
})
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistency in FromMap implementations
# Expected: All FromMap implementations should use Result with RemotingError
# Search for FromMap implementations
ast-grep --pattern 'impl FromMap for $type {
$$$
fn from($map: &HashMap<$_, $_>) -> $ret {
$$$
}
}'
# Specifically check for any remaining Option return types in FromMap implementations
rg "fn from.*->.*Option<" --type rust
Length of output: 151
Script:
#!/bin/bash
# Let's try a different approach to find FromMap implementations
# First, find all files that might contain FromMap implementations
rg "impl.*FromMap" --type rust -l | while read -r file; do
echo "=== $file ==="
# Show the implementation context
rg "impl.*FromMap" -A 10 --type rust "$file"
done
# Also search for trait definition to understand the expected pattern
rg "trait FromMap" --type rust -A 5
Length of output: 36078
rocketmq-remoting/src/protocol/header/notify_consumer_ids_changed_request_header.rs (2)
55-56: LGTM: Appropriate error type selection
The addition of RemotingError as the associated Error type is well-aligned with the crate's error handling patterns.
59-71: Verify FromMap implementations across the codebase
Let's ensure this change is consistent with other FromMap implementations in the codebase.
✅ Verification successful
FromMap implementation is consistent with other implementations in the codebase
The implementation of FromMap for NotifyConsumerIdsChangedRequestHeader follows the same pattern as other implementations in the codebase:
- Uses
get()withCheetahString::from_static_str()to retrieve values - Handles optional fields with
cloned()andunwrap_or_default() - Includes
RpcRequestHeaderas an optional field usingSome(<RpcRequestHeader as FromMap>::from(map)?)
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other FromMap implementations to verify consistency
ast-grep --pattern 'impl FromMap for $_ {
$$$
fn from($_) -> Result<$_, $_> {
$$$
}
}'
# Search for direct usage of NotifyConsumerIdsChangedRequestHeader::from
rg "NotifyConsumerIdsChangedRequestHeader::from" -A 5
Length of output: 101582
rocketmq-remoting/src/protocol/header/get_consumer_listby_group_request_header.rs (1)
65-65: Verify error propagation from RpcRequestHeader::from
The rpc field uses the ? operator, which will return an error if <RpcRequestHeader as FromMap>::from(map) fails. Please verify if it's acceptable for an error in parsing RpcRequestHeader to cause the entire from method to fail. If it's acceptable for rpc to be None when parsing fails, consider handling the error accordingly.
If the rpc field is optional, you might adjust the code as follows:
rpc: Some(<RpcRequestHeader as FromMap>::from(map)?),
+// If it's acceptable for `rpc` to be `None` on error:
+rpc: match <RpcRequestHeader as FromMap>::from(map) {
+ Ok(header) => Some(header),
+ Err(_) => None,
+},rocketmq-remoting/src/protocol/header/get_topic_stats_request_header.rs (2)
55-55: LGTM! Appropriate error type selection
The addition of RemotingError as the associated Error type aligns with Rust's error handling best practices and the PR's objective to improve error reporting.
59-61: Verify consumers of FromMap implementation
The signature change from Option to Result is a breaking change. While this improves error handling, we should ensure all consumers are updated.
✅ Verification successful
Let's try a broader search to find any usage or implementation of this type to understand the impact of the breaking change.
I want to verify how the decode_command_custom_header handles the FromMap conversion and if there are other FromMap implementations still using Option return type, as this will help us understand the full scope of the change.
Based on the search results, I can now generate the final response:
Breaking change is part of a wider refactoring pattern
The change from Option to Result in GetTopicStatsRequestHeader::from is part of a consistent error handling improvement. The decode_command_custom_header implementation in remoting_command.rs expects a Result return type from FromMap trait implementations, and all usages across the codebase handle this through unwrap/expect. No legacy Option-based implementations were found.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct usage of FromMap::from for GetTopicStatsRequestHeader
rg -l "GetTopicStatsRequestHeader.*from" | xargs rg "FromMap::from"
# Search for pattern matching on Option return type
ast-grep --pattern 'match $x {
Some($y) => $$$,
None => $$$
}'
Length of output: 136
Script:
#!/bin/bash
# Search for any usage of GetTopicStatsRequestHeader
rg -A 3 "GetTopicStatsRequestHeader"
# Search for implementations of FromMap trait
ast-grep --pattern 'impl FromMap for GetTopicStatsRequestHeader'
# Search for any match expressions involving GetTopicStatsRequestHeader
rg -A 5 "match.*GetTopicStatsRequestHeader"
# Search for any direct usage of the type
rg -A 3 "::GetTopicStatsRequestHeader"
Length of output: 6149
Script:
#!/bin/bash
# Let's check the decode_command_custom_header implementation since it's used with GetTopicStatsRequestHeader
rg -A 10 "decode_command_custom_header"
# Also check for any other FromMap implementations in the codebase to see if this is a wider pattern change
ast-grep --pattern 'impl FromMap for $_ {
$$$
fn from($_) -> Option<$_> {
$$$
}
}'
Length of output: 62861
rocketmq-remoting/src/protocol/header/heartbeat_request_header.rs (2)
47-47: LGTM! Good choice of error type
The addition of RemotingError as the associated error type provides consistent error handling across the remoting module.
51-54: Verify consistent FromMap implementation pattern
Let's ensure this change pattern is consistently applied across all FromMap implementations.
✅ Verification successful
FromMap implementation pattern is consistent across the codebase
The implementation in heartbeat_request_header.rs follows the correct pattern, which is consistently used across all FromMap implementations in the codebase. All implementations:
- Use
Result<Self::Target, Self::Error>as the return type - Have
type Error = RemotingError(or its qualified path) - Have
type Target = Self - Return
ResultwithOk(...)pattern
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining Option<Self::Target> implementations of FromMap
# that haven't been updated to Result
# Look for FromMap implementations
ast-grep --pattern 'impl FromMap for $type {
$$$
fn from($_) -> Option<$_> {
$$$
}
}'
# Also check for any direct usage of the old Option return type
rg "FromMap::from.*Option<" --type rust
Length of output: 139
Script:
#!/bin/bash
# Let's search for all FromMap implementations to verify the pattern
ast-grep --pattern 'impl FromMap for $type {
$$$
fn from($map: &HashMap<CheetahString, CheetahString>) -> Result<$_, $_> {
$$$
}
}'
# Also search for any FromMap trait definitions to understand the expected signature
rg "trait FromMap" -A 5 --type rust
# Search for any files that might use FromMap to verify consistency
rg "impl FromMap" --type rust
Length of output: 7206
Script:
#!/bin/bash
# Let's check the implementation of a few of these headers to verify the Result pattern
ast-grep --pattern 'impl FromMap for $_ {
type Error = $_;
type Target = $_;
fn from($_) -> $ret_type {
$$$
}
}'
# Also check a few specific implementations to verify the pattern
rg "impl FromMap" -A 10 rocketmq-remoting/src/protocol/header/get_all_topic_config_response_header.rs
rg "impl FromMap" -A 10 rocketmq-remoting/src/protocol/header/end_transaction_request_header.rs
rg "impl FromMap" -A 10 rocketmq-remoting/src/rpc/rpc_request_header.rs
Length of output: 1472
rocketmq-remoting/src/protocol/header/query_consumer_offset_request_header.rs (2)
77-78: LGTM! Good error type selection.
The addition of the RemotingError type as the associated Error type is appropriate for this context and aligns well with the PR's goal of improving error handling.
Line range hint 81-98: Verify consistent error handling across FromMap implementations
Let's ensure that other implementations of FromMap follow similar error handling patterns.
✅ Verification successful
Based on the extensive search results, I can now generate a final response regarding the error handling patterns across FromMap implementations.
Error handling is consistent across FromMap implementations in the codebase
The error handling patterns in the QueryConsumerOffsetRequestHeader implementation align with the broader codebase's practices:
- Using
unwrap_or_default()for optional string fields - Using
map_or()with default values for numeric parsing - Using
and_then()withparse().ok()for optional boolean parsing
These patterns are consistently used across numerous FromMap implementations in the protocol headers, ensuring uniform error handling throughout the codebase.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other FromMap implementations to verify consistent error handling
ast-grep --pattern 'impl FromMap for $type {
$$$
fn from($map: &HashMap<$_, $_>) -> $ret {
$$$
}
$$$
}'
Length of output: 117
Script:
#!/bin/bash
# Let's try a different approach to find FromMap implementations
# First, let's find the trait definition
rg "trait FromMap" -A 5
# Then search for all impl blocks for FromMap
rg "impl FromMap"
# Also search for specific error handling patterns in Rust files
rg "map_or|unwrap_or|and_then.*parse|parse.*unwrap" --type rust
Length of output: 71167
rocketmq-remoting/src/protocol/header/get_consumer_listby_group_response_header.rs (2)
35-37: LGTM! Well-defined associated types
The addition of explicit Error and Target associated types improves the trait implementation's clarity and type safety. Using RemotingError as the error type is appropriate for the remoting protocol context.
39-40: Consider adding validation logic
While the change from Option to Result is good for error handling, the current implementation always returns Ok(Self {}) without any validation. Consider whether there are any conditions that should be validated before creating the response header.
Let's check if other response headers perform validation:
If validation is needed, consider this pattern:
fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> {
// Add validation logic here if needed
// Example:
// if let Some(value) = map.get("some_required_field") {
// // Validate value
// } else {
// return Err(RemotingError::InvalidField("missing required field"));
// }
Ok(Self {})
}rocketmq-remoting/src/protocol/header/pull_message_response_header.rs (2)
181-182: LGTM: Error type definition is appropriate
The addition of the RemotingError type aligns with the PR objective of improving error handling and follows Rust conventions.
185-185: LGTM: Method signature updated correctly
The change from Option to Result return type properly implements the new error handling approach specified in the PR objectives.
rocketmq-remoting/src/protocol/header/message_operation_header/send_message_request_header_v2.rs (1)
207-208: LGTM! Good error type choice.
The RemotingError is an appropriate error type for this context, allowing for proper error propagation in the remoting operations.
rocketmq-remoting/src/protocol/header/get_consume_stats_request_header.rs (2)
76-77: LGTM! Appropriate error type selection
The addition of RemotingError as the associated Error type is well-aligned with the remoting module's error handling patterns.
Line range hint 80-92: Verify similar FromMap implementations across the codebase
Let's check if other FromMap implementations have similar patterns that need improvement.
rocketmq-remoting/src/protocol/header/unregister_client_request_header.rs (2)
45-46: LGTM! Good error type choice
The addition of RemotingError as the associated Error type is appropriate and aligns with Rust's error handling best practices.
Line range hint 49-67: Verify consistent error handling across FromMap implementations
Let's ensure this change is consistent with other FromMap implementations in the codebase.
✅ Verification successful
Based on the search results, I can see that the error handling in the FromMap implementation for UnregisterClientRequestHeader is consistent with other implementations in the codebase. The pattern follows:
- Required fields use
ok_or()with error messages - Optional fields use
and_then()or direct unwrap with defaults - Nested headers (like
RpcRequestHeader) are handled withSome(<Type as FromMap>::from(map)?)pattern
Let me verify direct usage of UnregisterClientRequestHeader::from:
Error handling is consistent with codebase patterns
The implementation of FromMap for UnregisterClientRequestHeader follows the established error handling patterns in the codebase:
- Required field
client_idusesunwrap_or_default()which is consistent with similar headers likeGetConsumeStatsRequestHeader - Optional fields
producer_groupandconsumer_groupusecloned()without error handling, matching other implementations - Nested
RpcRequestHeaderuses the standardSome(<RpcRequestHeader as FromMap>::from(map)?)pattern seen across the codebase
The usage is also verified in:
rocketmq-broker/src/processor/client_manage_processor.rs: Used in request decodingrocketmq-client/src/implementation/mq_client_api_impl.rs: Used in client API implementation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other FromMap implementations to verify consistent error handling
ast-grep --pattern 'impl FromMap for $_ {
$$$
fn from($_) -> Result<$_, $_> {
$$$
}
}'
# Search for direct usage of UnregisterClientRequestHeader::from
rg "UnregisterClientRequestHeader::from" -A 5
Length of output: 100953
Script:
#!/bin/bash
# Search for direct usage of UnregisterClientRequestHeader::from
rg "UnregisterClientRequestHeader::from" -l
# Search for any usage of UnregisterClientRequestHeader
rg "UnregisterClientRequestHeader" -A 2 -B 2
Length of output: 8407
rocketmq-remoting/src/protocol/header/get_topic_config_request_header.rs (1)
62-74: LGTM! The FromMap implementation correctly adopts Result-based error handling
The changes align well with the PR objectives:
- Properly defines the Error associated type
- Returns Result instead of Option
- Correctly propagates errors from TopicRequestHeader using the ? operator
rocketmq-remoting/src/protocol/header/reply_message_request_header.rs (2)
171-172: Proper definition of the Error associated type
Defining type Error = crate::remoting_error::RemotingError; correctly specifies the error type for the FromMap trait implementation, enhancing error handling consistency.
175-176: Enhanced error handling in the from method
Changing the return type of from to Result<Self::Target, Self::Error> allows for explicit error propagation, improving the robustness of the mapping process.
rocketmq-remoting/src/protocol/header/query_topic_consume_by_who_request_header.rs (1)
55-56: LGTM: Appropriate error type selection
The addition of RemotingError as the associated Error type is correct and aligns with the crate's error handling patterns.
rocketmq-remoting/src/protocol/header/pull_message_request_header.rs (2)
327-328: Approved: Addition of type Error associated type
Defining type Error = crate::remoting_error::RemotingError; correctly specifies the error type for the FromMap trait implementation, aligning with the change to return a Result.
381-381: Verify error propagation for optional topic_request field
The line initializing topic_request uses the ? operator:
topic_request: Some(<TopicRequestHeader as FromMap>::from(map)?),This means that any error from TopicRequestHeader::from(map) will cause the entire from method to return an error. If topic_request is meant to be optional, consider whether it's appropriate to fail the entire parsing process due to an error in this field. You may want to handle errors from topic_request separately to allow parsing to succeed even if this optional field is invalid.
Would you like assistance in modifying the code to handle this case?
rocketmq-remoting/src/protocol/header/consumer_send_msg_back_request_header.rs (2)
96-127: Enhanced error handling for required fields is correctly implemented
The conversion now properly handles missing or invalid required fields (offset, group, delay_level) by returning descriptive errors, improving robustness.
143-143: Verify error handling in RpcRequestHeader::from
The rpc_request_header field uses FromMap::from(map)?, which propagates any errors from RpcRequestHeader::from. Ensure that RpcRequestHeader::from has been updated to return Result and handles errors consistently with this implementation.
Run the following script to verify the FromMap implementation of RpcRequestHeader:
✅ Verification successful
Let me gather more context about the error handling implementation.
Error handling in RpcRequestHeader::from is properly implemented
The implementation correctly returns Result<Self::Target, RemotingError> and follows the FromMap trait requirements. The error type is properly defined as RemotingError which satisfies the trait bound type Error: From<RemotingError>. The implementation handles optional fields safely using Option types and parse operations with ok() for graceful error handling.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify that RpcRequestHeader's FromMap implementation returns Result and handles errors.
# Test: Check the signature of RpcRequestHeader's from method.
rg 'impl FromMap for RpcRequestHeader' -A 10 | rg 'fn from\(' -A 2
Length of output: 365
Script:
#!/bin/bash
# Get more context around the FromMap implementation for RpcRequestHeader
ast-grep --pattern 'impl FromMap for RpcRequestHeader {
$$$
}'
# Also check the FromMap trait definition to understand the expected error type
rg 'trait FromMap' -A 5
Length of output: 2757
rocketmq-remoting/src/protocol/header/end_transaction_request_header.rs (1)
114-120: LGTM! The trait implementation aligns with the PR objectives.
The change from Option to Result with RemotingError as the error type is a good improvement for better error handling. This allows for more detailed error reporting instead of just None.
Let's verify that all consumers of this trait have been updated:
✅ Verification successful
All FromMap implementations have been updated to use Result return type
Based on the comprehensive search results, all implementations of the FromMap trait across the codebase have been updated to use Result as the return type with RemotingError as the error type. The verification shows consistent implementation pattern:
impl FromMap for Type {
type Error = RemotingError;
type Target = Self;
fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error>
}🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for any remaining FromMap implementations that still return Option
ast-grep --pattern 'impl FromMap for $_ {
$$$
fn from($_) -> Option<$_> {
$$$
}
}'
Length of output: 94
Script:
#!/bin/bash
# Let's first find all files that might implement FromMap
rg "impl.*FromMap" -l
# Then let's look at the actual implementations to verify the return types
rg "impl.*FromMap" -A 10
Length of output: 69588
rocketmq-remoting/src/protocol/header/message_operation_header/send_message_request_header.rs (1)
357-359: Verify the decoding of SendMessageRequestHeaderV2.
Ensure that the decode_command_custom_header method returns a Result instead of an Option, aligning with the new error handling approach.
Run the following script to verify the method's return type:
| type Error = crate::remoting_error::RemotingError; | ||
|
|
||
| type Target = Self; | ||
|
|
||
| fn from(map: &HashMap<CheetahString, CheetahString>) -> Option<Self::Target> { | ||
| Some(DeleteTopicFromNamesrvRequestHeader { | ||
| fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { | ||
| Ok(DeleteTopicFromNamesrvRequestHeader { | ||
| topic: map | ||
| .get(&CheetahString::from_static_str(Self::TOPIC)) | ||
| .cloned() |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate required fields in 'from' implementations
In the from method for DeleteTopicFromNamesrvRequestHeader, the topic field is defaulted to an empty value if missing (unwrap_or_default()). Since topic is likely a required field, it's better to return an error when it's absent to prevent unintended behavior.
Consider updating the code to validate the presence of the required topic field:
fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> {
let topic = map
.get(&CheetahString::from_static_str(Self::TOPIC))
.cloned()
.ok_or(Self::Error::RemotingCommandError(
"Missing 'topic' field".to_string(),
))?;
let cluster_name = map
.get(&CheetahString::from_static_str(Self::CLUSTER_NAME))
.cloned();
Ok(DeleteTopicFromNamesrvRequestHeader {
topic,
cluster_name,
})
}| type Error = crate::remoting_error::RemotingError; | ||
|
|
||
| type Target = Self; | ||
|
|
||
| fn from(map: &HashMap<CheetahString, CheetahString>) -> Option<Self::Target> { | ||
| Some(RegisterTopicRequestHeader { | ||
| fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { | ||
| Ok(RegisterTopicRequestHeader { | ||
| topic: map | ||
| .get(&CheetahString::from_static_str(Self::TOPIC)) | ||
| .cloned() | ||
| .unwrap_or_default(), | ||
| topic_request: <TopicRequestHeader as FromMap>::from(map), | ||
| topic_request: Some(<TopicRequestHeader as FromMap>::from(map)?), |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate required fields in 'from' implementations
In the from method for RegisterTopicRequestHeader, the topic field is defaulted to an empty value if missing (unwrap_or_default()). Since topic is a required field, it's recommended to return an error if it's missing to ensure proper error handling.
[duplicate_comment from lines 67-75]
Consider updating the code as shown previously to return an error when topic is missing.
| type Error = crate::remoting_error::RemotingError; | ||
|
|
||
| type Target = Self; | ||
|
|
||
| fn from(map: &HashMap<CheetahString, CheetahString>) -> Option<Self::Target> { | ||
| Some(GetTopicsByClusterRequestHeader { | ||
| fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { | ||
| Ok(GetTopicsByClusterRequestHeader { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Validate required fields in 'from' implementations
The cluster field in GetTopicsByClusterRequestHeader is defaulted to an empty value if missing (unwrap_or_default()). Since cluster is required, consider returning an error when it's not present to prevent unexpected behavior.
[duplicate_comment from lines 67-75]
Update the from method to validate the presence of cluster:
fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> {
let cluster = map
.get(&CheetahString::from_static_str(Self::CLUSTER))
.cloned()
.ok_or(Self::Error::RemotingCommandError(
"Missing 'cluster' field".to_string(),
))?;
Ok(GetTopicsByClusterRequestHeader { cluster })
}|
|
||
| type Error = crate::remoting_error::RemotingError; | ||
|
|
||
| type Target = Self; | ||
|
|
||
| fn from(map: &std::collections::HashMap<cheetah_string::CheetahString, cheetah_string::CheetahString>) -> Option<Self::Target> { | ||
| Some(#struct_name { | ||
| fn from(map: &std::collections::HashMap<cheetah_string::CheetahString, cheetah_string::CheetahString>) -> Result<Self::Target, Self::Error> { | ||
| Ok(#struct_name { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Enhance error handling in generated 'from' methods
In the macro implementation of FromMap, the from method returns Ok(#struct_name { ... }) without explicitly handling missing required fields. This could lead to default values being used inadvertently when required fields are absent.
Consider updating the code generation to validate required fields and return appropriate errors when they are missing. Modify the generation logic to include ok_or for required fields:
#field_name: map
.get(&cheetah_string::CheetahString::from_static_str(Self::#static_name))
.cloned()
.ok_or(Self::Error::RemotingCommandError(
format!("Missing '{}' field", Self::#static_name),
))?,This change ensures that missing required fields are properly reported as errors.
| let request_header = request | ||
| .decode_command_custom_header::<GetMinOffsetRequestHeader>() | ||
| .unwrap(); //need to optimize |
There was a problem hiding this comment.
Avoid using unwrap(); handle errors properly
Using unwrap() can cause the application to panic if an error occurs during the decoding of the command custom header. It's better to handle the error gracefully. Consider using the ? operator to propagate the error or handling it explicitly to prevent unexpected panics.
Apply this diff to handle the error properly:
-let request_header = request
- .decode_command_custom_header::<GetMinOffsetRequestHeader>()
- .unwrap(); //need to optimize
+let request_header = request
+ .decode_command_custom_header::<GetMinOffsetRequestHeader>()?;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let request_header = request | |
| .decode_command_custom_header::<GetMinOffsetRequestHeader>() | |
| .unwrap(); //need to optimize | |
| let request_header = request | |
| .decode_command_custom_header::<GetMinOffsetRequestHeader>()?; |
| @@ -227,7 +229,7 @@ | |||
| .get(&CheetahString::from_static_str(Self::STORE_TIMESTAMP)) | |||
| .and_then(|value| value.parse().ok()) | |||
| .unwrap_or_default(), | |||
| topic_request: <TopicRequestHeader as FromMap>::from(map), | |||
| topic_request: Some(<TopicRequestHeader as FromMap>::from(map)?), | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Potential issue with unconditionally setting topic_request to Some(...)
By unconditionally setting topic_request to Some(<TopicRequestHeader as FromMap>::from(map)?), the code assumes that TopicRequestHeader can always be successfully constructed. If the required fields for TopicRequestHeader are missing from map, this will result in an error.
Consider handling the absence of TopicRequestHeader gracefully by making topic_request conditional based on the availability of required fields.
Apply the following diff to handle conditional creation of topic_request:
-topic_request: Some(<TopicRequestHeader as FromMap>::from(map)?),
+topic_request: match <TopicRequestHeader as FromMap>::from(map) {
+ Ok(header) => Some(header),
+ Err(crate::remoting_error::RemotingError::MissingField(_)) => None,
+ Err(e) => return Err(e),
+},This change ensures that if TopicRequestHeader cannot be constructed due to missing fields, topic_request will be set to None instead of causing the entire from method to fail. Any other errors will still be propagated appropriately.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| topic_request: Some(<TopicRequestHeader as FromMap>::from(map)?), | |
| topic_request: match <TopicRequestHeader as FromMap>::from(map) { | |
| Ok(header) => Some(header), | |
| Err(crate::remoting_error::RemotingError::MissingField(_)) => None, | |
| Err(e) => return Err(e), | |
| }, |
| fn from( | ||
| map: &std::collections::HashMap<CheetahString, CheetahString>, | ||
| ) -> Result<Self::Target, Self::Error> { | ||
| Ok(QueryTopicConsumeByWhoRequestHeader { | ||
| topic: map | ||
| .get(&CheetahString::from_static_str(Self::TOPIC)) | ||
| .cloned() | ||
| .unwrap_or_default(), | ||
| topic_request_header: <TopicRequestHeader as FromMap>::from(map), | ||
| topic_request_header: Some(<TopicRequestHeader as FromMap>::from(map)?), | ||
| }) |
There was a problem hiding this comment.
Critical: Improve error handling consistency and field validation
The current implementation has several issues that should be addressed:
- The
topicfield silently defaults when missing, but it appears to be a required field. This could lead to runtime issues. - Error handling is inconsistent between fields.
- The
Some()wrapper is redundant for thetopic_request_headerfield.
Consider this improved implementation:
fn from(
map: &std::collections::HashMap<CheetahString, CheetahString>,
) -> Result<Self::Target, Self::Error> {
+ let topic = map
+ .get(&CheetahString::from_static_str(Self::TOPIC))
+ .cloned()
+ .ok_or_else(|| Self::Error::new("Missing required field: topic"))?;
+
Ok(QueryTopicConsumeByWhoRequestHeader {
- topic: map
- .get(&CheetahString::from_static_str(Self::TOPIC))
- .cloned()
- .unwrap_or_default(),
- topic_request_header: Some(<TopicRequestHeader as FromMap>::from(map)?),
+ topic,
+ topic_request_header: <TopicRequestHeader as FromMap>::from(map).ok(),
})
}This implementation:
- Properly validates the required
topicfield - Returns an error if
topicis missing - Removes the redundant
Some()wrapper - Maintains backward compatibility by converting the Result to Option for
topic_request_header
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| fn from( | |
| map: &std::collections::HashMap<CheetahString, CheetahString>, | |
| ) -> Result<Self::Target, Self::Error> { | |
| Ok(QueryTopicConsumeByWhoRequestHeader { | |
| topic: map | |
| .get(&CheetahString::from_static_str(Self::TOPIC)) | |
| .cloned() | |
| .unwrap_or_default(), | |
| topic_request_header: <TopicRequestHeader as FromMap>::from(map), | |
| topic_request_header: Some(<TopicRequestHeader as FromMap>::from(map)?), | |
| }) | |
| fn from( | |
| map: &std::collections::HashMap<CheetahString, CheetahString>, | |
| ) -> Result<Self::Target, Self::Error> { | |
| let topic = map | |
| .get(&CheetahString::from_static_str(Self::TOPIC)) | |
| .cloned() | |
| .ok_or_else(|| Self::Error::new("Missing required field: topic"))?; | |
| Ok(QueryTopicConsumeByWhoRequestHeader { | |
| topic, | |
| topic_request_header: <TopicRequestHeader as FromMap>::from(map).ok(), | |
| }) | |
| } |
| type Error = crate::remoting_error::RemotingError; | ||
|
|
||
| type Target = Self; | ||
|
|
||
| fn from(map: &HashMap<CheetahString, CheetahString>) -> Option<Self::Target> { | ||
| Some(WipeWritePermOfBrokerResponseHeader { | ||
| fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { | ||
| Ok(WipeWritePermOfBrokerResponseHeader { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling for invalid wipe_topic_count values
The current implementation silently converts invalid values to 0. Consider explicitly handling parsing errors and validating for negative counts.
fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> {
- Ok(WipeWritePermOfBrokerResponseHeader {
- wipe_topic_count: map
- .get(&CheetahString::from_static_str(
- WipeWritePermOfBrokerResponseHeader::WIPE_TOPIC_COUNT,
- ))
- .and_then(|s| s.parse::<i32>().ok())
- .unwrap_or(0),
- })
+ let wipe_topic_count = map
+ .get(&CheetahString::from_static_str(
+ WipeWritePermOfBrokerResponseHeader::WIPE_TOPIC_COUNT,
+ ))
+ .map(|s| s.parse::<i32>())
+ .transpose()
+ .map_err(|_| RemotingError::InvalidHeader("invalid wipe_topic_count format".into()))?
+ .unwrap_or(0);
+
+ if wipe_topic_count < 0 {
+ return Err(RemotingError::InvalidHeader("wipe_topic_count cannot be negative".into()));
+ }
+
+ Ok(WipeWritePermOfBrokerResponseHeader { wipe_topic_count })
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| type Error = crate::remoting_error::RemotingError; | |
| type Target = Self; | |
| fn from(map: &HashMap<CheetahString, CheetahString>) -> Option<Self::Target> { | |
| Some(WipeWritePermOfBrokerResponseHeader { | |
| fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { | |
| Ok(WipeWritePermOfBrokerResponseHeader { | |
| type Error = crate::remoting_error::RemotingError; | |
| type Target = Self; | |
| fn from(map: &HashMap<CheetahString, CheetahString>) -> Result<Self::Target, Self::Error> { | |
| let wipe_topic_count = map | |
| .get(&CheetahString::from_static_str( | |
| WipeWritePermOfBrokerResponseHeader::WIPE_TOPIC_COUNT, | |
| )) | |
| .map(|s| s.parse::<i32>()) | |
| .transpose() | |
| .map_err(|_| RemotingError::InvalidHeader("invalid wipe_topic_count format".into()))? | |
| .unwrap_or(0); | |
| if wipe_topic_count < 0 { | |
| return Err(RemotingError::InvalidHeader("wipe_topic_count cannot be negative".into())); | |
| } | |
| Ok(WipeWritePermOfBrokerResponseHeader { wipe_topic_count }) |
| ) -> crate::Result<SendMessageRequestHeader> { | ||
| let mut request_header_v2 = None; | ||
| if RequestCode::SendMessageV2 == request_code || RequestCode::SendBatchMessage == request_code { | ||
| request_header_v2 = request.decode_command_custom_header::<SendMessageRequestHeaderV2>(); | ||
| request_header_v2 = request | ||
| .decode_command_custom_header::<SendMessageRequestHeaderV2>() | ||
| .ok(); | ||
| } | ||
|
|
||
| match request_header_v2 { | ||
| Some(header) => { | ||
| Some(SendMessageRequestHeaderV2::create_send_message_request_header_v1(&header)) | ||
| Ok(SendMessageRequestHeaderV2::create_send_message_request_header_v1(&header)) |
There was a problem hiding this comment.
Ensure consistent return types in parse_request_header function.
The parse_request_header function returns crate::Result<SendMessageRequestHeader>, but in the case where request_header_v2 is None, it directly calls request.decode_command_custom_header::<SendMessageRequestHeader>(), which may return an Option.
Make sure that the function consistently returns a Result type.
None => request.decode_command_custom_header::<SendMessageRequestHeader>(),
+ .ok_or_else(|| {
+ crate::remoting_error::RemotingError::RemotingCommandError(
+ "Failed to decode SendMessageRequestHeader".to_string(),
+ )
+ }),This change ensures that if decoding fails, an appropriate error is returned.
Committable suggestion skipped: line range outside the PR's diff.
| queue_id: Some( | ||
| map.get(&CheetahString::from_static_str(Self::QUEUE_ID)) | ||
| .cloned() | ||
| .ok_or(Self::Error::RemotingCommandError( | ||
| "Miss queueId field".to_string(), | ||
| ))? | ||
| .parse() | ||
| .map_err(|_| { | ||
| Self::Error::RemotingCommandError("Parse queueId field error".to_string()) | ||
| })?, | ||
| ), |
There was a problem hiding this comment.
Handle optional queue_id correctly.
The queue_id field is defined as Option<i32>, but the current implementation returns an error if it's missing. If queue_id is indeed optional, the code should handle its absence gracefully.
Modify the code to handle queue_id as an optional field:
- queue_id: Some(
- map.get(&CheetahString::from_static_str(Self::QUEUE_ID))
- .cloned()
- .ok_or(Self::Error::RemotingCommandError(
- "Missing queueId field".to_string(),
- ))?
- .parse()
- .map_err(|e| {
- Self::Error::RemotingCommandError(format!("Failed to parse queueId field: {}", e))
- })?,
- ),
+ queue_id: map
+ .get(&CheetahString::from_static_str(Self::QUEUE_ID))
+ .cloned()
+ .map(|v| {
+ v.parse().map_err(|e| {
+ Self::Error::RemotingCommandError(format!("Failed to parse queueId field: {}", e))
+ })
+ })
+ .transpose()?,This change ensures that if queue_id is present, it parses the value; if it's absent, it remains None.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| queue_id: Some( | |
| map.get(&CheetahString::from_static_str(Self::QUEUE_ID)) | |
| .cloned() | |
| .ok_or(Self::Error::RemotingCommandError( | |
| "Miss queueId field".to_string(), | |
| ))? | |
| .parse() | |
| .map_err(|_| { | |
| Self::Error::RemotingCommandError("Parse queueId field error".to_string()) | |
| })?, | |
| ), | |
| queue_id: map | |
| .get(&CheetahString::from_static_str(Self::QUEUE_ID)) | |
| .cloned() | |
| .map(|v| { | |
| v.parse().map_err(|e| { | |
| Self::Error::RemotingCommandError(format!("Failed to parse queueId field: {}", e)) | |
| }) | |
| }) | |
| .transpose()?, |
Which Issue(s) This PR Fixes(Closes)
Fixes #1470
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
Resulttypes instead ofOption, allowing for explicit error reporting.?operator in several methods, enhancing code maintainability.