Copilot commented on code in PR #2372:
URL: https://github.com/apache/age/pull/2372#discussion_r3039028834
##########
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:
In `handleAnnotatedValue()` (vertex branch), switching from required key
access to `d.get(...)` can silently construct a `Vertex` with `id=None` and/or
`properties=None` when the parsed dict is missing keys (e.g., after parser
error recovery). This can also poison `vertexCache` by storing under the `None`
key and later cause runtime errors (e.g., `Vertex.__getitem__` assumes
`properties` is a dict). Consider validating required fields (`id`, `label`,
`properties`) and either raising `AGTypeError` when they’re missing/invalid or
defaulting `properties` to `{}` before constructing/caching the vertex.
##########
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:
The docstring says truncated agtype “must raise AGTypeError”, but the test
body explicitly allows successful recovery (`assertIsNotNone(result)`). Update
the docstring (or the assertions) so the test’s contract is unambiguous.
##########
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:
This assertion doesn’t meaningfully validate “recovered” output:
`assertNotIsInstance(result, type(NotImplemented))` will pass for essentially
any normal parse result (including `None`). To make the test effective, assert
an expected set of types/shape (e.g., `None` or a dict/list/model object) or
assert specific properties when recovery occurs, while still ensuring no
`AttributeError` is raised.
```suggestion
# parsed value with an expected shape (None, container, or
# model-like object) — not just any arbitrary object.
self.assertTrue(
result is None
or isinstance(result, (dict, list, tuple))
or hasattr(result, "__dict__")
or hasattr(result, "__getitem__"),
f"Malformed input recovered to unexpected result type "
f"{type(result).__name__}: {inp}"
)
```
##########
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:
Similarly for the `edge` branch: using `d.get(...)` can create an `Edge`
with missing identifiers or `properties=None`, which can later crash when the
edge is accessed as a mapping (since `Edge.__getitem__` assumes `properties` is
a dict). Recommend enforcing required keys (`id`, `label`, `start_id`,
`end_id`, `properties`) and/or defaulting `properties` to `{}`; if the parsed
value is incomplete due to recovery, raising `AGTypeError` is safer than
returning a partially-initialized model object.
--
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]
