[GitHub] [shardingsphere] sandynz commented on a diff in pull request #24134: Refactor `NoneYamlTupleProcessor`

["sandynz (via GitHub)" <gi...@apache.org>]

sandynz commented on code in PR #24134:
URL: https://github.com/apache/shardingsphere/pull/24134#discussion_r1104089234


##########
features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/yaml/engine/representer/processor/NoneYamlTupleProcessor.java:
##########
@@ -37,11 +37,11 @@ public String getTupleName() {
     
     @Override
     public NodeTuple process(final NodeTuple nodeTuple) {
-        return (isNullNode(nodeTuple.getValueNode()) || isEmptyMappingNode(nodeTuple.getValueNode())) ? null : processNoneTuple(nodeTuple);
+        return (isNullNode(nodeTuple.getKeyNode(), nodeTuple.getValueNode()) || isEmptyMappingNode(nodeTuple.getValueNode())) ? processNoneTuple(nodeTuple) : null;
     }
     
-    private boolean isNullNode(final Node valueNode) {
-        return Tag.NULL.equals(valueNode.getTag());
+    private boolean isNullNode(final Node keyNode, final Node valueNode) {
+        return !"none".equalsIgnoreCase(((ScalarNode) keyNode).getValue()) && Tag.NULL.equals(valueNode.getTag());

Review Comment:
   If yaml text contains `none: 'xxx'`, then node is handled as null. Could we add more accurate check here?



##########
features/sharding/core/src/main/java/org/apache/shardingsphere/sharding/yaml/engine/representer/processor/NoneYamlTupleProcessor.java:
##########
@@ -37,11 +37,11 @@ public String getTupleName() {
     
     @Override
     public NodeTuple process(final NodeTuple nodeTuple) {
-        return (isNullNode(nodeTuple.getValueNode()) || isEmptyMappingNode(nodeTuple.getValueNode())) ? null : processNoneTuple(nodeTuple);
+        return (isNullNode(nodeTuple.getKeyNode(), nodeTuple.getValueNode()) || isEmptyMappingNode(nodeTuple.getValueNode())) ? processNoneTuple(nodeTuple) : null;

Review Comment:
   Since `processNoneTuple` invocation is reversed with `null`, could we add more unit test for cases fit `isNullNode` or `isEmptyMappingNode`?



-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

To unsubscribe, e-mail: notifications-unsubscribe@shardingsphere.apache.org

For queries about this service, please contact Infrastructure at:
users@infra.apache.org