Repository navigation
fix(django-google-spanner): harden identifier quoting - #19069
Conversation
Previously, Django's identifier quoting delegated directly to the DB-API driver's name-escaping helper, which is meant for dot-qualified SQL paths and unconditionally quotes all identifiers in newer versions. That caused single identifiers containing dots to be split into separate parts and broke compatibility with existing Django SQL expectations. This change implements single-identifier quoting directly in the Django backend: - Quotes identifiers as a single unit without splitting on dots. - Escapes backslashes, backticks (using GoogleSQL hex escapes so both Spanner DDL and downstream SQL comment/statement parsing keep the identifier intact), and ASCII control characters without rejecting identifiers client-side. - Preserves already validly quoted single identifiers idempotently while re-escaping malformed inputs and injection payloads. - Keeps unreserved regular ASCII identifiers unquoted for now, with a TODO to switch to unconditional quoting in a follow-up change. - Expands unit tests to cover dotted identifiers, control characters, malformed quotes, idempotency, and SQL injection payloads.
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request improves the identifier quoting mechanism in django_spanner/operations.py by rewriting quote_name to safely escape and quote table, index, and column names using robust regular expressions and an escape translation table. This prevents SQL injection and ensures compatibility with GoogleSQL. Additionally, a comprehensive set of unit tests has been added in test_operations.py to cover various edge cases, including reserved keywords, special characters, and SQL injection payloads. There are no review comments, and I have no feedback to provide.
chalmerlowe
left a comment
chalmerlowe
left a comment
There was a problem hiding this comment.
LGTM, pending resolution of Blocker and typehinting.
Leave the preference up to your discretion.
| return escape_name(name) | ||
| if _QUOTED_IDENTIFIER_RE.match(name): | ||
| return name | ||
| # TODO: Unconditionally quote all identifiers in a follow-up PR (after |
There was a problem hiding this comment.
BLOCKER
TODO comments should include a parenthesized issue reference (TODO(issue): ...) for traceability
| @@ -106,28 +127,46 @@ def max_name_length(self): | |||
|
|
|||
| def quote_name(self, name): | |||
There was a problem hiding this comment.
| def quote_name(self, name): | |
| def quote_name(self, name: str) -> str: |
| with self.subTest(invalid=invalid), self.assertRaises(TypeError): | ||
| self.db_operations.quote_name(invalid) |
There was a problem hiding this comment.
PREFERENCE
Bare self.assertRaises(TypeError) only checks the exception type and would pass even if TypeError were raised unintentionally by an internal call (e.g., re.match(name) or name.translate(...)) or if the diagnostic message regressed. If we use self.assertRaisesRegex to pin the exact error message format (Identifier must be a string, got <type>)
| with self.subTest(invalid=invalid), self.assertRaises(TypeError): | |
| self.db_operations.quote_name(invalid) | |
| expected_msg = ( | |
| rf"^Identifier must be a string, got {type(invalid).__name__}$" | |
| ) | |
| with self.subTest(invalid=invalid), self.assertRaisesRegex( | |
| TypeError, expected_msg | |
| ): | |
| self.db_operations.quote_name(invalid) |
Previously, Django's identifier quoting delegated directly to the DB-API driver's name-escaping helper, which is meant for dot-qualified SQL paths and unconditionally quotes all identifiers in newer versions. That caused single identifiers containing dots to be split into separate parts and broke compatibility with existing Django SQL expectations.
This change implements single-identifier quoting directly in the Django backend: