uesleilima commented on code in PR #2372:
URL: https://github.com/apache/age/pull/2372#discussion_r3041586190
##########
drivers/python/age/builder.py:
##########
@@ -171,38 +195,49 @@ def visitArray(self, ctx:AgtypeParser.ArrayContext):
return li
def handleAnnotatedValue(self, anno:str, ctx:ParserRuleContext):
+ # Each branch below constructs a model object (Vertex, Edge, Path)
+ # and populates it from the parsed dict/list. If a type check
+ # fails (e.g. the parsed value is not a dict), AGTypeError is
+ # raised and the partially-constructed object is discarded — no
+ # cleanup is needed because the caller propagates the exception.
if anno == "numeric":
return Decimal(ctx.getText())
elif anno == "vertex":
- dict = ctx.accept(self)
- vid = dict["id"]
+ d = ctx.accept(self)
+ if not isinstance(d, dict):
+ raise AGTypeError(str(ctx.getText()), "Expected dict for
vertex, got " + type(d).__name__)
+ vid = d.get("id")
vertex = None
- if self.vertexCache != None and vid in self.vertexCache :
+ if self.vertexCache is not None and vid in self.vertexCache:
vertex = self.vertexCache[vid]
else:
vertex = Vertex()
- vertex.id = dict["id"]
- vertex.label = dict["label"]
- vertex.properties = dict["properties"]
+ vertex.id = d.get("id")
+ vertex.label = d.get("label")
+ vertex.properties = d.get("properties")
- if self.vertexCache != None:
+ if self.vertexCache is not None:
self.vertexCache[vid] = vertex
return vertex
elif anno == "edge":
edge = Edge()
- dict = ctx.accept(self)
- edge.id = dict["id"]
- edge.label = dict["label"]
- edge.end_id = dict["end_id"]
- edge.start_id = dict["start_id"]
- edge.properties = dict["properties"]
+ d = ctx.accept(self)
+ if not isinstance(d, dict):
+ raise AGTypeError(str(ctx.getText()), "Expected dict for edge,
got " + type(d).__name__)
+ edge.id = d.get("id")
+ edge.label = d.get("label")
+ edge.end_id = d.get("end_id")
+ edge.start_id = d.get("start_id")
+ edge.properties = d.get("properties")
Review Comment:
Fixed — same treatment for edge: `d.get("properties") or {}` ensures
properties is always a dict.
##########
drivers/python/test_agtypes.py:
##########
@@ -127,6 +127,158 @@ def test_path(self):
self.assertEqual(vertexEnd.label, "Person")
self.assertEqual(vertexEnd["name"], "Joe")
+ def test_vertex_large_array_properties(self):
+ """Issue #2367: Parser should handle vertices with large array
properties."""
+ vertexExp = (
+ '{"id": 1125899906842625, "label": "TestNode", '
+ '"properties": {"name": "test", '
+ '"tags": ["tag1", "tag2", "tag3", "tag4", "tag5", "tag6", "tag7", '
+ '"tag8", "tag9", "tag10", "tag11", "tag12"]}}::vertex'
+ )
+ vertex = self.parse(vertexExp)
+ self.assertEqual(vertex.id, 1125899906842625)
+ self.assertEqual(vertex.label, "TestNode")
+ self.assertEqual(vertex["name"], "test")
+ self.assertEqual(len(vertex["tags"]), 12)
+ self.assertEqual(vertex["tags"][0], "tag1")
+ self.assertEqual(vertex["tags"][11], "tag12")
+
+ def test_vertex_special_characters_in_properties(self):
+ """Issue #2367: Parser should handle properties with special
characters."""
+ vertexExp = (
+ '{"id": 1125899906842626, "label": "TestNode", '
+ '"properties": {"name": "test", '
+ '"description": "A long description with unicode chars"}}::vertex'
+ )
+ vertex = self.parse(vertexExp)
+ self.assertEqual(vertex.id, 1125899906842626)
+ self.assertEqual(vertex["name"], "test")
+ self.assertIn("unicode", vertex["description"])
+
+ def test_vertex_nested_properties(self):
+ """Issue #2367: Parser should handle deeply nested property
structures."""
+ vertexExp = (
+ '{"id": 1125899906842627, "label": "TestNode", '
+ '"properties": {"name": "test", '
+ '"metadata": {"level1": {"level2": {"level3":
"deep_value"}}}}}::vertex'
+ )
+ vertex = self.parse(vertexExp)
+ self.assertEqual(vertex.id, 1125899906842627)
+ self.assertEqual(vertex["name"], "test")
+ self.assertEqual(vertex["metadata"]["level1"]["level2"]["level3"],
"deep_value")
+
+ def test_vertex_empty_properties(self):
+ """Parser should handle vertices with empty properties dict."""
+ vertexExp = '{"id": 1125899906842628, "label": "EmptyNode",
"properties": {}}::vertex'
+ vertex = self.parse(vertexExp)
+ self.assertEqual(vertex.id, 1125899906842628)
+ self.assertEqual(vertex.label, "EmptyNode")
+ self.assertEqual(vertex.properties, {})
+
+ def test_vertex_null_property_values(self):
+ """Parser should handle vertices with null property values."""
+ vertexExp = (
+ '{"id": 1125899906842629, "label": "TestNode", '
+ '"properties": {"name": "test", "optional": null, "also_null":
null}}::vertex'
+ )
+ vertex = self.parse(vertexExp)
+ self.assertEqual(vertex["name"], "test")
+ self.assertIsNone(vertex["optional"])
+ self.assertIsNone(vertex["also_null"])
+
+ def test_edge_with_complex_properties(self):
+ """Parser should handle edges with complex property structures."""
+ edgeExp = (
+ '{"id": 2533274790396577, "label": "HAS_RELATION", '
+ '"end_id": 1125899906842625, "start_id": 1125899906842626, '
+ '"properties": {"weight": 3, "tags": ["a", "b", "c"], "active":
true}}::edge'
+ )
+ edge = self.parse(edgeExp)
+ self.assertEqual(edge.id, 2533274790396577)
+ self.assertEqual(edge.label, "HAS_RELATION")
+ self.assertEqual(edge.start_id, 1125899906842626)
+ self.assertEqual(edge.end_id, 1125899906842625)
+ self.assertEqual(edge["weight"], 3)
+ self.assertEqual(edge["tags"], ["a", "b", "c"])
+ self.assertEqual(edge["active"], True)
+
+ def test_path_with_multiple_edges(self):
+ """Parser should handle paths with multiple edges and complex
properties."""
+ pathExp = (
+ '[{"id": 1, "label": "A", "properties": {"name":
"start"}}::vertex, '
+ '{"id": 10, "label": "r1", "end_id": 2, "start_id": 1,
"properties": {"w": 1}}::edge, '
+ '{"id": 2, "label": "B", "properties": {"name":
"middle"}}::vertex, '
+ '{"id": 11, "label": "r2", "end_id": 3, "start_id": 2,
"properties": {"w": 2}}::edge, '
+ '{"id": 3, "label": "C", "properties": {"name":
"end"}}::vertex]::path'
+ )
+ path = self.parse(pathExp)
+ self.assertEqual(len(path), 5)
+ self.assertEqual(path[0]["name"], "start")
+ self.assertEqual(path[2]["name"], "middle")
+ self.assertEqual(path[4]["name"], "end")
+
+ def test_empty_input(self):
+ """Parser should handle empty/null input gracefully."""
+ self.assertIsNone(self.parse(''))
+ self.assertIsNone(self.parse(None))
+
+ def test_array_of_mixed_types(self):
+ """Parser should handle arrays with mixed types including nested
arrays."""
+ arrStr = '["str", 42, true, null, [1, 2, 3], {"key": "val"}]'
+ result = self.parse(arrStr)
+ self.assertEqual(result[0], "str")
+ self.assertEqual(result[1], 42)
+ self.assertEqual(result[2], True)
+ self.assertIsNone(result[3])
+ self.assertEqual(result[4], [1, 2, 3])
+ self.assertEqual(result[5], {"key": "val"})
+
+ def test_malformed_vertex_raises_agtypeerror_or_recovers(self):
+ """Issue #2367: Malformed agtype must raise AGTypeError or recover
gracefully."""
+ from age.exceptions import AGTypeError
+
+ malformed_inputs = [
+ '{"id": 1, "label":}::vertex',
+ '{"id": 1, "label": "X", "properties": {}::vertex',
+ '{::vertex',
+ '{"id": 1, "label": "X", "properties": {"key":}}::vertex',
+ ]
+ for inp in malformed_inputs:
+ try:
+ result = self.parse(inp)
+ # If the parser recovered, the result should be a usable
+ # value (None, dict, Vertex, etc.) — not a crash.
+ self.assertNotIsInstance(result, type(NotImplemented))
Review Comment:
Fixed — replaced with a structural check: `result is None or
isinstance(result, (dict, list, tuple)) or hasattr(result, "__dict__")`. This
validates recovery produces a usable Python value.
##########
drivers/python/age/builder.py:
##########
@@ -171,38 +195,49 @@ def visitArray(self, ctx:AgtypeParser.ArrayContext):
return li
def handleAnnotatedValue(self, anno:str, ctx:ParserRuleContext):
+ # Each branch below constructs a model object (Vertex, Edge, Path)
+ # and populates it from the parsed dict/list. If a type check
+ # fails (e.g. the parsed value is not a dict), AGTypeError is
+ # raised and the partially-constructed object is discarded — no
+ # cleanup is needed because the caller propagates the exception.
if anno == "numeric":
return Decimal(ctx.getText())
elif anno == "vertex":
- dict = ctx.accept(self)
- vid = dict["id"]
+ d = ctx.accept(self)
+ if not isinstance(d, dict):
+ raise AGTypeError(str(ctx.getText()), "Expected dict for
vertex, got " + type(d).__name__)
+ vid = d.get("id")
vertex = None
- if self.vertexCache != None and vid in self.vertexCache :
+ if self.vertexCache is not None and vid in self.vertexCache:
vertex = self.vertexCache[vid]
else:
vertex = Vertex()
- vertex.id = dict["id"]
- vertex.label = dict["label"]
- vertex.properties = dict["properties"]
+ vertex.id = d.get("id")
+ vertex.label = d.get("label")
+ vertex.properties = d.get("properties")
- if self.vertexCache != None:
+ if self.vertexCache is not None:
self.vertexCache[vid] = vertex
Review Comment:
Fixed — vertex `properties` now defaults to `{}` via `d.get("properties") or
{}`, preventing `__getitem__` crashes when properties are missing from the
parsed dict.
##########
drivers/python/test_agtypes.py:
##########
@@ -127,6 +127,158 @@ def test_path(self):
self.assertEqual(vertexEnd.label, "Person")
self.assertEqual(vertexEnd["name"], "Joe")
+ def test_vertex_large_array_properties(self):
+ """Issue #2367: Parser should handle vertices with large array
properties."""
+ vertexExp = (
+ '{"id": 1125899906842625, "label": "TestNode", '
+ '"properties": {"name": "test", '
+ '"tags": ["tag1", "tag2", "tag3", "tag4", "tag5", "tag6", "tag7", '
+ '"tag8", "tag9", "tag10", "tag11", "tag12"]}}::vertex'
+ )
+ vertex = self.parse(vertexExp)
+ self.assertEqual(vertex.id, 1125899906842625)
+ self.assertEqual(vertex.label, "TestNode")
+ self.assertEqual(vertex["name"], "test")
+ self.assertEqual(len(vertex["tags"]), 12)
+ self.assertEqual(vertex["tags"][0], "tag1")
+ self.assertEqual(vertex["tags"][11], "tag12")
+
+ def test_vertex_special_characters_in_properties(self):
+ """Issue #2367: Parser should handle properties with special
characters."""
+ vertexExp = (
+ '{"id": 1125899906842626, "label": "TestNode", '
+ '"properties": {"name": "test", '
+ '"description": "A long description with unicode chars"}}::vertex'
+ )
+ vertex = self.parse(vertexExp)
+ self.assertEqual(vertex.id, 1125899906842626)
+ self.assertEqual(vertex["name"], "test")
+ self.assertIn("unicode", vertex["description"])
+
+ def test_vertex_nested_properties(self):
+ """Issue #2367: Parser should handle deeply nested property
structures."""
+ vertexExp = (
+ '{"id": 1125899906842627, "label": "TestNode", '
+ '"properties": {"name": "test", '
+ '"metadata": {"level1": {"level2": {"level3":
"deep_value"}}}}}::vertex'
+ )
+ vertex = self.parse(vertexExp)
+ self.assertEqual(vertex.id, 1125899906842627)
+ self.assertEqual(vertex["name"], "test")
+ self.assertEqual(vertex["metadata"]["level1"]["level2"]["level3"],
"deep_value")
+
+ def test_vertex_empty_properties(self):
+ """Parser should handle vertices with empty properties dict."""
+ vertexExp = '{"id": 1125899906842628, "label": "EmptyNode",
"properties": {}}::vertex'
+ vertex = self.parse(vertexExp)
+ self.assertEqual(vertex.id, 1125899906842628)
+ self.assertEqual(vertex.label, "EmptyNode")
+ self.assertEqual(vertex.properties, {})
+
+ def test_vertex_null_property_values(self):
+ """Parser should handle vertices with null property values."""
+ vertexExp = (
+ '{"id": 1125899906842629, "label": "TestNode", '
+ '"properties": {"name": "test", "optional": null, "also_null":
null}}::vertex'
+ )
+ vertex = self.parse(vertexExp)
+ self.assertEqual(vertex["name"], "test")
+ self.assertIsNone(vertex["optional"])
+ self.assertIsNone(vertex["also_null"])
+
+ def test_edge_with_complex_properties(self):
+ """Parser should handle edges with complex property structures."""
+ edgeExp = (
+ '{"id": 2533274790396577, "label": "HAS_RELATION", '
+ '"end_id": 1125899906842625, "start_id": 1125899906842626, '
+ '"properties": {"weight": 3, "tags": ["a", "b", "c"], "active":
true}}::edge'
+ )
+ edge = self.parse(edgeExp)
+ self.assertEqual(edge.id, 2533274790396577)
+ self.assertEqual(edge.label, "HAS_RELATION")
+ self.assertEqual(edge.start_id, 1125899906842626)
+ self.assertEqual(edge.end_id, 1125899906842625)
+ self.assertEqual(edge["weight"], 3)
+ self.assertEqual(edge["tags"], ["a", "b", "c"])
+ self.assertEqual(edge["active"], True)
+
+ def test_path_with_multiple_edges(self):
+ """Parser should handle paths with multiple edges and complex
properties."""
+ pathExp = (
+ '[{"id": 1, "label": "A", "properties": {"name":
"start"}}::vertex, '
+ '{"id": 10, "label": "r1", "end_id": 2, "start_id": 1,
"properties": {"w": 1}}::edge, '
+ '{"id": 2, "label": "B", "properties": {"name":
"middle"}}::vertex, '
+ '{"id": 11, "label": "r2", "end_id": 3, "start_id": 2,
"properties": {"w": 2}}::edge, '
+ '{"id": 3, "label": "C", "properties": {"name":
"end"}}::vertex]::path'
+ )
+ path = self.parse(pathExp)
+ self.assertEqual(len(path), 5)
+ self.assertEqual(path[0]["name"], "start")
+ self.assertEqual(path[2]["name"], "middle")
+ self.assertEqual(path[4]["name"], "end")
+
+ def test_empty_input(self):
+ """Parser should handle empty/null input gracefully."""
+ self.assertIsNone(self.parse(''))
+ self.assertIsNone(self.parse(None))
+
+ def test_array_of_mixed_types(self):
+ """Parser should handle arrays with mixed types including nested
arrays."""
+ arrStr = '["str", 42, true, null, [1, 2, 3], {"key": "val"}]'
+ result = self.parse(arrStr)
+ self.assertEqual(result[0], "str")
+ self.assertEqual(result[1], 42)
+ self.assertEqual(result[2], True)
+ self.assertIsNone(result[3])
+ self.assertEqual(result[4], [1, 2, 3])
+ self.assertEqual(result[5], {"key": "val"})
+
+ def test_malformed_vertex_raises_agtypeerror_or_recovers(self):
+ """Issue #2367: Malformed agtype must raise AGTypeError or recover
gracefully."""
+ from age.exceptions import AGTypeError
+
+ malformed_inputs = [
+ '{"id": 1, "label":}::vertex',
+ '{"id": 1, "label": "X", "properties": {}::vertex',
+ '{::vertex',
+ '{"id": 1, "label": "X", "properties": {"key":}}::vertex',
+ ]
+ for inp in malformed_inputs:
+ try:
+ result = self.parse(inp)
+ # If the parser recovered, the result should be a usable
+ # value (None, dict, Vertex, etc.) — not a crash.
+ self.assertNotIsInstance(result, type(NotImplemented))
+ except AGTypeError:
+ pass # expected
+ except AttributeError:
+ self.fail(
+ f"Malformed input raised AttributeError instead of "
+ f"AGTypeError: {inp}"
+ )
+
+ def test_truncated_agtype_raises_agtypeerror(self):
+ """Issue #2367: Truncated agtype must raise AGTypeError, never
AttributeError."""
+ from age.exceptions import AGTypeError
+
+ truncated_inputs = [
+ '{"id": 1, "label": "X", "properties": {"name": "te',
+ '{"id": 1, "label": "X"',
+ '[{"id": 1}::vertex, {"id": 2',
+ ]
+ for inp in truncated_inputs:
+ try:
+ result = self.parse(inp)
+ # Recovery is acceptable for truncated input
+ self.assertIsNotNone(result)
+ except AGTypeError:
Review Comment:
Fixed — renamed to `test_truncated_agtype_does_not_crash` and updated
docstring to: "Truncated agtype must raise AGTypeError or recover, never
AttributeError." Matches the actual assertion behavior.
--
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]
