fix(sqlalchemy-spanner): quote and escape identifiers in generated DDL - #18225
fix(sqlalchemy-spanner): quote and escape identifiers in generated DDL#18225Samin061 wants to merge 3 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
Code Review
This pull request adds escaping for backslashes and backticks inside backtick-quoted identifiers in the Spanner dialect, and applies quoting to primary key columns and interleaved parent table names. Feedback was provided to handle cases where the interleaved parent is passed as a Table object rather than a string, which would otherwise cause an AttributeError during quoting.
Sorry, something went wrong.
| if table.kwargs.get("spanner_interleave_in"): | ||
| post_cmds += ",\nINTERLEAVE IN PARENT {}".format( | ||
| table.kwargs["spanner_interleave_in"] | ||
| self.preparer.quote(table.kwargs["spanner_interleave_in"]) | ||
| ) |
There was a problem hiding this comment.
If spanner_interleave_in is passed as a Table object (which is common in SQLAlchemy), calling self.preparer.quote() directly on it will raise an AttributeError because quote expects a string identifier. We should check if the object has a name attribute (like a Table object) and extract it before quoting.
if table.kwargs.get("spanner_interleave_in"):
parent = table.kwargs["spanner_interleave_in"]
parent_name = parent.name if hasattr(parent, "name") else parent
post_cmds += ",\nINTERLEAVE IN PARENT {}".format(
self.preparer.quote(parent_name)
)
Sorry, something went wrong.
There was a problem hiding this comment.
Hi @Samin061, Please could you take a look?
Sorry, something went wrong.
There was a problem hiding this comment.
Good catch. I handled it, though the failure actually happens a bit earlier: spanner_interleave_in is documented as a table name string, but if a Table object is passed the if table.kwargs.get(...) truthiness check itself raises TypeError (Table is a ClauseElement) before quote() is ever reached. So I pull the name out ahead of that guard: parent.name if hasattr(parent, "name") else parent, then quote it. Added unit tests for both the string and Table-object parent.
Sorry, something went wrong.
Signed-off-by: bibi samina <sam@bugqore.com>
|
@Samin061 , Please could you resolve the file conflict? Tests won't run until the conflict is resolved |
Sorry, something went wrong.
…l-identifier-quote # Conflicts: # packages/sqlalchemy-spanner/tests/unit/test_dialect.py
|
Merged main in and resolved it. The conflict was just the import block at the top of test_dialect.py, no logic changed. Should be clear to run now. |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
The PRIMARY KEY and INTERLEAVE IN PARENT clauses that post_create_table appends to generated CREATE TABLE DDL interpolate column and table identifiers raw, unlike the STORING/INTERLEAVE handling in visit_create_index and DROP INDEX which route through self.preparer.quote. A reserved-word or hyphenated primary-key column produces invalid DDL, and a name carrying a backtick (reachable by reflecting a shared database's primary key via get_multi_pk_constraint and recreating it with create_all) terminates the quoted identifier and injects trailing DDL. SpannerIdentifierPreparer also kept the base preparer's double-quote escape, so quote wrapped names in backticks without neutralizing an embedded backtick; this routes the primary-key columns and interleave parent through quote and overrides _escape_identifier to backslash-escape backslash and backtick, matching parse_utils.escape_name.