tcolo commented on PR #2373: URL: https://github.com/apache/age/pull/2373#issuecomment-4189377389
@uesleilima many thanks for this. First check with Claude code ## Code Review: [apache/age#2373](https://github.com/apache/age/pull/2373) — `fix(python-driver): quote-escape column aliases in buildCypher()` ### Summary A minimal, well-targeted fix for a real crash ([#2370](https://github.com/apache/age/issues/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. -- 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]
