fix(python-driver): quote-escape column aliases in buildCypher()

[uesleilima]

Summary

Fix execCypher() crashing when column aliases are PostgreSQL reserved words (#2370).

Problem

The buildCypher() function generates SQL like:

SELECT * FROM cypher(NULL,NULL) AS (count agtype);

When the column alias is a reserved word (count, order, type, group, select, etc.), PostgreSQL raises a parse error because the identifier is unquoted.

This commonly happens when Cypher RETURN clauses use aggregation functions:

cursor = age.execCypher(conn, "test_graph",
                        "MATCH (n:TestNode) RETURN count(n)",
                        cols=["count"])  # crashes

Fix

Always double-quote column names in _validate_column():

# Before
return f"{name} {type_name}"     # count agtype  → ERROR
# After
return f'"{name}" {type_name}'   # "count" agtype → OK

PostgreSQL always accepts double-quoted identifiers, so quoting unconditionally is safe for all names — no reserved word list needed. This is simpler and more maintainable than checking against a list of 60+ reserved words.

Generated SQL after fix:

SELECT * FROM cypher(NULL,NULL) AS ("count" agtype);

Tests

Added 11 new unit tests in TestBuildCypher class (no DB required):

• Simple column, column with type, multiple columns
• Reserved words: count, order, type, select, group
• Default column quoting
• Invalid column rejection
• _validate_column() direct quoting verification

Closes

• Closes Python driver: execCypher() should quote-escape column aliases that are PostgreSQL reserved words #2370

[tcolo]

@uesleilima many thanks for this. First check with Claude code

Code Review: apache/age#2373 — fix(python-driver): quote-escape column aliases in buildCypher()

Summary

A minimal, well-targeted fix for a real crash (#2370): column aliases matching PostgreSQL reserved words (e.g. count, order) caused parse errors. The solution double-quotes all column names unconditionally, which is safe and idiomatic PostgreSQL. Includes 11 new unit tests.

---

🔴 Critical Issues

None.

---

💡 Suggestions

#	File	Location	Suggestion	Category
1	age.py	_validate_column line 261	Type name is not quoted. return f'"{name}" {type_name}' only quotes the name, but type_name is also a user-provided identifier. While in practice AGE only uses agtype, it is inconsistent — if a caller passes v mytype, the type name is unquoted. Consider quoting it too, or documenting why it is intentionally left unquoted.	Correctness
2	age.py	buildCypher line 268	if graphName == None: should be if graphName is None: — pre-existing issue, but touched in this diff's context.	Style
3	test_age_py.py	test_reserved_word_count	The assertNotIn("(count agtype", result) check is narrowly crafted. It would miss a regression like ( count agtype (space after paren). A stronger assertion would be a regex like r'\bcount\s+agtype\b' outside of quotes.	Testing
4	test_age_py.py	TestBuildCypher imports	from age.age import buildCypher, _validate_column imports a private function (_validate_column is prefixed _). Testing private functions directly is acceptable here since the behavior is important, but worth a comment noting it's intentional.	Maintainability
5	age.py	buildCypher	No test covers the "name type" path where type_name contains a reserved-word-like string (e.g., "order agtype"). Adding one would verify the quoting is applied in the two-token branch as well.	Testing

---

✅ What Looks Good

• Minimal diff — only the three affected lines changed, no scope creep.
• Correct fix — unconditional double-quoting is the right approach; PostgreSQL always accepts double-quoted identifiers, and the identifier regex ^[A-Za-z_][A-Za-z0-9_]*$ already prevents injection of embedded quotes.
• Updated comment — the design note in buildCypher was proactively updated to document the quoting rationale.
• Solid test coverage — 11 unit tests with no DB dependency is exactly right for this kind of fix; the reserved-word cases are all explicitly called out.
• test_validate_column_quoting directly asserts the output format, making future regressions immediately obvious.

---

Verdict

Approve with minor suggestions. The fix is correct and safe. Suggestion #1 (unquoted type_name) is the only one worth a follow-up conversation — the rest are low-priority polish.

[tcolo]

Please check the comment I made

[Copilot]

Pull request overview

Fixes the Python driver’s buildCypher() SQL generation so column aliases that are PostgreSQL reserved words don’t cause parse errors, by always double-quoting the column identifier portion of each AS (...) column spec.

Changes:

• Update _validate_column() to always wrap validated column names in double quotes (while leaving type names unquoted).
• Quote the default column alias (v) in buildCypher() when cols is not provided.
• Add unit tests covering reserved-word aliases and direct _validate_column() quoting behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.

File	Description
drivers/python/age/age.py	Always double-quotes column aliases produced by _validate_column() / buildCypher() to avoid reserved-word parse errors.
drivers/python/test_age_py.py	Adds unit tests validating quoted output for common/reserved column aliases and invalid identifiers.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[Copilot]

This change will break existing Python tests that assert unquoted column specs (e.g., drivers/python/test_security.py currently expects v agtype / n agtype in buildCypher() output and _validate_column() return values). Update those assertions to match the new always-quoted behavior so the test suite stays consistent and CI doesn’t fail.

[Copilot]

The new always-quoted column output from buildCypher() / _validate_column() requires updating any in-repo callers/tests that compare generated SQL strings (e.g., security tests that look for v agtype or n agtype). Without updating those expectations, the Python test suite will fail.

Suggested change

	columnExp.append('"v" agtype')
	columnExp.append(_validate_column('v'))

[Copilot]

The rationale comment about not quoting type_name is a bit misleading: double-quoting doesn’t inherently “break user-defined type lookup”; it changes name-folding semantics (quoted identifiers become case-sensitive, unquoted fold to lower-case). Consider rewording to clarify that leaving the type unquoted preserves PostgreSQL’s default case-folding behavior and avoids surprising resolution changes.

Suggested change

	# 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.
	# Only the column name is double-quoted. 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.

[Copilot]

Always double-quoting column names changes PostgreSQL’s identifier folding semantics: previously an unquoted alias like V would be exposed as v (folded to lower-case), but now "V" will stay uppercase. If callers relied on the old lowercasing behavior for cols values with uppercase letters, this is a user-visible behavior change; consider normalizing name to lower-case before quoting or documenting this change.