Skip to content

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

Open
Samin061 wants to merge 1 commit into
googleapis:mainfrom
Samin061:sqlalchemy-spanner-reflection-escape
Open

fix(sqlalchemy-spanner): escape names in reflection queries#18205
Samin061 wants to merge 1 commit into
googleapis:mainfrom
Samin061:sqlalchemy-spanner-reflection-escape

Conversation

@Samin061

Copy link
Copy Markdown
Contributor

The reflection methods on SpannerDialect build INFORMATION_SCHEMA predicates by dropping the table, schema, view and sequence names straight into single- and double-quoted GoogleSQL string literals and then run them through snapshot.execute_sql with no query parameters. A name that contains a quote closes the literal so the remainder is parsed as SQL; such a name can arrive from a shared or foreign database enumerated by get_table_names and fed back into get_columns/has_table during MetaData.reflect. Route every reflected name through a GoogleSQL string-literal escape so it stays contained; legitimate identifiers are unchanged.

  • Make sure to open an issue as a bug/issue before writing your code! That way we can discuss the change, evaluate designs, and agree on the general idea
  • Ensure the tests and linter pass
  • Code coverage does not decrease (if any source code was changed)
  • Appropriate docs were updated (if necessary)

@Samin061
Samin061 requested a review from a team as a code owner August 24, 2026 09:04

@gemini-code-assist gemini-code-assist Bot left a comment

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.

Code Review

This pull request introduces a helper function _escape_sql_string_literal to escape special characters (backslashes, quotes, and newlines) in string literals used within INFORMATION_SCHEMA queries, protecting against SQL injection and syntax errors during database reflection. This helper is integrated into several reflection methods, and corresponding unit tests are added. The reviewer feedback suggests enhancing the helper to raise a ProgrammingError when encountering unsupported non-string types (such as None) to ensure fail-fast behavior, along with adding corresponding unit tests to verify this error handling.

Comment on lines +79 to +95
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")
)

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.

medium

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.

Comment on lines +146 to +156
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")

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.

medium

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant