uesleilima commented on code in PR #2373:
URL: https://github.com/apache/age/pull/2373#discussion_r3041586553
##########
drivers/python/test_age_py.py:
##########
@@ -28,6 +34,72 @@
TEST_GRAPH_NAME = "test_graph"
+class TestBuildCypher(unittest.TestCase):
+ """Unit tests for buildCypher() and _validate_column() — no DB required."""
+
+ def test_simple_column(self):
+ result = buildCypher("g", "MATCH (n) RETURN n", ["n"])
+ self.assertIn('"n" agtype', result)
+
+ def test_column_with_type(self):
+ result = buildCypher("g", "MATCH (n) RETURN n", ["n agtype"])
+ self.assertIn('"n" agtype', result)
Review Comment:
Fixed — updated all `test_security.py` assertions to expect double-quoted
column names (`'"v" agtype'`, `'"n" agtype'`, etc.). CI should pass now.
##########
drivers/python/age/age.py:
##########
@@ -208,14 +208,18 @@ def _validate_column(col: str) -> str:
name, type_name = parts
validate_identifier(name, "Column name")
validate_identifier(type_name, "Column type")
- return f"{name} {type_name}"
+ # Only the column name is double-quoted. The type name is left
+ # unquoted because PostgreSQL type names in column definitions
+ # are case-insensitive identifiers; double-quoting them would
+ # force exact-case matching and break user-defined type lookup.
Review Comment:
Fixed — reworded the comment to clarify case-folding semantics: "The type
name is left unquoted so PostgreSQL applies its default identifier folding for
type names in column definitions. Double-quoting would make the type name
case-sensitive and could change type resolution in surprising ways for
user-defined types."
##########
drivers/python/age/age.py:
##########
@@ -208,14 +208,18 @@ def _validate_column(col: str) -> str:
name, type_name = parts
validate_identifier(name, "Column name")
validate_identifier(type_name, "Column type")
- return f"{name} {type_name}"
+ # Only the column name is double-quoted. The type name is left
+ # unquoted because PostgreSQL type names in column definitions
+ # are case-insensitive identifiers; double-quoting them would
+ # force exact-case matching and break user-defined type lookup.
+ return f'"{name}" {type_name}'
else:
validate_identifier(col, "Column name")
- return f"{col} agtype"
+ return f'"{col}" agtype'
Review Comment:
This is inherent to the fix — previously, unquoted reserved words like
`count` caused SQL parse errors, which is worse than the case-folding change.
The `validate_identifier()` regex restricts names to `[A-Za-z_][A-Za-z0-9_]*`,
and in practice AGE column aliases are lowercase. The trade-off
(case-sensitivity for reserved-word safety) is the correct one.
##########
drivers/python/age/age.py:
##########
@@ -225,16 +229,18 @@ def buildCypher(graphName:str, cypherStmt:str,
columns:list) ->str:
if validated:
columnExp.append(validated)
else:
- columnExp.append('v agtype')
+ columnExp.append('"v" agtype')
Review Comment:
The default column already goes through `_validate_column` logic via the
`else` branch in `buildCypher` which hardcodes `'"v" agtype'`. Using
`_validate_column('v')` would work too but adds a function call for a static
value — keeping it inline is consistent with the existing pattern.
--
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]
