rocketmq-remoting/src/protocol/header/check_transaction_state_request_header.rs (2)

Line range hint 39-122: Remove obsolete commented-out code

There is a large block of commented-out code spanning lines 39 to 122, including implementations of CommandCustomHeader and FromMap. Since the RequestHeaderCodec macro now handles these implementations, consider removing this obsolete code to enhance readability and maintainability.

Apply this diff to remove the commented-out code:

-/*impl CheckTransactionStateRequestHeader {
-    pub const TOPIC: &'static str = "topic";
-    pub const TRAN_STATE_TABLE_OFFSET: &'static str = "tranStateTableOffset";
-    pub const COMMIT_LOG_OFFSET: &'static str = "commitLogOffset";
-    pub const MSG_ID: &'static str = "msgId";
-    pub const TRANSACTION_ID: &'static str = "transactionId";
-    pub const OFFSET_MSG_ID: &'static str = "offsetMsgId";
-}
-
-impl CommandCustomHeader for CheckTransactionStateRequestHeader {
-    fn to_map(&self) -> Option<std::collections::HashMap<CheetahString, CheetahString>> {
-        let mut map = std::collections::HashMap::new();
-        if let Some(value) = self.topic.as_ref() {
-            map.insert(CheetahString::from_static_str(Self::TOPIC), value.clone());
-        }
-        map.insert(
-            CheetahString::from_static_str(Self::TRAN_STATE_TABLE_OFFSET),
-            CheetahString::from_string(self.tran_state_table_offset.to_string()),
-        );
-
-        map.insert(
-            CheetahString::from_static_str(Self::COMMIT_LOG_OFFSET),
-            CheetahString::from_string(self.commit_log_offset.to_string()),
-        );
-        if let Some(value) = self.msg_id.as_ref() {
-            map.insert(CheetahString::from_static_str(Self::MSG_ID), value.clone());
-        }
-        if let Some(value) = self.transaction_id.as_ref() {
-            map.insert(
-                CheetahString::from_static_str(Self::TRANSACTION_ID),
-                value.clone(),
-            );
-        }
-        if let Some(value) = self.offset_msg_id.as_ref() {
-            map.insert(
-                CheetahString::from_static_str(Self::OFFSET_MSG_ID),
-                value.clone(),
-            );
-        }
-        if let Some(value) = self.rpc_request_header.as_ref() {
-            if let Some(value) = value.to_map() {
-                map.extend(value);
-            }
-        }
-        Some(map)
-    }
-}
-
-impl FromMap for CheckTransactionStateRequestHeader {
-    type Error = crate::remoting_error::RemotingError;
-
-    type Target = Self;
-
-    fn from(
-        map: &std::collections::HashMap<CheetahString, CheetahString>,
-    ) -> Result<Self::Target, Self::Error> {
-        Ok(CheckTransactionStateRequestHeader {
-            topic: map
-                .get(&CheetahString::from_static_str(Self::TOPIC))
-                .cloned(),
-            tran_state_table_offset: map
-                .get(&CheetahString::from_static_str(
-                    Self::TRAN_STATE_TABLE_OFFSET,
-                ))
-                .and_then(|v| v.parse().ok())
-                .unwrap_or_default(),
-            commit_log_offset: map
-                .get(&CheetahString::from_static_str(Self::COMMIT_LOG_OFFSET))
-                .and_then(|v| v.parse().ok())
-                .unwrap_or_default(),
-            msg_id: map
-                .get(&CheetahString::from_static_str(Self::MSG_ID))
-                .cloned(),
-            transaction_id: map
-                .get(&CheetahString::from_static_str(Self::TRANSACTION_ID))
-                .cloned(),
-            offset_msg_id: map
-                .get(&CheetahString::from_static_str(Self::OFFSET_MSG_ID))
-                .cloned(),
-            rpc_request_header: Some(<RpcRequestHeader as FromMap>::from(map)?),
-        })
-    }
-}
-*/

---

215-232: Add tests for missing required fields

While there is a test for missing optional fields, consider adding a test case where required fields (tran_state_table_offset or commit_log_offset) are missing to ensure that deserialization fails as expected when mandatory data is absent.

Example test case to add:

#[test]
fn check_transaction_state_request_header_errors_on_missing_required_fields() {
    let mut map = HashMap::new();
    // Intentionally omit 'tranStateTableOffset' and 'commitLogOffset'
    let result = <CheckTransactionStateRequestHeader as FromMap>::from(&map);
    assert!(result.is_err());
}

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Description: Find all instantiations of CheckTransactionStateRequestHeader and check for the presence of required fields.

# Search for instantiations of the struct
rg --type rust 'CheckTransactionStateRequestHeader\s*{[^}]*}' -A 5

# Highlight instances missing the required fields
rg --type rust 'CheckTransactionStateRequestHeader\s*{[^}]*}' -A 5 | rg -v 'tran_state_table_offset|commit_log_offset'

#!/bin/bash
# Let's try a different approach to find struct instantiations
# First, find all Rust files that mention CheckTransactionStateRequestHeader
rg -l --type rust "CheckTransactionStateRequestHeader"

# Then look for actual usage patterns with proper escaping
rg --type rust "CheckTransactionStateRequestHeader\s*\{" -A 10