diff --git a/CHANGELOG.md b/CHANGELOG.md index dc8551e0..e658860b 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 929c4ad6..a5a63d9d 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 20f5047e..731fbc54 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 a9ccbd9c..fbeb53d8 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 2554656e..c534344e 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 685ec94f..d43ad581 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 32107c1c..53e14a80 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 b1e2258d..75ff593e 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 a248b611..1ac67422 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 64cf6fbc..0ea860ca 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 b1658781..218bebfb 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 17d5d130..bec22465 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 c8b2675b..d94da335 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 be74c2a7..31f2ecfa 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 bd54476f..3ea79c48 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 328c97cb..34459093 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 80f47e59..2c00d251 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 232ccd1b..3d538063 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 00000000..e02f6190 --- /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) + )