Fixed the configMTree deserialisation bug for "root" database #16961
Conversation
|
There was a problem hiding this comment.
Pull request overview
This PR attempts to fix a deserialization bug in ConfigMTree where the loop would terminate prematurely when encountering a database named "root". The fix modifies the loop termination condition to check both the node name and type, and adds a test case for a database named "root".
Key Changes
- Modified the deserialization loop condition to check both name and node type instead of just name
- Updated test to include a database named "root" (using backticks:
root.\root``) - Removed outdated comments about internal mNode deserialization
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| ConfigMTree.java | Modified deserialization loop condition to check both node name ("root") and node type (INTERNAL_MNODE_TYPE) before terminating, and removed outdated comments |
| ConfigMTreeTest.java | Updated serialization test to include edge case of a database named "root" |
Comments suppressed due to low confidence (1)
iotdb-core/confignode/src/test/java/org/apache/iotdb/confignode/persistence/schema/ConfigMTreeTest.java:268
- The test should include a case with a path containing an internal node named "root" (e.g., "root.a.
root.b.sg") to verify that deserialization correctly handles escaped "root" names in the middle of a path, not just at the database level. Currently, the test only covers "root.root" which is a database named "root" directly under the tree root.
new PartialPath("root.`root`"),
new PartialPath("root.a.sg"),
new PartialPath("root.a.b.sg"),
new PartialPath("root.a.a.b.sg")
};
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| } | ||
|
|
||
| while (!PATH_ROOT.equals(name)) { | ||
| while (!PATH_ROOT.equals(name) || type != INTERNAL_MNODE_TYPE) { |
There was a problem hiding this comment.
The loop termination condition has a logical flaw. It will incorrectly stop when encountering any internal node named "root", not just the tree root. For example, if there's a path like "root.a.root.b.sg" where root is an escaped internal node name, the deserialization would terminate prematurely at that internal node.
The condition should ensure we've reached the actual tree root, which is the last node serialized. One approach is to check if the stack has accumulated all the tree content, or use a different marker to identify the tree root specifically.
| while (!PATH_ROOT.equals(name) || type != INTERNAL_MNODE_TYPE) { | |
| while (stack.size() != 1 || !PATH_ROOT.equals(name) || type != INTERNAL_MNODE_TYPE) { |
There was a problem hiding this comment.
The stack size cannot reflect whether the "root" is an internal node.... "root.a.root.b.sg" will cause the stack to be (sg, b, root, a, root), and the stack size always == 1. Besides, a pattern like this will not pass the partial path AST check.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #16961 +/- ##
=========================================
Coverage 39.32% 39.32%
Complexity 212 212
=========================================
Files 5053 5053
Lines 337216 337218 +2
Branches 42923 42923
=========================================
+ Hits 132603 132624 +21
+ Misses 204613 204594 -19 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
Description
As the title said.
This PR has:
for an unfamiliar reader.
for code coverage.
Key changed/added classes (or packages if there are too many classes) in this PR