[ISSUE #1416]🐛Fix TopicRouteData Serialize and Deserialize field name error #1419
Conversation
WalkthroughThe changes in this pull request involve modifications to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
|
🔊@mxsm 🚀Thanks for your contribution 🎉. CodeRabbit(AI) will review your code first 🔥 |
rocketmq-rust-bot
added
AI review first
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
rocketmq-remoting/src/protocol/route/topic_route_data.rs(2 hunks)
🔇 Additional comments (3)
rocketmq-remoting/src/protocol/route/topic_route_data.rs (3)
37-38: LGTM! Field rename fixes the serialization issue.
The change correctly uses serde's rename attribute to fix the JSON field name while maintaining Rust naming conventions in the code. This addresses the serialization/deserialization field name error mentioned in issue #1416.
82-146: LGTM! Comprehensive test coverage.
The test suite provides excellent coverage:
- Verifies default values
- Tests initialization with specific values
- Confirms correct JSON serialization with the new field name
- Validates the topic route data change detection logic
Also applies to: 179-201
37-38: Verify no hardcoded field names in the codebase.
Let's ensure there are no hardcoded references to either the old or new field name in the codebase.
✅ Verification successful
Field rename is properly handled with serde attribute
The field rename from topic_queue_mapping_by_broker to topicQueueMappingInfo is correctly handled using the #[serde(rename = "topicQueueMappingInfo")] attribute. The search results show that:
- The Rust struct field uses snake_case
topic_queue_mapping_by_brokerinternally - The JSON serialization name
topicQueueMappingInfois properly specified via serde attribute - All references to this field in tests and code use the correct Rust field name
- Other occurrences of "topicQueueMappingInfo" are either:
- Part of the
TopicQueueMappingInfotype name - Other serde renames in different structs
- Test assertions verifying correct JSON serialization
- Part of the
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for hardcoded field names in the codebase
echo "Searching for old field name..."
rg -i "TopicQueueMappingInfo" --type rust
echo "Searching for new field name..."
rg -i "topicQueueMappingInfo" --type rust
Length of output: 8077
| /* #[test] | ||
| fn deserialize_topic_route_data() { | ||
| let json = r#"{ | ||
| "orderTopicConf": "conf", | ||
| "queueDatas": [{}], | ||
| "brokerDatas": [{}], | ||
| "filterServerTable": {"key": ["value"]}, | ||
| "topicQueueMappingInfo": {"broker": {}} | ||
| }"#; | ||
| let deserialized: TopicRouteData = serde_json::from_str(json).unwrap(); | ||
| assert_eq!( | ||
| deserialized.order_topic_conf, | ||
| Some(CheetahString::from("conf")) | ||
| ); | ||
| assert_eq!(deserialized.queue_datas.len(), 1); | ||
| assert_eq!(deserialized.broker_datas.len(), 1); | ||
| assert_eq!(deserialized.filter_server_table.len(), 1); | ||
| assert!(deserialized.topic_queue_mapping_by_broker.is_some()); | ||
| }*/ | ||
|
|
||
| /* #[test] | ||
| fn deserialize_topic_route_data_missing_optional_fields() { | ||
| let json = r#"{}"#; | ||
| let deserialized: TopicRouteData = serde_json::from_str(json).unwrap(); | ||
| assert!(deserialized.order_topic_conf.is_none()); | ||
| assert!(deserialized.queue_datas.is_empty()); | ||
| assert!(deserialized.broker_datas.is_empty()); | ||
| assert!(deserialized.filter_server_table.is_empty()); | ||
| assert!(deserialized.topic_queue_mapping_by_broker.is_none()); | ||
| }*/ |
There was a problem hiding this comment.
Uncomment and fix the deserialization tests.
Since this PR fixes a serialization/deserialization field name error, it's crucial to have the deserialization tests enabled and passing. These tests would verify that the fix works in both directions.
Please uncomment and update these tests to work with the corrected field name. Let me know if you need help with this.
rocketmq-rust-bot
added
approved
Which Issue(s) This PR Fixes(Closes)
Fixes #1416
Brief Description
How Did You Test This Change?
Summary by CodeRabbit
Bug Fixes
TopicRouteDatastruct to ensure correct data representation.Tests
TopicRouteDatastruct, covering initialization, serialization, and method behavior.