Repository navigation
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.
Contributor
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.
This branch has not been deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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: