fix: one identifier quoting style, and the unquoted identifiers underneath it (#785, #409) - #795
Open
axellpadilla wants to merge 1 commit into
Conversation
…785) Relation rendering already emits ANSI "double quoted" identifiers, since the adapter overrides neither quote_character nor quote(). A handful of macros hand-formatted [brackets] instead, so single statements mixed both styles. Those now route through adapter.quote() / the relation object: USE (via the existing get_use_database_sql helper), CREATE SCHEMA, contract column lists, grantees, index and constraint names, and generated test view names. Two things were required to make that correct, so they come with it. Identifiers the adapter builds inside string literals were never quoted. OBJECT_ID('schema.table') returns NULL rather than erroring when the schema holds a '.' or a '"', and callers read that as "object does not exist": the drop-before-create guards in create_table_as skipped and the CREATE then hit Msg 2714, and the mask introspection in apply_masks found no columns, so configured DDM masks were never applied. sp_rename shared the defect, failing the rename-swap with "No item by the name of ...". Those sites now pass quoted, qualified names; sp_rename's @NewName stays bare, since quoting it would make the quotes part of the name. Embedded delimiters are escaped, in adapter.quote() and in relation rendering. A '"' needs no escaping inside [brackets] but does inside double quotes, so the delimiter change alone would have rejected names the adapter previously accepted; and the string-literal fix renders identifiers through the relation. Neither override delegates to super(), which wraps verbatim -- pre-escaping its input would double-escape if upstream ever starts escaping. Also fixes the clustered columnstore index name, which embeds the schema and was emitted bare, so a domain-qualified schema produced unparseable DDL (Incorrect syntax near '\'). A unit test fails the build if a hand-formatted bracket identifier reappears. Server-side QUOTENAME() keeps its brackets: that is dynamic SQL built inside T-SQL, where brackets are the injection-safe idiom. Closes #785 Closes #409
axellpadilla
force-pushed
the
fix/785-consistent-identifier-quoting
branch
from
August 1, 2026 19:11
b4f8dbe to
8d16c6e
Compare
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #785. Closes #409.
Summary
{{ relation }}renders ANSI"double quoted"identifiers — the adapter overrides neitherquote_characternorquote(). A handful of macros hand-formatted[brackets]instead, so single statements mixed both styles. Those now route throughadapter.quote()/ the relation object.Making that correct also required quoting identifiers the adapter builds inside string literals, and escaping embedded delimiters. Both are included.
What changed
One quoting style. No macro hand-formats an identifier any more:
USE,CREATE SCHEMA, contract column lists, grantees, index/constraint names,alter table … drop constraint, and generated test/unit-test view names.get_use_database_sql()already existed with 15+ callers, so the seven hand-formattedUSEspots now call it, and the helper alone emitsadapter.quote()— one place to change rather than nine.Identifiers inside string literals are quoted.
OBJECT_ID('schema.table')returnsNULLrather than erroring when the schema contains a.or a", and every call site read that as "object does not exist". That meant the drop-before-create guards increate_table_asskipped and the CREATE then hitMsg 2714, and the mask introspection inapply_masksfound no columns, so configured DDM masks were never applied.sp_renamehad the same defect, failing the table rename-swap withNo item by the name of …. Eleven sites acrosscreate.sql,apply_masks.sql,indexes.sql,relation.sqlandcolumns.sqlnow pass quoted, qualified names;sp_rename's@newnamestays bare, since quoting it would make the quotes part of the name.Embedded delimiters are escaped.
ab"cdrenders"ab""cd"through bothadapter.quote()andSQLServerRelation.quoted(). This is load-bearing in both directions: a"needs no escaping inside[brackets]but does inside double quotes, so the delimiter change would otherwise reject names the adapter previously accepted; and the string-literal fix renders identifiers through the relation. Neither override delegates tosuper()— pre-escaping input to a method whose contract is wrap verbatim would double-escape if upstream ever starts escaping too.#409. The clustered columnstore index name embeds the schema and was emitted bare, so
domain\userproduced unparseable DDL (Incorrect syntax near '\'). It is quoted at both DDL sites, and still compared as a raw string againstsys.indexes.name.Verification
Checked against a live
SQL_Latin1_General_CP1_CS_ASserver:USE "db"works only withQUOTED_IDENTIFIER ON— with it off it is a hardMsg 102. It is already on by default on all three backends:sessionproperty('QUOTED_IDENTIFIER') = 1onpyodbc,mssql-python, and thego-mssqldb-backedadbc. Issue item 1 therefore needs no code change.OBJECT_ID's bare form returns NULL for.and", but resolves for a backslash or a space.sp_renameaccepts both"quoted"and[bracketed]@objname."in delimited identifiers at schema, table, column and index level.Tests
Full suite green: 784 passed, 48 skipped, 2 xfailed.
Coverage sits in the existing classes rather than new files:
TestIndexMacros— the model's schema is…_dom\usr.x"q, covering the backslash (Database error when using slash in schema name #409, CCI name), the dot and the quote (string literals, rendering) in one name, reusing the class's singledbt build.TestColumnPropertyMasks— a dotted-schema model asserting the mask is applied, guarding the mask path specifically.test_constraints.py— expected-DDL assertions updated to the new output.tests/unit/adapters/mssql/test_quote.py: quoting behaviour plus a lint that fails the build if a hand-formatted bracket identifier reappears (4 patterns, positive-controlled against every form removed here).Non-goals
QUOTENAME()keeps emitting brackets. That is dynamic SQL built inside T-SQL, where brackets are the injection-safe idiom, and it is not SQL the dbt Core v2.0 port copies."handling is hardening rather than a security fix — reachable only through YAML-supplied names, never model names.cci_namestill strips.andfrom the generated index name, and theQUOTENAME()blocks still assume the model's own schema. Both are unchanged here.Stacked on #793 (itself on #792); change the base to
masterand rebase once those land.