SAY-5 commented on code in PR #2425:
URL: https://github.com/apache/age/pull/2425#discussion_r3279670863
##########
drivers/python/age/builder.py:
##########
@@ -107,7 +107,12 @@ def visitAgValue(self, ctx:AgtypeParser.AgValueContext):
# Visit a parse tree produced by AgtypeParser#StringValue.
def visitStringValue(self, ctx:AgtypeParser.StringValueContext):
- return ctx.STRING().getText().strip('"')
+ # The STRING token always has surrounding '"' delimiters per the
+ # Agtype grammar. str.strip('"') would also drop any '"' characters
+ # that are part of the actual data when the value starts or ends
+ # with an escaped quote, e.g. '"foo \\"bar\\""' -> 'foo \\"bar\\',
+ # so trim exactly the first and last character instead.
+ return ctx.STRING().getText()[1:-1]
Review Comment:
Centralized into a small `_stripStringDelimiters` helper.
##########
drivers/python/age/builder.py:
##########
@@ -182,7 +187,8 @@ def visitPair(self, ctx:AgtypeParser.PairContext):
raise AGTypeError(ctx.getText(), "Missing key in object pair")
if agValNode is None:
raise AGTypeError(ctx.getText(), "Missing value in object pair")
- return (strNode.getText().strip('"') , agValNode)
+ # See visitStringValue() for why we slice instead of using strip('"').
+ return (strNode.getText()[1:-1] , agValNode)
Review Comment:
Both call sites now use `_stripStringDelimiters`.
##########
drivers/python/age/builder.py:
##########
@@ -182,7 +187,8 @@ def visitPair(self, ctx:AgtypeParser.PairContext):
raise AGTypeError(ctx.getText(), "Missing key in object pair")
if agValNode is None:
raise AGTypeError(ctx.getText(), "Missing value in object pair")
- return (strNode.getText().strip('"') , agValNode)
+ # See visitStringValue() for why we slice instead of using strip('"').
+ return (strNode.getText()[1:-1] , agValNode)
Review Comment:
Removed the stray space.
##########
drivers/python/test_agtypes.py:
##########
@@ -245,6 +245,20 @@ def test_array_of_mixed_types(self):
self.assertEqual(result[4], [1, 2, 3])
self.assertEqual(result[5], {"key": "val"})
+ def test_string_value_preserves_inner_quotes(self):
+ """Issue #2418: visitStringValue must remove only the outer quote
+ delimiters, not every '"' on either side, otherwise values that end
+ with an escaped quote (e.g. '"foo \\"bar\\""') lose data."""
Review Comment:
Reformatted to summary + body + closing quotes on their own line.
--
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: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
