From 8d16c6edb44d2d3c22e1eec96e4998e8162ddcd3 Mon Sep 17 00:00:00 2001 From: Axell Padilla <68310020+axellpadilla@users.noreply.github.com> Date: Sat, 1 Aug 2026 07:37:05 +0000 Subject: [PATCH] fix: emit one identifier quoting style, and quote what was left bare (#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 --- CHANGELOG.md | 10 +++ dbt/adapters/sqlserver/sqlserver_adapter.py | 16 ++++ dbt/adapters/sqlserver/sqlserver_relation.py | 9 +++ .../macros/adapters/apply_grants.sql | 6 +- .../sqlserver/macros/adapters/apply_masks.sql | 6 +- .../sqlserver/macros/adapters/columns.sql | 2 +- .../sqlserver/macros/adapters/indexes.sql | 49 +++++++----- .../sqlserver/macros/adapters/metadata.sql | 2 +- .../sqlserver/macros/adapters/relation.sql | 3 +- .../sqlserver/macros/adapters/schema.sql | 6 +- .../unit_test/unit_test_create_table_as.sql | 2 +- .../macros/materializations/tests.sql | 7 +- .../macros/materializations/unit_tests.sql | 10 ++- .../macros/relations/table/create.sql | 12 +-- .../macros/relations/views/create.sql | 2 +- .../adapter/dbt/test_constraints.py | 8 +- .../adapter/mssql/test_index_macros.py | 29 +++++++ tests/functional/adapter/mssql/test_masks.py | 38 ++++++++- tests/unit/adapters/mssql/test_quote.py | 78 +++++++++++++++++++ 19 files changed, 243 insertions(+), 52 deletions(-) create mode 100644 tests/unit/adapters/mssql/test_quote.py diff --git a/CHANGELOG.md b/CHANGELOG.md index dc8551e02..e658860ba 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,16 @@ - **Behavior change:** `dbt_sqlserver_use_native_string_types` now defaults to `True`: `STRING` maps to `VARCHAR(MAX)`, `NCHAR` to `NCHAR(1)`, and `NVARCHAR` to `NVARCHAR(4000)`, instead of the legacy `VARCHAR(8000)`/`CHAR(1)` mappings. The `False` (legacy) behavior is deprecated and will be removed in a future release. - Add an experimental `adbc` backend (`backend: adbc`), an alternative to `pyodbc`/`mssql-python` built on [ADBC](https://arrow.apache.org/adbc/) that talks to SQL Server via `go-mssqldb` instead of an ODBC/DB-API bridge. Install with `dbt-sqlserver[adbc]` plus the separate `dbc` CLI-installed driver binary; supports SQL Server (user/password) authentication only for now. See [docs/adbc_backend.md](docs/adbc_backend.md). [#771](https://github.com/dbt-msft/dbt-sqlserver/issues/771) +#### Bugfixes + +- Fix models failing with `Incorrect syntax near '\'` when the schema name needs delimiters, such as a domain-qualified `domain\user`. The clustered columnstore index name embeds the schema and was emitted as a bare identifier, so the generated DDL did not parse. [#409](https://github.com/dbt-msft/dbt-sqlserver/issues/409) +- Fix identifiers built inside string literals not being quoted, which broke schema names containing a `.` or a `"`. `OBJECT_ID('schema.table')` returns `NULL` rather than erroring for such a name, so the failures were silent: the drop-before-create guards in `create_table_as` treated an existing table as absent (then hit `Msg 2714`), and the mask introspection in `apply_masks` found no columns, so configured masks were never applied. `sp_rename` was affected too, failing the table rename-swap with `No item by the name of ...`. All now pass quoted, qualified names. [#785](https://github.com/dbt-msft/dbt-sqlserver/issues/785) + +#### Under the hood + +- **Behavior change (generated SQL only):** the few macros that hand-formatted `[bracket]` identifiers now use `adapter.quote()`, matching the `"double quoted"` identifiers `{{ relation }}` already rendered, so one statement no longer mixes both styles. Affects `USE` (now via the existing `get_use_database_sql()` helper), `CREATE SCHEMA`, contract column lists, grantees, index/constraint names and generated test view names. Server-side `QUOTENAME()` keeps brackets by design. Visible only if you parse dbt's generated SQL or have custom macros assuming brackets. [#785](https://github.com/dbt-msft/dbt-sqlserver/issues/785) +- Identifier quoting escapes an embedded `"` by doubling it (`ab"cd` → `"ab""cd"`), in both `adapter.quote()` and relation rendering (`SQLServerRelation.quoted`). Needed twice over: a `"` requires no escaping inside `[brackets]` but does inside double quotes, so the delimiter change above would otherwise have rejected names the adapter previously accepted; and the string-literal fix above renders identifiers through the relation, so a schema containing a `"` depends on it. [#785](https://github.com/dbt-msft/dbt-sqlserver/issues/785) + ### v1.11.0 #### Features diff --git a/dbt/adapters/sqlserver/sqlserver_adapter.py b/dbt/adapters/sqlserver/sqlserver_adapter.py index 929c4ad62..a5a63d9d0 100644 --- a/dbt/adapters/sqlserver/sqlserver_adapter.py +++ b/dbt/adapters/sqlserver/sqlserver_adapter.py @@ -188,6 +188,22 @@ def get_column_schema_from_query(self, sql: str) -> List[BaseColumn]: ] return columns + @classmethod + def quote(cls, identifier: str) -> str: + """Double-quote an identifier, doubling any embedded double quote. + + ``SQLAdapter.quote`` interpolates the identifier verbatim, so a name + containing a ``"`` would close the quoted identifier early and the + remainder would parse as SQL. T-SQL escapes a delimiter by doubling + it -- the same rule ``QUOTENAME()`` applies to brackets -- so + ``ab"cd`` must render as ``"ab""cd"``. + + This is the quoting used by every macro that formats an identifier + (see #785). Relation rendering escapes nothing, since it goes through + ``BaseRelation.quote_character`` upstream rather than this method. + """ + return '"{}"'.format(str(identifier).replace('"', '""')) + @classmethod def convert_boolean_type(cls, agate_table, col_idx): return "bit" diff --git a/dbt/adapters/sqlserver/sqlserver_relation.py b/dbt/adapters/sqlserver/sqlserver_relation.py index 20f5047ef..731fbc541 100644 --- a/dbt/adapters/sqlserver/sqlserver_relation.py +++ b/dbt/adapters/sqlserver/sqlserver_relation.py @@ -26,6 +26,15 @@ class SQLServerRelation(BaseRelation): def get_relation_type(cls) -> Type[SQLServerRelationType]: return SQLServerRelationType + def quoted(self, identifier): + """Quote a path part, doubling any embedded quote character (#785). + + Not delegated to super(): that wraps verbatim, so pre-escaping the + input would double-escape if upstream ever starts escaping too. + """ + quote_char = self.quote_character + return f"{quote_char}{str(identifier).replace(quote_char, quote_char * 2)}{quote_char}" + def _render_limited_alias(self) -> str: if self.disable_empty_relation_aliases: return "" diff --git a/dbt/include/sqlserver/macros/adapters/apply_grants.sql b/dbt/include/sqlserver/macros/adapters/apply_grants.sql index a9ccbd9cd..fbeb53d8f 100644 --- a/dbt/include/sqlserver/macros/adapters/apply_grants.sql +++ b/dbt/include/sqlserver/macros/adapters/apply_grants.sql @@ -48,7 +48,7 @@ {%- macro sqlserver__get_grant_sql(relation, privilege, grantees) -%} {%- set grantees_safe = [] -%} {%- for grantee in grantees -%} - {%- set grantee_safe = "[" ~ grantee ~ "]" -%} + {%- set grantee_safe = adapter.quote(grantee) -%} {%- do grantees_safe.append(grantee_safe) -%} {%- endfor -%} grant {{ privilege }} on {{ relation }} to {{ grantees_safe | join(', ') }} @@ -57,7 +57,7 @@ {%- macro sqlserver__get_revoke_sql(relation, privilege, grantees) -%} {%- set grantees_safe = [] -%} {%- for grantee in grantees -%} - {%- set grantee_safe = "[" ~ grantee ~ "]" -%} + {%- set grantee_safe = adapter.quote(grantee) -%} {%- do grantees_safe.append(grantee_safe) -%} {%- endfor -%} revoke {{ privilege }} on {{ relation }} from {{ grantees_safe | join(', ') }} @@ -66,6 +66,6 @@ {% macro get_provision_sql(relation, privilege, grantees) %} {% for grantee in grantees %} if not exists(select name from sys.database_principals where name = '{{ grantee }}') - create user [{{ grantee }}] from external provider; + create user {{ adapter.quote(grantee) }} from external provider; {% endfor %} {% endmacro %} diff --git a/dbt/include/sqlserver/macros/adapters/apply_masks.sql b/dbt/include/sqlserver/macros/adapters/apply_masks.sql index 2554656e2..c534344e1 100644 --- a/dbt/include/sqlserver/macros/adapters/apply_masks.sql +++ b/dbt/include/sqlserver/macros/adapters/apply_masks.sql @@ -33,7 +33,7 @@ c.name as name, c.masking_function as masking_function from sys.masked_columns c {{ information_schema_hints() }} - where c.object_id = OBJECT_ID('{{ relation.schema }}.{{ relation.identifier }}') + where c.object_id = OBJECT_ID('{{ escape_single_quotes(relation.include(database=False)) }}') {% endmacro %} @@ -47,7 +47,7 @@ from sys.index_columns ic {{ information_schema_hints() }} inner join sys.columns col {{ information_schema_hints() }} on col.object_id = ic.object_id and col.column_id = ic.column_id - where ic.object_id = OBJECT_ID('{{ relation.schema }}.{{ relation.identifier }}') + where ic.object_id = OBJECT_ID('{{ escape_single_quotes(relation.include(database=False)) }}') and ic.is_included_column = 0 {% endcall %} {% set result = [] %} @@ -64,7 +64,7 @@ {% call statement('get_unmaskable_columns', fetch_result=True) %} select col.name as name from sys.columns col {{ information_schema_hints() }} - where col.object_id = OBJECT_ID('{{ relation.schema }}.{{ relation.identifier }}') + where col.object_id = OBJECT_ID('{{ escape_single_quotes(relation.include(database=False)) }}') and ( col.is_computed = 1 or col.is_filestream = 1 diff --git a/dbt/include/sqlserver/macros/adapters/columns.sql b/dbt/include/sqlserver/macros/adapters/columns.sql index 685ec94f6..d43ad5818 100644 --- a/dbt/include/sqlserver/macros/adapters/columns.sql +++ b/dbt/include/sqlserver/macros/adapters/columns.sql @@ -62,7 +62,7 @@ drop column "{{ column_name }}"; {%- endset %} {% set rename_column %} - exec sp_rename '{{ relation | replace('"', '') }}.{{ tmp_column }}', '{{ column_name }}', 'column' + exec sp_rename '{{ escape_single_quotes(relation.include(database=False)) }}.{{ escape_single_quotes(adapter.quote(tmp_column)) }}', '{{ escape_single_quotes(column_name) }}', 'column' {%- endset %} {% do run_query(add_column) %} diff --git a/dbt/include/sqlserver/macros/adapters/indexes.sql b/dbt/include/sqlserver/macros/adapters/indexes.sql index 32107c1c3..53e14a80b 100644 --- a/dbt/include/sqlserver/macros/adapters/indexes.sql +++ b/dbt/include/sqlserver/macros/adapters/indexes.sql @@ -1,17 +1,28 @@ +{#- Quoted, comma-separated column list for index DDL. -#} +{% macro quote_column_list(columns) -%} + {%- set quoted = [] -%} + {%- for column in columns -%} + {%- do quoted.append(adapter.quote(column)) -%} + {%- endfor -%} + {{- quoted | join(', ') -}} +{%- endmacro %} + + {% macro sqlserver__create_clustered_columnstore_index(relation) -%} + {#- cci_name embeds the schema, so it must be quoted as an identifier + (raw only in the string comparison below) -- issue #409 -#} {%- set cci_name = (relation.schema ~ '_' ~ relation.identifier ~ '_cci') | replace(".", "") | replace(" ", "") -%} {%- set relation_name = relation.include(database=False) -%} - {%- set full_relation = '"' ~ relation.schema ~ '"."' ~ relation.identifier ~ '"' -%} - use [{{ relation.database }}]; + {{ get_use_database_sql(relation.database) }} if EXISTS ( SELECT * FROM sys.indexes {{ information_schema_hints() }} - WHERE name = '{{cci_name}}' - AND object_id=object_id('{{relation_name}}') + WHERE name = '{{ escape_single_quotes(cci_name) }}' + AND object_id=object_id('{{ escape_single_quotes(relation_name) }}') ) - DROP index {{full_relation}}.{{cci_name}} - CREATE CLUSTERED COLUMNSTORE INDEX {{cci_name}} - ON {{full_relation}} + DROP index {{ relation_name }}.{{ adapter.quote(cci_name) }} + CREATE CLUSTERED COLUMNSTORE INDEX {{ adapter.quote(cci_name) }} + ON {{ relation_name }} {% endmacro %} {% macro drop_xml_indexes() -%} @@ -152,7 +163,7 @@ {% endif %} clustered index {{ idx_name }} - on {{ this }} ({{ '[' + columns|join("], [") + ']' }}) + on {{ this }} ({{ quote_column_list(columns) }}) end {%- endmacro %} @@ -180,9 +191,9 @@ begin create nonclustered index {{ idx_name }} - on {{ this }} ({{ '[' + columns|join("], [") + ']' }}) + on {{ this }} ({{ quote_column_list(columns) }}) {% if includes -%} - include ({{ '[' + includes|join("], [") + ']' }}) + include ({{ quote_column_list(includes) }}) {% endif %} end {% endmacro %} @@ -190,7 +201,7 @@ {% macro drop_fk_indexes_on_table(relation) -%} {% call statement('find_references', fetch_result=true) %} - USE [{{ relation.database }}]; + {{ get_use_database_sql(relation.database) }} SELECT obj.name AS FK_NAME, sch.name AS [schema_name], tab1.name AS [table], @@ -215,7 +226,7 @@ {% set references = load_result('find_references')['data'] %} {% for reference in references -%} {% call statement('main') -%} - alter table [{{reference[1]}}].[{{reference[2]}}] drop constraint [{{reference[0]}}] + alter table {{ adapter.quote(reference[1]) }}.{{ adapter.quote(reference[2]) }} drop constraint {{ adapter.quote(reference[0]) }} {%- endcall %} {% endfor %} {% endmacro %} @@ -229,7 +240,7 @@ FROM sys.indexes AS i {{ information_schema_hints() }} INNER JOIN sys.index_columns AS ic {{ information_schema_hints() }} ON i.object_id = ic.object_id AND i.index_id = ic.index_id and i.type <> 5 - WHERE i.object_id = OBJECT_ID('{{ relation.schema }}.{{ relation.identifier }}') + WHERE i.object_id = OBJECT_ID('{{ escape_single_quotes(relation.include(database=False)) }}') UNION ALL @@ -283,21 +294,21 @@ and object_id = OBJECT_ID('{{ relation }}') ) begin - {# key columns: bracket-quoted (with ]] escaping) plus per-column direction #} + {# key columns: quoted (adapter.quote escapes embedded quotes) plus per-column direction #} {%- set key_columns = [] -%} {%- for column in index_config.columns -%} {%- do key_columns.append( - '[' ~ column | replace(']', ']]') ~ ']' + adapter.quote(column) ~ (' desc' if column in index_config.descending_columns else '') ) -%} {%- endfor -%} {%- set include_columns = [] -%} {%- for column in index_config.included_columns -%} - {%- do include_columns.append('[' ~ column | replace(']', ']]') ~ ']') -%} + {%- do include_columns.append(adapter.quote(column)) -%} {%- endfor %} create {% if index_config.unique -%} unique {% endif %}{{ index_config.type }} - index [{{ index_name }}] + index {{ adapter.quote(index_name) }} on {{ relation }} ({{ key_columns | join(', ') }}) {% if include_columns -%} @@ -436,7 +447,7 @@ from sys.partitions p {{ information_schema_hints() }} where p.object_id = i.object_id and p.index_id = i.index_id ) part - where i.object_id = OBJECT_ID('{{ relation.schema }}.{{ relation.identifier }}') + where i.object_id = OBJECT_ID('{{ escape_single_quotes(relation.include(database=False)) }}') and i.index_id > 0 and i.[type] not in (3, 4, 7) /* xml, spatial, memory-optimized hash */ {%- endcall %} @@ -445,7 +456,7 @@ {% macro sqlserver__get_drop_index_sql(relation, index_name) -%} - drop index [{{ index_name }}] on {{ relation }} + drop index {{ adapter.quote(index_name) }} on {{ relation }} {%- endmacro %} diff --git a/dbt/include/sqlserver/macros/adapters/metadata.sql b/dbt/include/sqlserver/macros/adapters/metadata.sql index b1e2258d6..75ff593e1 100644 --- a/dbt/include/sqlserver/macros/adapters/metadata.sql +++ b/dbt/include/sqlserver/macros/adapters/metadata.sql @@ -109,7 +109,7 @@ {% endmacro %} {%- macro sqlserver__get_use_database_sql(database) -%} - USE [{{database | replace('"', '')}}]; + USE {{ adapter.quote(database | replace('"', '')) }}; {%- endmacro -%} {% macro sqlserver__list_schemas(database) %} diff --git a/dbt/include/sqlserver/macros/adapters/relation.sql b/dbt/include/sqlserver/macros/adapters/relation.sql index a248b611f..1ac67422d 100644 --- a/dbt/include/sqlserver/macros/adapters/relation.sql +++ b/dbt/include/sqlserver/macros/adapters/relation.sql @@ -50,7 +50,8 @@ {% macro sqlserver__rename_relation(from_relation, to_relation) -%} {% call statement('rename_relation') -%} {{ get_use_database_sql(from_relation.database) }} - EXEC sp_rename '{{ from_relation.schema }}.{{ from_relation.identifier }}', '{{ to_relation.identifier }}' + {#- @objname takes a quoted, qualified name; @newname must stay bare -#} + EXEC sp_rename '{{ escape_single_quotes(from_relation.include(database=False)) }}', '{{ escape_single_quotes(to_relation.identifier) }}' {%- endcall %} {% endmacro %} diff --git a/dbt/include/sqlserver/macros/adapters/schema.sql b/dbt/include/sqlserver/macros/adapters/schema.sql index 64cf6fbc9..0ea860ca7 100644 --- a/dbt/include/sqlserver/macros/adapters/schema.sql +++ b/dbt/include/sqlserver/macros/adapters/schema.sql @@ -1,9 +1,9 @@ {% macro sqlserver__create_schema(relation) -%} {% call statement('create_schema') -%} - USE [{{ relation.database }}]; + {{ get_use_database_sql(relation.database) }} IF NOT EXISTS (SELECT * FROM sys.schemas WHERE name = '{{ relation.schema }}') BEGIN - EXEC('CREATE SCHEMA [{{ relation.schema }}]') + EXEC('CREATE SCHEMA {{ adapter.quote(relation.schema) }}') END {% endcall %} {% endmacro %} @@ -13,7 +13,7 @@ {{ get_use_database_sql(relation.database) }} IF NOT EXISTS (SELECT * FROM sys.schemas WHERE name = '{{ relation.schema }}') BEGIN - EXEC('CREATE SCHEMA [{{ relation.schema }}] AUTHORIZATION [{{ schema_authorization }}]') + EXEC('CREATE SCHEMA {{ adapter.quote(relation.schema) }} AUTHORIZATION {{ adapter.quote(schema_authorization) }}') END {% endcall %} {% endmacro %} diff --git a/dbt/include/sqlserver/macros/materializations/models/unit_test/unit_test_create_table_as.sql b/dbt/include/sqlserver/macros/materializations/models/unit_test/unit_test_create_table_as.sql index b16587813..218bebfb1 100644 --- a/dbt/include/sqlserver/macros/materializations/models/unit_test/unit_test_create_table_as.sql +++ b/dbt/include/sqlserver/macros/materializations/models/unit_test/unit_test_create_table_as.sql @@ -37,7 +37,7 @@ {% set listColumns %} {% for column in model['columns'] %} - {{ "["~column~"]" }}{{ ", " if not loop.last }} + {{ adapter.quote(column) }}{{ ", " if not loop.last }} {% endfor %} {%endset%} diff --git a/dbt/include/sqlserver/macros/materializations/tests.sql b/dbt/include/sqlserver/macros/materializations/tests.sql index 17d5d1303..bec224656 100644 --- a/dbt/include/sqlserver/macros/materializations/tests.sql +++ b/dbt/include/sqlserver/macros/materializations/tests.sql @@ -1,14 +1,15 @@ {% macro sqlserver__get_test_sql(main_sql, fail_calc, warn_if, error_if, limit) -%} -- Create target schema if it does not - USE [{{ target.database }}]; + {{ get_use_database_sql(target.database) }} IF NOT EXISTS (SELECT * FROM sys.schemas WHERE name = '{{ target.schema }}') BEGIN - EXEC('CREATE SCHEMA [{{ target.schema }}]') + EXEC('CREATE SCHEMA {{ adapter.quote(target.schema) }}') END + {% set testview_name = "testview_" ~ local_md5(main_sql) ~ "_" ~ (range(1300, 19000) | random) %} {% set testview %} - [{{ target.schema }}].[testview_{{ local_md5(main_sql) }}_{{ range(1300, 19000) | random }}] + {{ adapter.quote(target.schema) }}.{{ adapter.quote(testview_name) }} {% endset %} {% set sql = main_sql.replace("'", "''")%} diff --git a/dbt/include/sqlserver/macros/materializations/unit_tests.sql b/dbt/include/sqlserver/macros/materializations/unit_tests.sql index c8b2675ba..d94da335f 100644 --- a/dbt/include/sqlserver/macros/materializations/unit_tests.sql +++ b/dbt/include/sqlserver/macros/materializations/unit_tests.sql @@ -1,19 +1,21 @@ {% macro sqlserver__get_unit_test_sql(main_sql, expected_fixture_sql, expected_column_names) -%} - USE [{{ target.database }}]; + {{ get_use_database_sql(target.database) }} IF NOT EXISTS (SELECT * FROM sys.schemas WHERE name = '{{ target.schema }}') BEGIN - EXEC('CREATE SCHEMA [{{ target.schema }}]') + EXEC('CREATE SCHEMA {{ adapter.quote(target.schema) }}') END + {% set test_view_name = "testview_" ~ local_md5(main_sql) ~ "_" ~ (range(1300, 19000) | random) %} {% set test_view %} - [{{ target.schema }}].[testview_{{ local_md5(main_sql) }}_{{ range(1300, 19000) | random }}] + {{ adapter.quote(target.schema) }}.{{ adapter.quote(test_view_name) }} {% endset %} {% set test_sql = main_sql.replace("'", "''")%} EXEC('create view {{test_view}} as {{ test_sql }};') + {% set expected_view_name = "expectedview_" ~ local_md5(expected_fixture_sql) ~ "_" ~ (range(1300, 19000) | random) %} {% set expected_view %} - [{{ target.schema }}].[expectedview_{{ local_md5(expected_fixture_sql) }}_{{ range(1300, 19000) | random }}] + {{ adapter.quote(target.schema) }}.{{ adapter.quote(expected_view_name) }} {% endset %} {% set expected_sql = expected_fixture_sql.replace("'", "''")%} EXEC('create view {{expected_view}} as {{ expected_sql }};') diff --git a/dbt/include/sqlserver/macros/relations/table/create.sql b/dbt/include/sqlserver/macros/relations/table/create.sql index be74c2a77..31f2ecfad 100644 --- a/dbt/include/sqlserver/macros/relations/table/create.sql +++ b/dbt/include/sqlserver/macros/relations/table/create.sql @@ -23,7 +23,7 @@ {%- set build_into_temp = temporary or _ident.endswith('__dbt_tmp') or _ident.endswith('__dbt_tmp_vw') -%} {%- do adapter.drop_relation(tmp_relation) -%} - USE [{{ relation.database }}]; + {{ get_use_database_sql(relation.database) }} {{ get_create_view_as_sql(tmp_relation, sql) }} {%- set table_name -%} @@ -39,7 +39,7 @@ {{ build_columns_constraints(relation) }} {% set listColumns %} {% for column in model['columns'] %} - {{ "["~column~"]" }}{{ ", " if not loop.last }} + {{ adapter.quote(column) }}{{ ", " if not loop.last }} {% endfor %} {%endset%} INSERT INTO {{relation}} WITH (TABLOCK) ({{listColumns}}) @@ -47,7 +47,7 @@ {% else %} {%- if build_into_temp -%} - IF OBJECT_ID('{{ escape_single_quotes(relation.schema) }}.{{ escape_single_quotes(relation.identifier) }}', 'U') IS NOT NULL + IF OBJECT_ID('{{ escape_single_quotes(relation.include(database=False)) }}', 'U') IS NOT NULL EXEC('DROP TABLE {{ relation }}'); {%- endif -%} SELECT * INTO {{ table_name }} FROM {{ tmp_relation }} {{ query_label }} @@ -120,7 +120,7 @@ later failure during the load would roll the marker back right along with it, defeating the point. -#} {%- set setup_sql -%} - USE [{{ relation.database }}]; + {{ get_use_database_sql(relation.database) }} {{ get_create_view_as_sql(tmp_relation, sql) }} {#- drop any physical target before the bare CREATE / SELECT ... INTO @@ -128,7 +128,7 @@ dbt's relation cache being stale (reporting none while the table exists) - which would otherwise collide with Msg 2714 (object already exists) -#} - IF OBJECT_ID('{{ escape_single_quotes(relation.schema) }}.{{ escape_single_quotes(relation.identifier) }}', 'U') IS NOT NULL + IF OBJECT_ID('{{ escape_single_quotes(relation.include(database=False)) }}', 'U') IS NOT NULL EXEC('DROP TABLE {{ relation }}'); {% if contract_enforced %} @@ -169,7 +169,7 @@ {%- if contract_enforced -%} {%- set listColumns -%} {%- for column in model['columns'] -%} - {{ "["~column~"]" }}{{ ", " if not loop.last }} + {{ adapter.quote(column) }}{{ ", " if not loop.last }} {%- endfor -%} {%- endset -%} {%- set insert_statement -%} diff --git a/dbt/include/sqlserver/macros/relations/views/create.sql b/dbt/include/sqlserver/macros/relations/views/create.sql index bd54476f8..3ea79c488 100644 --- a/dbt/include/sqlserver/macros/relations/views/create.sql +++ b/dbt/include/sqlserver/macros/relations/views/create.sql @@ -24,7 +24,7 @@ CREATE OR ALTER VIEW {{ relation.include(database=False) }} AS {{ sql }}; {% endset %} - USE [{{ relation.database }}]; + {{ get_use_database_sql(relation.database) }} EXEC('{{- escape_single_quotes(query) -}}') {% endmacro %} diff --git a/tests/functional/adapter/dbt/test_constraints.py b/tests/functional/adapter/dbt/test_constraints.py index 328c97cb0..34459093a 100644 --- a/tests/functional/adapter/dbt/test_constraints.py +++ b/tests/functional/adapter/dbt/test_constraints.py @@ -472,7 +472,7 @@ def models(self): @pytest.fixture(scope="class") def expected_sql(self): return """ - EXEC(' CREATE OR ALTER VIEW AS -- depends_on: select ''blue'' as color, 1 as id, ''2019-01-01'' as date_day; ') EXEC(' CREATE TABLE ( id int not null , color varchar(100), date_day varchar(100) ) INSERT INTO WITH (TABLOCK) ( [id], [color], [date_day] ) SELECT [id], [color], [date_day] FROM ') EXEC('DROP VIEW IF EXISTS + EXEC(' CREATE OR ALTER VIEW AS -- depends_on: select ''blue'' as color, 1 as id, ''2019-01-01'' as date_day; ') EXEC(' CREATE TABLE ( id int not null , color varchar(100), date_day varchar(100) ) INSERT INTO WITH (TABLOCK) ( "id", "color", "date_day" ) SELECT "id", "color", "date_day" FROM ') EXEC('DROP VIEW IF EXISTS """ # EXEC('DROP view IF EXISTS @@ -494,7 +494,7 @@ def test__constraints_ddl(self, project, expected_sql): generated_sql_generic = _find_and_replace( generated_sql_generic, "foreign_key_model", "" ) - generated_sql_wodb = generated_sql_generic.replace("USE [" + project.database + "];", "") + generated_sql_wodb = generated_sql_generic.replace('USE "' + project.database + '";', "") generated_sql_option_clause = generated_sql_wodb.replace( "OPTION (LABEL = ''dbt-sqlserver'');", "" ) @@ -592,7 +592,7 @@ def models(self): @pytest.fixture(scope="class") def expected_sql(self): return """ - EXEC(' CREATE OR ALTER VIEW AS -- depends_on: select ''blue'' as color, 1 as id, ''2019-01-01'' as date_day; ') EXEC(' CREATE TABLE ( id int not null , color varchar(100), date_day varchar(100) ) INSERT INTO WITH (TABLOCK) ( [id], [color], [date_day] ) SELECT [id], [color], [date_day] FROM ') EXEC('DROP VIEW IF EXISTS + EXEC(' CREATE OR ALTER VIEW AS -- depends_on: select ''blue'' as color, 1 as id, ''2019-01-01'' as date_day; ') EXEC(' CREATE TABLE ( id int not null , color varchar(100), date_day varchar(100) ) INSERT INTO WITH (TABLOCK) ( "id", "color", "date_day" ) SELECT "id", "color", "date_day" FROM ') EXEC('DROP VIEW IF EXISTS """ def test__model_constraints_ddl(self, project, expected_sql): @@ -611,7 +611,7 @@ def test__model_constraints_ddl(self, project, expected_sql): generated_sql_generic = _find_and_replace( generated_sql_generic, "foreign_key_model", "" ) - generated_sql_wodb = generated_sql_generic.replace("USE [" + project.database + "];", "") + generated_sql_wodb = generated_sql_generic.replace('USE "' + project.database + '";', "") generated_sql_option_clause = generated_sql_wodb.replace( "OPTION (LABEL = ''dbt-sqlserver'');", "" ) diff --git a/tests/functional/adapter/mssql/test_index_macros.py b/tests/functional/adapter/mssql/test_index_macros.py index 80f47e595..2c00d2513 100644 --- a/tests/functional/adapter/mssql/test_index_macros.py +++ b/tests/functional/adapter/mssql/test_index_macros.py @@ -120,6 +120,14 @@ select * from {{ ref('raw_data') }} """ +# A schema name needing delimiters: the backslash reaches the generated CCI +# name (#409), the dot and the quote reach identifiers built inside string +# literals. Raw string: what is written here is exactly what Jinja parses. +backslash_schema_model = r""" +{{ config(materialized='table', schema=target.schema ~ '_dom\\usr.x\"q') }} +select 1 as id +""" + class TestIndexMacros: """Every index-macro assertion in this module shares one project and one @@ -151,6 +159,7 @@ def models(self): "index_model.sql": drop_schema_model, "index_ccs_model.sql": model_sql_ccs, "fk_model.sql": drop_both_fk_directions_model, + "backslash_schema_model.sql": backslash_schema_model, "schema.yml": model_yml, } @@ -207,12 +216,15 @@ def drop_schema_artifacts(self, project): drop_index = f"DROP INDEX IF EXISTS sample_schema ON {_schema}.index_model" drop_table = f"DROP TABLE IF EXISTS {_schema}.index_model" drop_schema = f"DROP SCHEMA IF EXISTS {_schema}" + bs_schema = (project.test_schema + '_dom\\usr.x"q').replace('"', '""') # single backslash with get_connection(project.adapter): project.adapter.execute(drop_child_table, fetch=True) project.adapter.execute(drop_index) project.adapter.execute(drop_table) project.adapter.execute(drop_schema) + project.adapter.execute(f'DROP TABLE IF EXISTS "{bs_schema}".backslash_schema_model') + project.adapter.execute(f'DROP SCHEMA IF EXISTS "{bs_schema}"') def drop_fk_artifacts(self, project): # Ordered so teardown still works when the macro under test leaves a @@ -268,6 +280,23 @@ def test_create_index(self, project): "nonclustered": 4, } + def test_builds_in_a_schema_needing_delimiters(self, project): + """A backslash in the schema reaches the CCI name (#409).""" + with get_connection(project.adapter): + _, table = project.adapter.execute( + f""" + select i.type_desc + from sys.indexes i + join sys.tables t on t.object_id = i.object_id + join sys.schemas s on s.schema_id = t.schema_id + where s.name like '{project.test_schema}%' and s.name like '%\\%' + and t.name = 'backslash_schema_model' + and i.type_desc = 'CLUSTERED COLUMNSTORE' + """, + fetch=True, + ) + assert len(table.rows) == 1, f"expected a clustered columnstore index, got {table.rows}" + def test_leaves_other_schemas_alone(self, project): self.validate_other_schema(project) diff --git a/tests/functional/adapter/mssql/test_masks.py b/tests/functional/adapter/mssql/test_masks.py index 232ccd1bb..3d5380638 100644 --- a/tests/functional/adapter/mssql/test_masks.py +++ b/tests/functional/adapter/mssql/test_masks.py @@ -17,12 +17,13 @@ # --------------------------------------------------------------------------- -def masked_columns(project, table_name): +def masked_columns(project, table_name, schema=None): """Return {column_name: masking_function} from sys.masked_columns.""" + schema = schema or project.test_schema sql = f""" select c.name, c.masking_function from sys.masked_columns c - where c.object_id = OBJECT_ID('{project.test_schema}.{table_name}') + where c.object_id = OBJECT_ID('"{schema}"."{table_name}"') """ with get_connection(project.adapter): _, table = project.adapter.execute(sql, fetch=True) @@ -71,6 +72,22 @@ def select_as_unprivileged(project, table_name, columns): masked_with: 'partial(0,"XXXXXXXXXX",0)' """ +# A schema whose name needs delimiters: a dot made the bare OBJECT_ID() lookup +# in the mask macros return NULL, so masks were silently never applied (#785). +dotted_schema_model_sql = """ +{{ config(materialized="table", schema=target.schema ~ '.x') }} +select cast('Smith' as varchar(50)) as surname +""" + +dotted_schema_yml = """ +version: 2 +models: + - name: dotted_schema_model + columns: + - name: surname + masked_with: "default()" +""" + # model-level `masks` dict surface model_level_masks_sql = """ {{ config( @@ -95,8 +112,18 @@ def models(self): return { "masked_model.sql": column_property_model_sql, "masked_model.yml": column_property_yml, + "dotted_schema_model.sql": dotted_schema_model_sql, + "dotted_schema_model.yml": dotted_schema_yml, } + @pytest.fixture(scope="class", autouse=True) + def drop_dotted_schema(self, project): + yield + schema = f"{project.test_schema}.x" + with get_connection(project.adapter): + project.adapter.execute(f'DROP TABLE IF EXISTS "{schema}".dotted_schema_model') + project.adapter.execute(f'DROP SCHEMA IF EXISTS "{schema}"') + def test_masks_applied_and_survive_full_refresh(self, project): run_dbt(["run"]) masks = masked_columns(project, "masked_model") @@ -111,6 +138,13 @@ def test_masks_applied_and_survive_full_refresh(self, project): assert masks.get("surname") == "default()" assert masks.get("nhs_number") == 'partial(0, "XXXXXXXXXX", 0)' + def test_masks_applied_in_a_schema_needing_delimiters(self, project): + """A dot in the schema made OBJECT_ID() return NULL, so the mask + introspection found nothing and masks were silently skipped (#785).""" + run_dbt(["run"]) + masks = masked_columns(project, "dotted_schema_model", schema=f"{project.test_schema}.x") + assert masks.get("surname") == "default()" + def test_unprivileged_user_sees_masked_values(self, project): run_dbt(["run"]) surname, nhs = select_as_unprivileged(project, "masked_model", ["surname", "nhs_number"]) diff --git a/tests/unit/adapters/mssql/test_quote.py b/tests/unit/adapters/mssql/test_quote.py new file mode 100644 index 000000000..e02f61905 --- /dev/null +++ b/tests/unit/adapters/mssql/test_quote.py @@ -0,0 +1,78 @@ +"""Identifier quoting (#785). + +Macros route identifiers through ``adapter.quote()`` instead of hand-formatting +them, so the escaping has to live in ``quote()`` itself. +""" + +import re +from pathlib import Path + +import pytest + +from dbt.adapters.sqlserver.sqlserver_adapter import SQLServerAdapter +from dbt.adapters.sqlserver.sqlserver_relation import SQLServerRelation + + +def _relation(**kwargs): + return SQLServerRelation.create(type="table", **kwargs) + + +@pytest.mark.parametrize( + "identifier,expected", + [ + ("id", '"id"'), + ("my column", '"my column"'), + ("MODEL", '"MODEL"'), # case preserved; it matters on a CS collation + ("weird]name", '"weird]name"'), # brackets are ordinary characters here + ('ab"cd', '"ab""cd"'), # the delimiter is escaped by doubling + ('"', '""""'), + ('x" from sys.tables--', '"x"" from sys.tables--"'), + ], +) +def test_quote_escapes_embedded_delimiters(identifier, expected): + assert SQLServerAdapter.quote(identifier) == expected + + +def test_relation_rendering_escapes_like_adapter_quote(): + """Both quoting paths must agree: identifiers built inside string literals + (OBJECT_ID, sp_rename) are rendered, not passed through adapter.quote().""" + assert _relation(database="d", schema='s"c', identifier="t").render() == '"d"."s""c"."t"' + for part in ("plain", 'ab"cd', "MODEL"): + rendered = _relation(database="d", schema="s", identifier=part).render() + assert rendered.split(".")[-1] == SQLServerAdapter.quote(part) + + +def test_relation_render_unchanged_for_ordinary_names(): + relation = _relation(database="TestDB", schema="dbo", identifier="my_model") + assert relation.render() == '"TestDB"."dbo"."my_model"' + + +MACRO_ROOT = Path(__file__).parents[4] / "dbt" / "include" / "sqlserver" / "macros" + +# Jinja-interpolated identifiers wrapped by hand. Each drifts from what relation +# rendering emits, and skips the escaping in quote(). +HAND_QUOTING = { + "[{{ ... }}]": re.compile(r"\[\s*\{\{"), + "'[' ~ concat": re.compile(r"""["']\[["']\s*[~+]"""), + 'join("], [")': re.compile(r"""join\(\s*["']\]\s*,\s*\[["']\s*\)"""), + "'\"' ~ concat": re.compile(r"""'"'\s*[~+]|"\\""\s*[~+]"""), +} + +# Brackets that are not identifier quoting: split_part's XQuery predicate. +NOT_QUOTING = (re.compile(r"AS XML\)\.value\("),) + + +@pytest.mark.parametrize("macro_file", sorted(MACRO_ROOT.rglob("*.sql")), ids=lambda p: p.name) +def test_macros_do_not_hand_format_identifiers(macro_file): + offenders = [] + for lineno, line in enumerate(macro_file.read_text().splitlines(), start=1): + if any(x.search(line) for x in NOT_QUOTING): + continue + for label, pattern in HAND_QUOTING.items(): + if pattern.search(line): + offenders.append(f" {macro_file.name}:{lineno} ({label}): {line.strip()}") + + assert not offenders, ( + "use adapter.quote() or the relation object so generated SQL keeps one " + "quoting style (#785):\n" + "\n".join(offenders) + )