rocketmq-common/src/common/topic.rs (2)

101-129: Consider moving error messages to module-level constants

The validation logic is well-optimized with CheetahString. However, the error messages could be moved to module-level constants for better reusability and maintenance.

Consider refactoring like this:

+ const BLANK_TOPIC_ERROR: &str = "The specified topic is blank.";
+ const ILLEGAL_CHARS_ERROR: &str = "The specified topic contains illegal characters, allowing only ^[%|a-zA-Z0-9_-]+$";

  pub fn validate_topic(topic: &str) -> ValidateTopicResult {
      if topic.trim().is_empty() {
-         const REMARK: &str = "The specified topic is blank.";
          return ValidateTopicResult {
              valid: false,
-             remark: CheetahString::from_static_str(REMARK),
+             remark: CheetahString::from_static_str(BLANK_TOPIC_ERROR),
          };
      }

      if Self::is_topic_or_group_illegal(topic) {
-         const REMARK: &str = "The specified topic contains illegal characters, allowing only ^[%|a-zA-Z0-9_-]+$";
          return ValidateTopicResult {
              valid: false,
-             remark: CheetahString::from_static_str(REMARK),
+             remark: CheetahString::from_static_str(ILLEGAL_CHARS_ERROR),
          };
      }

---

173-175: Add test coverage for set_remark method

The set_remark method is currently not covered by tests according to the code coverage report.

Would you like me to help generate a test case for the set_remark method? Here's a suggested test:

#[test]
fn test_set_remark() {
    let mut result = ValidateTopicResult {
        valid: true,
        remark: CheetahString::empty(),
    };
    let new_remark = CheetahString::from_static_str("test remark");
    result.set_remark(new_remark.clone());
    assert_eq!(result.remark(), &new_remark);
}

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 173-173: rocketmq-common/src/common/topic.rs#L173
Added line #L173 was not covered by tests

rocketmq-broker/src/processor/admin_broker_processor/topic_request_handler.rs (2)

90-90: Add test coverage for topic validation error paths

The static analysis indicates that the error paths for invalid topics are not covered by tests. These are critical paths that handle validation failures and should be tested.

Would you like me to help create unit tests for these scenarios? The tests should cover:

1. Invalid topic validation cases
2. System topic conflicts
3. Mixed message type validation

Example test structure:

#[test]
fn test_update_and_create_topic_validation_failures() {
    // Test invalid topic
    // Test system topic conflict
    // Test mixed message type
}

Also applies to: 216-216

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 90-90: rocketmq-broker/src/processor/admin_broker_processor/topic_request_handler.rs#L90
Added line #L90 was not covered by tests

---

89-91: Improve error handling consistency

The error handling patterns vary across different methods. Consider standardizing the approach:

1. Use constants for common error messages
2. Consistent formatting style for error messages
3. Structured error types for different validation failures

Consider creating an error module:

mod errors {
    pub const SYSTEM_TOPIC_CONFLICT: &str = "The topic[{}] is conflict with system topic.";
    pub const TOPIC_NOT_EXIST: &str = "The topic[{}] not exist.";
    // Add more error constants
}

This would improve maintainability and ensure consistency across error messages.

Also applies to: 215-217, 326-330, 402-406, 495-499, 583-587

🧰 Tools

🪛 GitHub Check: codecov/patch

[warning] 90-90: rocketmq-broker/src/processor/admin_broker_processor/topic_request_handler.rs#L90
Added line #L90 was not covered by tests

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Description: Check existing patterns for handling remarks in responses
# Look for other usages of set_remark to understand the common pattern

# Search for set_remark usage patterns
rg -A 2 'set_remark\(' 

# Search for ValidateTopicResult usage
ast-grep --pattern 'let result = TopicValidator::validate_topic($$$)'