FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

fix(sqlalchemy-spanner): escape names in reflection queries by Samin061 · Pull Request #18205 · googleapis/google-cloud-python · GitHub

/ google-cloud-python Public

fix(sqlalchemy-spanner): escape names in reflection queries #18205

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
Samin061 wants to merge 2 commits into
base: main
Choose a base branch
Could not load branches
Branch not found: {{ refName }}

Are you sure you want to change the base?

Some commits from the old base branch may be removed from the timeline, and old review comments may become outdated.
from
+120 −11
Conversation 9 Commits 2 Checks 10 Files changed 2
Open

fix(sqlalchemy-spanner): escape names in reflection queries #18205

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
0411049
fix(sqlalchemy-spanner): escape names in reflection queries
Samin061 Aug 24, 2026
aa4c445
fix(sqlalchemy-spanner): fail fast on non-string names and fix AND sp…
Samin061 Aug 27, 2026
File filter

Filter by extension

Filter by extension .py  (2) All 1 file type selected
Viewed files
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Unified
Split
Hide whitespace
Diff view
Unified
Split
Hide whitespace
61 changes: 50 additions & 11 deletions packages/sqlalchemy-spanner/google/cloud/sqlalchemy_spanner/sqlalchemy_spanner.py
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -76,6 +76,30 @@ def reset_connection(dbapi_conn, connection_record, reset_state=None):
OPERATORS[json_getitem_op] = operator_lookup["json_getitem_op"]


def _escape_sql_string_literal(value):
"""Escape a value for safe inclusion in a GoogleSQL string literal.

The reflection queries below build ``INFORMATION_SCHEMA`` predicates by
interpolating table, schema, view and sequence names into quoted string
literals. A name containing a quote (for example, one enumerated from a
shared or foreign database and fed back in during reflection) would
otherwise close the literal so the remainder is parsed as SQL. Escaping the
backslash, both quote characters and newlines keeps the name contained.
"""
if not isinstance(value, str):
raise spanner_dbapi.exceptions.ProgrammingError(
"Unsupported type for SQL string literal escaping: "
"{!r}".format(type(value).__name__)
)
return (
value.replace("\\", "\\\\")
.replace("'", "\\'")
.replace('"', '\\"')
.replace("\n", "\\n")
.replace("\r", "\\r")
)
Comment on lines +79 to +100

gemini-code-assist Bot Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

When _escape_sql_string_literal receives parameters of an unsupported type (such as None or non-string types), it should raise an error (e.g., ProgrammingError) instead of silently returning empty values or converting them. This ensures fail-fast behavior and prevents potential issues with missing parameter values in database operations.

Suggested change
def _escape_sql_string_literal(value):
"""Escape a value for safe inclusion in a GoogleSQL string literal.
The reflection queries below build ``INFORMATION_SCHEMA`` predicates by
interpolating table, schema, view and sequence names into quoted string
literals. A name containing a quote (for example, one enumerated from a
shared or foreign database and fed back in during reflection) would
otherwise close the literal so the remainder is parsed as SQL. Escaping the
backslash, both quote characters and newlines keeps the name contained.
"""
return (
value.replace("\\", "\\\\")
.replace("'", "\\'")
.replace('"', '\\"')
.replace("\n", "\\n")
.replace("\r", "\\r")
)
def _escape_sql_string_literal(value):
"""Escape a value for safe inclusion in a GoogleSQL string literal.
The reflection queries below build ``INFORMATION_SCHEMA`` predicates by
interpolating table, schema, view and sequence names into quoted string
literals. A name containing a quote (for example, one enumerated from a
shared or foreign database and fed back in during reflection) would
otherwise close the literal so the remainder is parsed as SQL. Escaping the
backslash, both quote characters and newlines keeps the name contained.
"""
if not isinstance(value, str):
from google.cloud.spanner_dbapi import ProgrammingError
raise ProgrammingError("Unsupported type for SQL string literal escaping.")
return (
value.replace("\\", "\\\\")
.replace("'", "\\'")
.replace('"', '\\"')
.replace("\n", "\\n")
.replace("\r", "\\r")
)
References
  1. When a function receives parameters of an unsupported type, it should raise an error (e.g., ProgrammingError) instead of silently returning empty values. This ensures fail-fast behavior and prevents potential issues with missing parameter values in database operations.

Samin061 Aug 27, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Added an isinstance check that raises spanner_dbapi.exceptions.ProgrammingError for non-string input, matching the existing error usage in this file. Test for None and int added too.


