fix(sqlalchemy-spanner): use query parameters in reflection queries

[Samin061]

The reflection methods on SpannerDialect (has_table, has_sequence, get_table_names, get_view_names, get_sequence_names, get_view_definition, get_unique_constraints and the get_multi_* family) built their INFORMATION_SCHEMA predicates by formatting the schema, table, view and sequence names straight into the SQL text. Those names come from the caller (has_table(), Table(..., autoload_with=...), MetaData.reflect(only=...)), so a name containing a quote was parsed as part of the statement.

Following review feedback, the names are now bound as query parameters instead of being escaped: @schema, @table_name / @view_name / @sequence_name, and IN UNNEST(@filter_names) (ARRAY<STRING>) for the filter_names list used by the get_multi_* methods. No name is interpolated into the SQL anymore. The mockserver test expectations are updated to the parameterised statements, and unit tests check both the SQL and the bound parameters for each query shape.

• 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)

[gemini-code-assist]

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.

[gemini-code-assist]

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]

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.

[gemini-code-assist]

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.

[parthea]

Thanks @Samin061! I added one observation, otherwise LGTM. Please also address the feedback from gemini-code-assist

[parthea]

Unrelated to your change, I just noticed that elsewhere in this file we have a trailing space after AND, but here it's missing

Suggested change

	schema_filter_query = " tc.table_schema = '{schema}' AND".format(
	schema_filter_query = " tc.table_schema = '{schema}' AND ".format(

[Samin061]

Good catch, added the trailing space to match the other three queries.

[Samin061]

Both done: the helper now raises ProgrammingError on non-string input (with a test), and I fixed the missing trailing space after AND in get_multi_foreign_keys. All unit tests pass.

[parthea]

@Samin061 , Please could you resolve the conflict to allow test to run?

[olavloite]

I think that the premise of this entire pull request is based on a misconception:

A name that contains a quote closes the literal so the remainder is parsed as SQL

Table names (and also other schema objects) cannot contain quotes in Spanner. Valid table names must adhere to these rules:

• Must be at least one character long.
• Can contain a maximum of 128 characters.
• Must start with an uppercase or lowercase letter.
• Can contain uppercase and lowercase letters, numbers, and underscores, but not hyphens.

See https://docs.cloud.google.com/spanner/docs/reference/standard-sql/data-definition-language#names

That text does indeed not explicitly state that quotes are not allowed, but if you try, then you will see that it is not allowed.

Also, instead of fixing any issues related to this by adding even more string interpolation, it should rather be refactored to use query parameters. That will make it entirely immune to potential SQL injection attacks.

[Samin061]

Fair point on the naming rules: Spanner won't store an object name containing a quote, so the shared-database angle doesn't hold. The names these queries receive still come straight from the caller (has_table(), Table(..., autoload_with=...), MetaData.reflect(only=...)), so I switched all of the INFORMATION_SCHEMA reflection queries to query parameters as you suggested, instead of escaping: @schema, @table_name / @view_name / @sequence_name, and IN UNNEST(@filter_names) (ARRAY<STRING>) for the get_multi_* filter list. Nothing is interpolated into the SQL text anymore and the escaping helper is gone. The mockserver expectations are updated to the parameterised statements, and the unit tests now check the SQL and the bound parameters for each query shape.

@parthea the branch is rebased onto main, which clears the conflict in test_dialect.py.