rocketmq-remoting/src/protocol/header/change_invisible_time_request_header.rs (4)

26-49: Documentation improvements needed for better maintainability.

While the struct definition is well-structured, some fields need better documentation:

1. The comment //startOffset popTime invisibleTime queueId needs clarification about its purpose
2. The invisible_time field should document its time unit (seconds/milliseconds)
3. The extra_info field's format and purpose should be documented

Consider adding documentation like this:

 #[derive(Serialize, Deserialize, Debug, Default, RequestHeaderCodec)]
 #[serde(rename_all = "camelCase")]
+/// Header for changing message visibility timeout in RocketMQ
 pub struct ChangeInvisibleTimeRequestHeader {
     #[required]
+    /// Consumer group name
     pub consumer_group: CheetahString,
 
     #[required]
+    /// Topic name
     pub topic: CheetahString,
 
     #[required]
+    /// Queue ID in the topic
     pub queue_id: i32,
 
-    //startOffset popTime invisibleTime queueId
     #[required]
+    /// Extra information in format: "{startOffset}_{popTime}_{invisibleTime}_{queueId}"
     pub extra_info: CheetahString,
 
     #[required]
+    /// Message offset in the queue
     pub offset: i64,
 
     #[required]
+    /// New visibility timeout in milliseconds
     pub invisible_time: i64,

---

51-65: Consider improving time format in Display implementation.

The current Display implementation is functional, but consider making the invisible_time more human-readable by formatting it as a duration.

 impl Display for ChangeInvisibleTimeRequestHeader {
     fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+        let duration = std::time::Duration::from_millis(self.invisible_time as u64);
         write!(
             f,
             "ChangeInvisibleTimeRequestHeader {{ consumer_group: {}, topic: {}, queue_id: {}, \
-             extra_info: {}, offset: {}, invisible_time: {} }}",
+             extra_info: {}, offset: {}, invisible_time: {}ms ({:?}) }}",
             self.consumer_group,
             self.topic,
             self.queue_id,
             self.extra_info,
             self.offset,
-            self.invisible_time
+            self.invisible_time,
+            duration
         )
     }
 }

---

67-142: Enhance test coverage with additional test cases.

While the current tests are good, consider adding these test cases for better coverage:

#[test]
fn test_required_fields_validation() {
    let json = r#"{"topic":"topic1","queueId":1}"#;
    let result: Result<ChangeInvisibleTimeRequestHeader, _> = serde_json::from_str(json);
    assert!(result.is_err(), "Should fail when required fields are missing");
}

#[test]
fn test_extra_info_format() {
    let header = ChangeInvisibleTimeRequestHeader {
        consumer_group: CheetahString::from("group1"),
        topic: CheetahString::from("topic1"),
        queue_id: 1,
        extra_info: CheetahString::from("1_2_3_4"), // valid format
        offset: 12345,
        invisible_time: 67890,
        topic_request_header: None,
    };
    // Test serialization with valid format
    let serialized = serde_json::to_string(&header).unwrap();
    assert!(serialized.contains(r#""extraInfo":"1_2_3_4""#));
}

#[test]
fn test_negative_values() {
    let header = ChangeInvisibleTimeRequestHeader {
        consumer_group: CheetahString::from("group1"),
        topic: CheetahString::from("topic1"),
        queue_id: -1, // negative queue_id
        extra_info: CheetahString::from("info"),
        offset: -12345, // negative offset
        invisible_time: -67890, // negative invisible_time
        topic_request_header: None,
    };
    // Verify serialization/deserialization works with negative values
    let serialized = serde_json::to_string(&header).unwrap();
    let deserialized: ChangeInvisibleTimeRequestHeader = serde_json::from_str(&serialized).unwrap();
    assert_eq!(deserialized.queue_id, -1);
    assert_eq!(deserialized.offset, -12345);
    assert_eq!(deserialized.invisible_time, -67890);
}

---

26-49: Consider adding value validation for numeric fields.

The current implementation allows negative values for queue_id, offset, and invisible_time. Consider adding validation to prevent potential runtime errors:

1. Add a custom deserialize implementation to validate values
2. Or implement a validate() method that consumers must call

Example implementation:

impl ChangeInvisibleTimeRequestHeader {
    /// Validates the header fields
    /// Returns Ok(()) if valid, Err with description if invalid
    pub fn validate(&self) -> Result<(), String> {
        if self.queue_id < 0 {
            return Err("queue_id must be non-negative".to_string());
        }
        if self.invisible_time <= 0 {
            return Err("invisible_time must be positive".to_string());
        }
        // Validate extra_info format
        let parts: Vec<&str> = self.extra_info.split('_').collect();
        if parts.len() != 4 {
            return Err("extra_info must be in format: startOffset_popTime_invisibleTime_queueId".to_string());
        }
        Ok(())
    }
}

Configuration used: .coderabbit.yaml
Review profile: CHILL