# PickleType that can be used with Spanner.
# Binary values are automatically encoded/decoded to/from base64.
# Usage:
Expand Down Expand Up @@ -972,7 +996,10 @@ def _get_table_filter_query(
table_filter_query = ""
if filter_names is not None:
for table_name in filter_names:
query = f"{info_schema_table}.table_name = '{table_name}'"
query = (
f"{info_schema_table}.table_name = "
f"'{_escape_sql_string_literal(table_name)}'"
)
if table_filter_query != "":
table_filter_query = table_filter_query + " OR " + query
else:
Expand Down Expand Up @@ -1104,7 +1131,10 @@ def get_view_definition(self, connection, view_name, schema=None, **kw):
SELECT view_definition
FROM information_schema.views
WHERE TABLE_SCHEMA='{schema_name}' AND TABLE_NAME='{view_name}'
""".format(schema_name=schema or "", view_name=view_name)
""".format(
schema_name=_escape_sql_string_literal(schema or ""),
view_name=_escape_sql_string_literal(view_name),
)

with connection.connection.database.snapshot() as snap:
rows = list(snap.execute_sql(sql))
Expand Down Expand Up @@ -1144,7 +1174,7 @@ def get_multi_columns(
"""
table_filter_query = self._get_table_filter_query(filter_names, "col", True)
schema_filter_query = " col.table_schema = '{schema}' AND ".format(
schema=schema or ""
schema=_escape_sql_string_literal(schema or "")
)
table_type_query = self._get_table_type_query(kind, True)

Expand Down Expand Up @@ -1273,7 +1303,7 @@ def get_multi_indexes(
"""
table_filter_query = self._get_table_filter_query(filter_names, "i", True)
schema_filter_query = " i.table_schema = '{schema}' AND ".format(
schema=schema or ""
schema=_escape_sql_string_literal(schema or "")
)
table_type_query = self._get_table_type_query(kind, True)

Expand Down Expand Up @@ -1414,7 +1444,7 @@ def get_multi_pk_constraint(
"""
table_filter_query = self._get_table_filter_query(filter_names, "tc", True)
schema_filter_query = " tc.table_schema = '{schema}' AND ".format(
schema=schema or ""
schema=_escape_sql_string_literal(schema or "")
)
table_type_query = self._get_table_type_query(kind, True)

Expand Down Expand Up @@ -1524,8 +1554,8 @@ def get_multi_foreign_keys(
The schema is ``None`` if no schema is provided.
"""
table_filter_query = self._get_table_filter_query(filter_names, "tc", True)
schema_filter_query = " tc.table_schema = '{schema}' AND".format(
schema=schema or ""
schema_filter_query = " tc.table_schema = '{schema}' AND ".format(
schema=_escape_sql_string_literal(schema or "")
)
table_type_query = self._get_table_type_query(kind, True)

Expand Down Expand Up @@ -1641,7 +1671,7 @@ def get_table_names(self, connection, schema=None, **kw):
SELECT table_name
FROM information_schema.tables
WHERE table_type = 'BASE TABLE' AND table_schema = '{schema}'
""".format(schema=schema or "")
""".format(schema=_escape_sql_string_literal(schema or ""))

table_names = []
with connection.connection.database.snapshot() as snap:
Expand Down Expand Up @@ -1677,7 +1707,10 @@ def get_unique_constraints(self, connection, table_name, schema=None, **kw):
AND tc.TABLE_SCHEMA="{table_schema}"
AND tc.CONSTRAINT_TYPE = "UNIQUE"
AND tc.CONSTRAINT_NAME IS NOT NULL
""".format(table_schema=schema or "", table_name=table_name)
""".format(
table_schema=_escape_sql_string_literal(schema or ""),
table_name=_escape_sql_string_literal(table_name),
)

cols = []
with connection.connection.database.snapshot() as snap:
Expand Down Expand Up @@ -1710,7 +1743,10 @@ def has_table(self, connection, table_name, schema=None, **kw):
FROM INFORMATION_SCHEMA.TABLES
WHERE TABLE_SCHEMA="{table_schema}" AND TABLE_NAME="{table_name}"
LIMIT 1
""".format(table_schema=schema or "", table_name=table_name)
""".format(
table_schema=_escape_sql_string_literal(schema or ""),
table_name=_escape_sql_string_literal(table_name),
)
)

for _ in rows:
Expand All @@ -1735,7 +1771,10 @@ def has_sequence(self, connection, sequence_name, schema=None, **kw):
WHERE NAME="{sequence_name}"
AND SCHEMA="{schema}"
LIMIT 1
""".format(sequence_name=sequence_name, schema=schema or "")
""".format(
sequence_name=_escape_sql_string_literal(sequence_name),
schema=_escape_sql_string_literal(schema or ""),
)
)

for _ in rows:
Expand Down
70 changes: 70 additions & 0 deletions packages/sqlalchemy-spanner/tests/unit/test_dialect.py
Show comments
View file
Open in desktop
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
Show hidden characters
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@
# limitations under the License.

from unittest.mock import MagicMock
import pytest
from sqlalchemy.testing import eq_
from sqlalchemy.testing.plugin.plugin_base import fixtures
from google.cloud.sqlalchemy_spanner.sqlalchemy_spanner import SpannerDialect
Expand Down Expand Up @@ -98,3 +99,72 @@ def test_max_size_exported(self):
eq_(SpannerDialect.max_size, MAX_SIZE)
eq_(int_from_size("MAX"), 2621440)
eq_(int_from_size("100"), 100)

@staticmethod
def _mock_connection(rows=None):
connection = MagicMock()
mock_snapshot = MagicMock()
mock_snapshot.execute_sql.return_value = rows if rows is not None else []
connection.connection.database.snapshot.return_value.__enter__.return_value = (
mock_snapshot
)
return connection, mock_snapshot

def test_get_columns_escapes_quote_in_table_name(self):
"""A single quote in a reflected table name must not break out of the
INFORMATION_SCHEMA string literal in get_columns."""
dialect = SpannerDialect()
connection, mock_snapshot = self._mock_connection()

dialect.get_columns(connection, table_name="t' OR '1'='1")

executed_sql = mock_snapshot.execute_sql.call_args[0][0]
assert "col.table_name = 't\\' OR \\'1\\'=\\'1'" in executed_sql
assert "col.table_name = 't' OR '1'='1'" not in executed_sql

def test_has_table_escapes_quote_in_table_name(self):
"""A double quote in a reflected table name must not break out of the
INFORMATION_SCHEMA string literal in has_table."""
dialect = SpannerDialect()
connection, mock_snapshot = self._mock_connection()

dialect.has_table(connection, table_name='a" OR "1"="1')

executed_sql = mock_snapshot.execute_sql.call_args[0][0]
assert 'TABLE_NAME="a\\" OR \\"1\\"=\\"1"' in executed_sql
assert 'TABLE_NAME="a" OR "1"="1"' not in executed_sql

def test_get_view_definition_escapes_quote(self):
"""A quote in a reflected view name must not break out of the literal."""
dialect = SpannerDialect()
connection, mock_snapshot = self._mock_connection(rows=[["def"]])

dialect.get_view_definition(connection, view_name="v' OR '1'='1")

executed_sql = mock_snapshot.execute_sql.call_args[0][0]
assert "TABLE_NAME='v\\' OR \\'1\\'=\\'1'" in executed_sql

def test_escape_sql_string_literal(self):
"""The helper escapes backslashes, both quote styles and newlines."""
from google.cloud.sqlalchemy_spanner.sqlalchemy_spanner import (
_escape_sql_string_literal,
)

eq_(_escape_sql_string_literal("a'b"), "a\\'b")
eq_(_escape_sql_string_literal('a"b'), 'a\\"b')
eq_(_escape_sql_string_literal("a\\b"), "a\\\\b")
eq_(_escape_sql_string_literal("a\nb"), "a\\nb")
eq_(_escape_sql_string_literal("plain"), "plain")
Comment on lines +147 to +157

gemini-code-assist Bot Aug 24, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

Let's add test assertions to verify that _escape_sql_string_literal raises a ProgrammingError when receiving unsupported types like None or non-string inputs, ensuring fail-fast behavior.

Suggested change
def test_escape_sql_string_literal(self):
"""The helper escapes backslashes, both quote styles and newlines."""
from google.cloud.sqlalchemy_spanner.sqlalchemy_spanner import (
_escape_sql_string_literal,
)
eq_(_escape_sql_string_literal("a'b"), "a\\'b")
eq_(_escape_sql_string_literal('a"b'), 'a\\"b')
eq_(_escape_sql_string_literal("a\\b"), "a\\\\b")
eq_(_escape_sql_string_literal("a\nb"), "a\\nb")
eq_(_escape_sql_string_literal("plain"), "plain")
def test_escape_sql_string_literal(self):
"""The helper escapes backslashes, both quote styles and newlines."""
from google.cloud.sqlalchemy_spanner.sqlalchemy_spanner import (
_escape_sql_string_literal,
)
from google.cloud.spanner_dbapi import ProgrammingError
eq_(_escape_sql_string_literal("a'b"), "a\\\'b")
eq_(_escape_sql_string_literal('a"b'), 'a\\"b')
eq_(_escape_sql_string_literal("a\\b"), "a\\\\b")
eq_(_escape_sql_string_literal("a\nb"), "a\\nb")
eq_(_escape_sql_string_literal("plain"), "plain")
with self.assertRaises(ProgrammingError):
_escape_sql_string_literal(None)
with self.assertRaises(ProgrammingError):
_escape_sql_string_literal(123)
References
  1. When a function receives parameters of an unsupported type, it should raise an error (e.g., ProgrammingError) instead of silently returning empty values. This ensures fail-fast behavior and prevents potential issues with missing parameter values in database operations.

def test_escape_sql_string_literal_rejects_non_string(self):
"""A non-string name must fail fast rather than be silently coerced."""
from google.cloud.sqlalchemy_spanner.sqlalchemy_spanner import (
_escape_sql_string_literal,
)
from google.cloud.spanner_dbapi.exceptions import ProgrammingError

with pytest.raises(ProgrammingError):
_escape_sql_string_literal(None)

with pytest.raises(ProgrammingError):
_escape_sql_string_literal(123)
Loading
Back | FazBrowse Home | New Git URL