fix(sqlalchemy-spanner): escape names in reflection queries - #18205
fix(sqlalchemy-spanner): escape names in reflection queries#18205Samin061 wants to merge 2 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| 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") | ||
| ) |
There was a problem hiding this comment.
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.
| 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") | |
| ) |
- 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.
Sorry, something went wrong.
There was a problem hiding this comment.
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.
Sorry, something went wrong.
| 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") |
There was a problem hiding this comment.
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.
| 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) |
- 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.
Sorry, something went wrong.
| @@ -1525,7 +1550,7 @@ def get_multi_foreign_keys( | |||
| """ | |||
| table_filter_query = self._get_table_filter_query(filter_names, "tc", True) | |||
| schema_filter_query = " tc.table_schema = '{schema}' AND".format( | |||
There was a problem hiding this comment.
Unrelated to your change, I just noticed that elsewhere in this file we have a trailing space after AND, but here it's missing
| schema_filter_query = " tc.table_schema = '{schema}' AND".format( | |
| schema_filter_query = " tc.table_schema = '{schema}' AND ".format( |
Sorry, something went wrong.
There was a problem hiding this comment.
Good catch, added the trailing space to match the other three queries.
Sorry, something went wrong.
|
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. |
Sorry, something went wrong.
|
@Samin061 , Please could you resolve the conflict to allow test to run? |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
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.