From 358b34eece262c1a012022670b6aa8b7cc03cb5a Mon Sep 17 00:00:00 2001 From: Leon Lee Date: Sat, 29 Aug 2026 21:57:19 +0800 Subject: [PATCH 1/2] fix(constraints): emit column and model constraints instead of dropping them Only `not_null` ever reached the database. `render_column_constraint` returned an empty string for every other type, and the macro its docstring pointed at as the place those were applied instead, `sqlserver__build_model_constraints`, was defined but called from nowhere, so model-level constraints were discarded outright. Where a constraint lands depends on whether it is named. An unnamed one renders inline in the CREATE TABLE column list, validated as the table is built, so a violation fails before the swap and leaves the previous table untouched. A named model-level constraint is applied by ALTER TABLE ... ADD CONSTRAINT after the build swaps the new table in and drops the old one, which is the first point at which the name is free: SQL Server scopes constraint names per schema (unlike index names, which are per table), so a name emitted inline would collide with the table being replaced on every rebuild after the first. A name on a column-level constraint is ignored with a warning pointing at the model-level form, and a foreign key that names no target warns rather than vanishing. Each ADD is guarded on the name already being present on the table, so the macro runs on every build path: a constraint added to a model that already exists lands on its next run instead of waiting for --full-refresh. A redefinition under an unchanged name is not detected - a constraint name, unlike a dbt_idx_ index name, is not a hash of its definition - and needs --full-refresh. Both are documented, as is the asymmetry that an unnamed constraint added to a model whose table persists is a silent no-op until then. Column-level CHECK constraints are hoisted into table-level clauses of the same CREATE TABLE: SQL Server accepts only one column-level CHECK per column. PRIMARY KEY and UNIQUE default to NONCLUSTERED so they coexist with the clustered columnstore index built for as_columnstore; dbt's own `expression` field overrides that, and anything other than those two keywords is rejected with a compile error rather than emitted as DDL that cannot parse. Foreign keys match the `to` / `to_columns` form as well as the older free-text `expression`, which was the only one recognised before. Unit-test fixture tables keep rendering `not_null` only, so a UNIQUE or FOREIGN KEY off the real contract cannot fail a unit test on stand-in data. Fixes #579 Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 3 + README.md | 96 ++++ dbt/adapters/sqlserver/sqlserver_adapter.py | 233 +++++++-- .../models/incremental/incremental.sql | 4 + .../models/table/columns_spec_ddl.sql | 81 ++- .../materializations/models/table/table.sql | 4 + .../unit_test/unit_test_create_table_as.sql | 6 +- .../adapter/dbt/test_constraints.py | 4 +- .../adapter/mssql/test_constraints_applied.py | 484 ++++++++++++++++++ tests/unit/adapters/mssql/test_constraints.py | 202 ++++++++ 10 files changed, 1069 insertions(+), 48 deletions(-) create mode 100644 tests/functional/adapter/mssql/test_constraints_applied.py create mode 100644 tests/unit/adapters/mssql/test_constraints.py diff --git a/CHANGELOG.md b/CHANGELOG.md index c35f48f76..f23b4cec7 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -15,6 +15,9 @@ #### Bugfixes +- Fix `primary_key`, `foreign_key`, `unique` and `check` constraints declared in a contract-enforced model's yaml never reaching the database - only `not_null` was ever emitted. `render_column_constraint` returned an empty string for every other type, and `sqlserver__build_model_constraints`, which its docstring pointed at as the place those were applied instead, was defined but called from nowhere, so model-level constraints were dropped outright. Column-level constraints now render inline in the `CREATE TABLE` column list, and model-level constraints render there too when they carry no `name:`. A model-level constraint *with* a `name:` is applied by `ALTER TABLE ... ADD CONSTRAINT` once the build has swapped the new table in and dropped the old one, which is the first point at which that name is free: SQL Server scopes constraint names per schema (unlike index names, which are scoped per table), so emitting a name inline would collide with the table being replaced on every rebuild after the first. A `name:` on a *column-level* constraint is ignored with a warning pointing at the model-level form. `PRIMARY KEY` and `UNIQUE` default to `NONCLUSTERED` so they can coexist with the clustered columnstore index built for `as_columnstore` (the default); declare `expression: clustered` on the constraint to override that. Foreign keys accept both the `to:` / `to_columns:` form and the older free-text `expression:` form - previously only the latter was matched, so even a wired-up model constraint using `to:` would have been silently discarded. Note that a foreign key pointing at a model makes that model's rebuild fail with `Msg 3726` while the swap's backup table is dropped; set `table_refresh_method: dml` on the referenced model to keep its table object across refreshes. [#579](https://github.com/dbt-msft/dbt-sqlserver/issues/579) +- Apply named model constraints on every build path rather than only on the build that creates the table. Each `ALTER TABLE ... ADD CONSTRAINT` is now guarded on the name already being present on the table, so a constraint added to an existing `incremental` model (or to a model using `table_refresh_method: dml`) lands on its next run instead of silently doing nothing until `--full-refresh`. A constraint whose *definition* changes under an unchanged name is still not detected - a constraint name, unlike a `dbt_idx_` index name, is not a hash of its definition - and needs `--full-refresh`; see the README. + - Fix a `view` model whose SQL text is unchanged (and so skips its `CREATE`/`ALTER`) going stale when a table it selects `*` from gains, loses, or reorders columns. SQL Server resolves an unqualified `select *` and caches the result at `CREATE`/`ALTER VIEW` time; skipping that statement left the cached column list silently out of sync with the underlying table, so the view kept serving old columns under their old names/positions even though every dbt run reported success. The skip path now runs `sp_refreshview` against the view instead of a no-op, which re-derives the cached metadata from the table's current shape without reissuing the `CREATE`. The refresh carries the model's database as a `USE` prefix, so a cross-database view model refreshes the intended object rather than erroring on a name that does not resolve in the connection's current database. Note that a metadata refresh advances the view's `sys.objects.modify_date` just as an `ALTER` would, so that column no longer distinguishes a skipped run from a rebuild. - Fix a `view` model silently skipping a rebuild when text was removed from the *start* of its body (e.g. deleting a leading comment or CTE). The skip test compared the stored definition against the model with `endswith()`, so any edit whose new body was a tail of the old one looked unchanged: `dbt run` reported `PASS` but the change never reached the database, and `--full-refresh` did not fix it. The header (`CREATE [OR ALTER] VIEW AS`) is now split off at its separating ` AS ` and the body compared exactly. The comparison also no longer lowercases or strips whitespace, both of which made genuinely different bodies (a string literal differing only in case, or any literal containing spaces) compare equal; where the definition cannot be parsed with certainty the view is rebuilt rather than skipped. - Fix snapshots failing on their second and later runs with `Invalid object name '..._dbt_tmp'`, and contract-enforced models silently losing their in-transaction `pre_hook` writes. `get_column_schema_from_query` reads a query's column shape by executing it, then returned without fetching the rows or closing the cursor. Closing a cursor whose result set the server is still producing makes the driver cancel the request, and SQL Server answers that cancel by rolling back the open transaction, since every connection runs `SET XACT_ABORT ON` (#718). Nothing is raised for any of it, so the snapshot lost the staging table it had just built and failed against it a statement later. The probe now drains and closes its cursor, as does the row-count probe in `expand_column_types`. Only queries opening with a CTE were affected - anything else is wrapped as `select * from (...) where 1 = 0` by `sqlserver__get_empty_subquery_sql` and returns no rows - which is why snapshot staging queries (`with snapshot_query as ...`, both `check` and `timestamp` strategies) and CTE-headed contract models were the ones that broke. diff --git a/README.md b/README.md index bb134f524..e7e023cbd 100644 --- a/README.md +++ b/README.md @@ -269,6 +269,102 @@ You can also set it per model: With `table_refresh_method: dml`, a schema change makes the refresh fall back to a rename-swap. On that run — and only that run — the scratch table is rebuilt through `CREATE TABLE … INSERT … WITH (TABLOCK)`, so it carries the model's columnstore index, and under an enforced contract its `NOT NULL`s, into the swap. That run therefore executes the model's SQL twice — once for the `SELECT … INTO` that probes for the schema change, once for the rebuild. Steady-state refreshes are unaffected and keep the single, cheaper `SELECT … INTO`. +### Constraints + +Constraints declared in a model's yaml are applied when — and only when — the model's [contract](https://docs.getdbt.com/reference/resource-configs/contract) is enforced, which is what every dbt adapter does and keeps their cost opt-in. (dbt-core does not raise if you declare constraints with the contract off — they are simply never emitted.) `not_null`, `check`, `unique`, `primary_key` and `foreign_key` are all supported. + +**Where a constraint lands depends on whether you name it.** + +An unnamed constraint is rendered inline in the `CREATE TABLE` column list and SQL Server names it (`PK__my_model__3213E83F…`). It is validated as the table is built, so a violation fails before the new table is swapped in and the previous one is left untouched. + +A *model-level* constraint carrying `name:` is applied by `ALTER TABLE … ADD CONSTRAINT` after the build swaps the new table into place and drops the old one. SQL Server scopes constraint names per schema, and a table is built alongside the one it replaces, so that is the first moment the name is free to reuse — naming a constraint inline would collide with the outgoing table (`Msg 2714`) on every rebuild after the first. The trade-off is that this runs after the model has committed: if the data violates the constraint, the model fails with the table already in place but unconstrained, and — as with any failure this late in a build — `post_hook`s declared with `transaction: false` do not run. + +Name a constraint when you want it stable across environments (schema-comparison tools report the generated names as differences) or need to reference it later. A `name:` on a *column-level* constraint is ignored with a warning — declare it under the model's `constraints:` key instead. + +```yaml +models: + - name: fact_sales + config: + contract: + enforced: true + constraints: + # named: applied by ALTER TABLE after the swap + - type: primary_key + name: PK_fact_sales + columns: [sale_id] + - type: foreign_key + name: FK_fact_sales_customer + columns: [customer_id] + to: ref('dim_customer') + to_columns: [customer_id] + # unnamed: rendered into the CREATE TABLE + - type: check + expression: amount >= 0 + columns: + - name: sale_id + data_type: int + constraints: + - type: not_null + - name: customer_id + data_type: int + constraints: + - type: not_null + - name: amount + data_type: decimal(18,2) +``` + +#### Clustering + +`primary_key` and `unique` are emitted as `NONCLUSTERED` by default so they can coexist with the clustered columnstore index built for [`as_columnstore`](#as_columnstore). Use dbt's own `expression` field to ask for something else: + +```yaml + constraints: + - type: primary_key + name: PK_fact_sales + columns: [sale_id] + expression: clustered # requires as_columnstore: false +``` + +`clustered` and `nonclustered` are the only values understood here. `expression` is free text that dbt splices between the keyword and the column list, which is the one place T-SQL accepts nothing else — index options such as `with (fillfactor = 90)` belong on a separate index, not on the constraint. + +#### Foreign keys + +Two things are worth knowing before adding them: + +- **They are not free at build time.** Every load is validated against them; add them where you want the guarantee, not everywhere the relationship exists. +- **A foreign key pointing at a model blocks that model's rebuild.** The build renames the outgoing table to a backup and drops it, but the child's foreign key follows the renamed object, so the drop fails with `Msg 3726`. SQL Server has no `DROP TABLE … CASCADE`. + + The adapter ships a macro for exactly this, meant as a `pre_hook` on the **referenced** (parent) model: + + ```sql + {{ config(pre_hook="{{ drop_fk_constraints() }}") }} + ``` + + It drops the foreign keys in both directions — the inbound ones other tables hold against this model, and this model's own outbound ones — so the rebuild's backup drop succeeds. The trade-off is explicit and worth stating: **the child's foreign key does not exist between the parent's rebuild and the child's next build.** (dbt-postgres makes the same trade silently, by issuing every `drop table` with `cascade`.) + + `table_refresh_method: dml` is *not* a workaround. Its steady-state refresh issues `DELETE FROM `, which fails with `Msg 547` as soon as the child holds referencing rows, and its schema-change path falls back to the same rename-swap, hitting `Msg 3726` anyway. + +- **SQL Server has no cross-database foreign keys.** `to: ref(...)` resolves to a fully qualified relation, database included, which SQL Server accepts as long as it names the current database. A target in another database fails with `references invalid table`. + +A foreign key also does not order the build on its own: add an explicit `-- depends_on: {{ ref('dim_customer') }}` to the child model so the parent is built first. + +#### Changing a constraint after the first build + +Named constraints are applied by an `ALTER TABLE` whose `ADD` is guarded on the name already being present on the table, so: + +- **Adding** a constraint to a model that already exists lands on its next run — no `--full-refresh` needed, on `table` and `incremental` alike. +- **Changing** an existing constraint's definition while keeping its name is *not* detected: a constraint name, unlike a `dbt_idx_` index name, says nothing about what the constraint does. Run `--full-refresh` to apply the new definition; that rebuilds the table, so the constraint is created fresh. (A `table` model rebuilds on every run and needs nothing special.) +- **Renaming** a constraint on a table that persists across runs adds the new name beside the old one. For a `check`, `unique` or `foreign_key` that means a duplicate; for a `primary_key` the run fails with `Msg 1779` (*table already has a primary key defined on it*) until the old one is dropped. Rename with `--full-refresh`. +- **Removing** a constraint from the yaml does not drop it from the database. Drop it yourself, or `--full-refresh`. + +Every bullet above is about *named* constraints. Unnamed ones ride the `CREATE TABLE`, so they follow the table and change only when the table is rebuilt — on a materialization whose table persists across runs (`incremental` in its steady state, `table_refresh_method: dml`), adding an unnamed constraint to an existing model does nothing until a `--full-refresh`, and the run still reports success. Name it, or full-refresh. + +#### Build-shape notes + +An *unnamed* `primary_key` or `unique` constraint on a column that also carries a [data mask](#dynamic-data-masking-masked_with--masks) is rejected by the build. The constraint rides the `CREATE TABLE`, so its index already exists by the time `apply_masks` runs, and the adapter refuses to mask any column that an index has as a key: *is configured for masking but is also an index key column*. Declare that constraint at the model level **with a `name:`** instead — named constraints are applied by `ALTER TABLE` after the masks are in place, which the adapter allows. + +With `full_refresh_build: prebuilt`, a `primary_key` or `unique` constraint creates a nonclustered index on the table *before* the bulk load. That secondary index has to be maintained row by row during `INSERT … WITH (TABLOCK)`, which is fully logged and adds to a load that `prebuilt` exists to make cheap. If a model is on `prebuilt` because its load time matters, weigh the key constraints against that; `check`, `not_null` and `foreign_key` do not create indexes and do not carry this cost. + ### Dynamic Data Masking (`masked_with` / `masks`) The adapter can apply SQL Server [Dynamic Data Masking](https://learn.microsoft.com/en-us/sql/relational-databases/security/dynamic-data-masking) (DDM) to columns as part of the materialization, so masks are re-applied on every build and survive dbt's drop-and-recreate on a full refresh. A principal granted `SELECT` but not `UNMASK` then sees masked values instead of real data (dbt's own build principal, being `db_owner`, keeps `UNMASK` and reads real data). Requires **SQL Server 2016+**. diff --git a/dbt/adapters/sqlserver/sqlserver_adapter.py b/dbt/adapters/sqlserver/sqlserver_adapter.py index c74514fce..1fa7be23e 100644 --- a/dbt/adapters/sqlserver/sqlserver_adapter.py +++ b/dbt/adapters/sqlserver/sqlserver_adapter.py @@ -45,6 +45,13 @@ logger = AdapterLogger("SQLServer") +# The constraint types this adapter renders itself rather than deferring to +# dbt-adapters: each needs SQL Server's CLUSTERED/NONCLUSTERED choice or its +# own column quoting. +_KEYED_CONSTRAINTS = frozenset( + {ConstraintType.unique, ConstraintType.primary_key, ConstraintType.foreign_key} +) + # Mirrors sqlserver__select_starts_with_cte # (dbt/include/sqlserver/macros/adapters/columns.sql): a query opening with a # CTE cannot be neutered as ``select * from (...) where 1 = 0``, so it reaches @@ -541,45 +548,213 @@ def run_sql_for_tests(self, sql, fetch, conn): finally: conn.transaction_open = False + @classmethod + def _clustering(cls, keyword: str, expression: str) -> str: + """Render a PRIMARY KEY / UNIQUE keyword with its index clustering. + + SQL Server defaults an unqualified PRIMARY KEY to CLUSTERED, which is + incompatible with the clustered columnstore index the adapter builds + for ``as_columnstore`` (the default), so NONCLUSTERED is the safe + default here. dbt's own ``expression`` field is the override - a + constraint declaring ``expression: clustered`` keeps what it asked for + - so no adapter-specific yaml key is needed. + + Those two keywords are the only thing T-SQL accepts in this position, + so anything else is rejected here rather than emitted as DDL that + cannot parse. + """ + clustering = expression.strip() + if not clustering: + return f"{keyword} {SQLServerIndexType.default()}" + if clustering.lower() not in ( + SQLServerIndexType.clustered, + SQLServerIndexType.nonclustered, + ): + raise dbt_common.exceptions.DbtValidationError( + f"Invalid expression '{expression}' on a {keyword} constraint. " + f"SQL Server accepts only '{SQLServerIndexType.clustered}' or " + f"'{SQLServerIndexType.nonclustered}' here; index options belong on a " + "separate index, not on the constraint." + ) + return f"{keyword} {clustering}" + + @classmethod + def _render_foreign_key_target(cls, constraint: ColumnLevelConstraint) -> Optional[str]: + """The ``references ()`` tail of a foreign key. + + Accepts both the ``to`` / ``to_columns`` form and the older free-text + ``expression`` form; returns None when neither is usable. + + dbt-core resolves ``to: ref(...)`` to a fully rendered relation, which + for this adapter includes the database. That three-part name is passed + through as-is: SQL Server resolves it fine while it names the current + database, and a genuinely cross-database target - which SQL Server does + not support for foreign keys - then fails with its own clear "references + invalid table" error rather than being silently rewritten to point at a + same-named table in this database. + """ + if constraint.to and constraint.to_columns: + columns = ", ".join(cls.quote(column) for column in constraint.to_columns) + return f"references {constraint.to} ({columns})" + if constraint.expression: + return f"references {constraint.expression}" + logger.warning( + f"Dropping the {constraint.type.value} constraint" + + (f" '{constraint.name}'" if constraint.name else "") + + ": it names no target. Declare `to:` together with `to_columns:`, " + "or the free-text `expression:` form (`
()`)." + ) + return None + + @classmethod + def _render_keyed_constraint( + cls, constraint: ColumnLevelConstraint, column_list: str = "" + ) -> Optional[str]: + """The constraint types this adapter renders differently from dbt-adapters. + + Shared by the column-level and model-level renderers, which differ only + in whether they name their columns: a column-level constraint is written + against the column it follows, a model-level one carries its own list. + """ + expression = constraint.expression or "" + suffix = f" ({column_list})" if column_list else "" + + if constraint.type == ConstraintType.unique: + return cls._clustering("unique", expression) + suffix + if constraint.type == ConstraintType.primary_key: + return cls._clustering("primary key", expression) + suffix + target = cls._render_foreign_key_target(constraint) + if target is None: + return None + return f"foreign key ({column_list}) {target}" if column_list else target + @available @classmethod def render_column_constraint(cls, constraint: ColumnLevelConstraint) -> Optional[str]: - """Render NOT NULL inline; every other constraint type renders empty. + """Render a column-level constraint inline in the CREATE TABLE column list. + + Column-level constraints are always anonymous: the table is built as + ``__dbt_tmp`` while the previous one still exists, and SQL + Server scopes constraint names per schema, so a name reused across + builds collides (Msg 2714). Declare the constraint at the model level + to name it - those are applied by ALTER after the swap, where the old + name is already gone. + + Only UNIQUE, PRIMARY KEY and FOREIGN KEY need SQL Server treatment; NOT + NULL, CHECK and CUSTOM fall through to dbt-adapters so they stay in step + with upstream. + """ + if constraint.name: + logger.warning( + f"Ignoring the name '{constraint.name}' on the column-level " + f"{constraint.type.value} constraint: SQL Server scopes constraint names " + "per schema, so naming one inline collides with the table being replaced. " + "Declare the constraint under the model's `constraints:` key to name it." + ) + + if constraint.type in _KEYED_CONSTRAINTS: + return cls._render_keyed_constraint(constraint) + return super().render_column_constraint(constraint) - CHECK, UNIQUE, PRIMARY KEY and FOREIGN KEY are emitted separately as - ALTER TABLE ADD CONSTRAINT (sqlserver__build_model_constraints), so - they contribute nothing to the column DDL. + @available + @classmethod + def render_raw_columns_constraints( + cls, raw_columns: Dict[str, Dict[str, Any]], only_not_null: bool = False + ) -> List[str]: + """Render the column DDL for a CREATE TABLE column list. + + A fork of the dbt-adapters loop of the same name, for two reasons. + CHECK constraints are hoisted out of the column definition and returned + as standalone table-level clauses: SQL Server accepts only one + column-level CHECK per column ("More than one column CHECK constraint + specified for column ..."), while the table-level form has no such + limit. Both are anonymous and both live inside the same CREATE TABLE. + + ``only_not_null`` drops everything but NOT NULL, for the unit-test + fixture tables: their rows are hand-written stand-ins for the model's + data, so a UNIQUE, PRIMARY KEY or FOREIGN KEY copied off the contract + would fail the unit test on data that was never meant to satisfy it. """ - return "not null" if constraint.type == ConstraintType.not_null else "" + rendered_columns = [] + table_level_clauses = [] + + for column in raw_columns.values(): + name = cls.quote(column["name"]) if column.get("quote") else column["name"] + parts = [f"{name} {column['data_type']}"] + for raw_constraint in column.get("constraints") or []: + constraint = cls._parse_column_constraint(raw_constraint) + if only_not_null and constraint.type != ConstraintType.not_null: + continue + clause = cls.process_parsed_constraint(constraint, cls.render_column_constraint) + if not clause: + continue + if constraint.type == ConstraintType.check: + table_level_clauses.append(clause) + else: + parts.append(clause) + rendered_columns.append(" ".join(parts)) + + return rendered_columns + table_level_clauses @classmethod def render_model_constraint(cls, constraint: ModelLevelConstraint) -> Optional[str]: - constraint_prefix = "add constraint " - column_list = ", ".join(constraint.columns) - - if constraint.name is None: - raise dbt_common.exceptions.DbtDatabaseError( - "Constraint name cannot be empty. Provide constraint name - column " - + column_list - + " and run the project again." - ) + """Render an unnamed model-level constraint inline in the column list. - if constraint.type == ConstraintType.unique: - return constraint_prefix + f"{constraint.name} unique nonclustered({column_list})" - elif constraint.type == ConstraintType.primary_key: - return constraint_prefix + f"{constraint.name} primary key nonclustered({column_list})" - elif constraint.type == ConstraintType.foreign_key and constraint.expression: - return ( - constraint_prefix - + f"{constraint.name} foreign key({column_list}) references " - + constraint.expression - ) - elif constraint.type == ConstraintType.check and constraint.expression: - return f"{constraint_prefix} {constraint.name} check ({constraint.expression})" - elif constraint.type == ConstraintType.custom and constraint.expression: - return f"{constraint_prefix} {constraint.name} {constraint.expression}" - else: + A named one renders nothing here and is applied afterwards by + ``render_raw_model_alter_constraints`` instead - see + ``sqlserver__build_model_constraints``. + """ + if constraint.name: return None + return cls._render_model_constraint_body(constraint) + + @available + @classmethod + def render_raw_model_alter_constraints( + cls, raw_constraints: List[Dict[str, Any]] + ) -> List[Dict[str, str]]: + """The *named* model constraints, as ``{name, clause}`` pairs. + + ``clause`` is an ``add constraint ...`` tail for ALTER TABLE; + ``name`` is the bare name, which the macro needs as a string literal to + test ``sys.objects`` before adding it. These are applied once the build + has swapped the new table into place and dropped the old one, which is + the only point at which the name is free to reuse. + """ + clauses = [] + for raw_constraint in raw_constraints: + constraint = cls._parse_model_constraint(raw_constraint) + if not constraint.name: + continue + body = cls.process_parsed_constraint(constraint, cls._render_model_constraint_body) + if body: + clauses.append( + { + "name": constraint.name, + "clause": f"add constraint {cls.quote(constraint.name)} {body}", + } + ) + return clauses + + @classmethod + def _render_model_constraint_body(cls, constraint: ModelLevelConstraint) -> Optional[str]: + """The constraint itself, without any name - valid both in a CREATE + TABLE column list and after ``ALTER TABLE ... ADD CONSTRAINT ``. + + The base renderer cannot stand in here: it prefixes ``constraint + `` whenever the constraint is named, which the ALTER form already + carries. + """ + expression = constraint.expression or "" + + if constraint.type in _KEYED_CONSTRAINTS: + column_list = ", ".join(cls.quote(column) for column in constraint.columns) + return cls._render_keyed_constraint(constraint, column_list) + if constraint.type == ConstraintType.check and expression: + return f"check ({expression})" + if constraint.type == ConstraintType.custom and expression: + return expression + return None def _get_row_count(self, relation) -> int: """Return the number of rows in the given relation.""" diff --git a/dbt/include/sqlserver/macros/materializations/models/incremental/incremental.sql b/dbt/include/sqlserver/macros/materializations/models/incremental/incremental.sql index 174f42f5d..a1ec40a0b 100644 --- a/dbt/include/sqlserver/macros/materializations/models/incremental/incremental.sql +++ b/dbt/include/sqlserver/macros/materializations/models/incremental/incremental.sql @@ -194,6 +194,10 @@ {% do adapter.drop_relation(rel) %} {% endfor %} + {#-- Named model-level constraints. After the to_drop loop, so the backup no + longer holds the old names. See sqlserver__build_model_constraints. --#} + {{ build_model_constraints(target_relation) }} + {{ run_hooks(post_hooks, inside_transaction=False) }} {{ return({'relations': [target_relation]}) }} diff --git a/dbt/include/sqlserver/macros/materializations/models/table/columns_spec_ddl.sql b/dbt/include/sqlserver/macros/materializations/models/table/columns_spec_ddl.sql index e545dbadf..53757e1f9 100644 --- a/dbt/include/sqlserver/macros/materializations/models/table/columns_spec_ddl.sql +++ b/dbt/include/sqlserver/macros/materializations/models/table/columns_spec_ddl.sql @@ -1,12 +1,20 @@ -{% macro build_columns_constraints(relation) %} - {{ return(adapter.dispatch('build_columns_constraints', 'dbt')(relation)) }} +{% macro build_columns_constraints(relation, only_not_null=False) %} + {{ return(adapter.dispatch('build_columns_constraints', 'dbt')(relation, only_not_null)) }} {% endmacro %} -{% macro sqlserver__build_columns_constraints(relation) %} - {# loop through user_provided_columns to create DDL with data types and constraints #} - {%- set raw_column_constraints = adapter.render_raw_columns_constraints(raw_columns=model['columns']) -%} +{% macro sqlserver__build_columns_constraints(relation, only_not_null=False) %} + {#- The parenthesised column list for CREATE TABLE. Carries every column-level + constraint plus the *unnamed* model-level ones: both are anonymous, so + SQL Server names them itself and nothing collides with the table this + build is about to replace. Named model-level constraints are applied + afterwards by build_model_constraints. -#} + {%- set raw_column_constraints = adapter.render_raw_columns_constraints( + raw_columns=model['columns'], only_not_null=only_not_null) -%} + {%- set raw_model_constraints = [] if only_not_null + else adapter.render_raw_model_constraints( + raw_constraints=model.get('constraints') or []) -%} ( - {% for c in raw_column_constraints -%} + {% for c in raw_column_constraints + raw_model_constraints -%} {{ c }}{{ "," if not loop.last }} {% endfor %} ) @@ -17,14 +25,55 @@ {% endmacro %} {% macro sqlserver__build_model_constraints(relation) %} - {# loop through user_provided_columns to create DDL with data types and constraints #} - {%- set raw_model_constraints = adapter.render_raw_model_constraints(raw_constraints=model['constraints']) -%} - {% for c in raw_model_constraints -%} - {% set alter_table_script %} - alter table {{ relation.include(database=False) }} {{c}}; - {%endset%} - {% call statement('alter_table_add_constraint') -%} - {{alter_table_script}} - {%- endcall %} - {% endfor -%} + {#- Named model-level constraints, applied once the build has swapped the new + table into place and dropped the old one: SQL Server scopes constraint + names per schema, so the name is only free to reuse after the table that + held it is gone. + + Each ADD is guarded on the name already being present on this table, so + the macro is safe to call on every build path, including the ones that + keep the existing table (a plain incremental run, a DML refresh). That + makes a constraint added to an existing model land on the next run rather + than waiting for a full refresh. + + What the guard cannot see is a constraint whose *definition* changed under + an unchanged name: unlike an index name (a hash of its definition), a + constraint name says nothing about what the constraint does. Redefining + one needs --full-refresh, which rebuilds the table and so applies the new + definition to a table that carries none. This is documented in the README. + + Emitted as a single batch: one round trip regardless of how many + constraints the model declares. -#} + {%- set contract_config = config.get('contract') -%} + {%- if not contract_config or not contract_config.enforced -%} + {{ return('') }} + {%- endif -%} + + {%- set constraints = adapter.render_raw_model_alter_constraints( + raw_constraints=model.get('constraints') or []) -%} + {%- if not constraints -%} + {{ return('') }} + {%- endif -%} + + {%- set object_id_literal = escape_single_quotes(relation.include(database=False)) -%} + {%- set alter_sql -%} + {{ get_use_database_sql(relation.database) }} + {%- for constraint in constraints %} + if not exists ( + select 1 + from sys.objects {{ information_schema_hints() }} + where name = '{{ escape_single_quotes(constraint['name']) }}' + and parent_object_id = OBJECT_ID('{{ object_id_literal }}') + ) + begin + alter table {{ relation.include(database=False) }} {{ constraint['clause'] }}; + end + {%- endfor %} + {%- endset %} + + {#- auto_begin=False: this runs after the materialization's adapter.commit(), + so opening the ambient transaction here would leave one dangling. -#} + {% call statement('alter_table_add_constraints', auto_begin=False) -%} + {{ alter_sql }} + {%- endcall %} {% endmacro %} diff --git a/dbt/include/sqlserver/macros/materializations/models/table/table.sql b/dbt/include/sqlserver/macros/materializations/models/table/table.sql index 690841be4..dd2e3083c 100644 --- a/dbt/include/sqlserver/macros/materializations/models/table/table.sql +++ b/dbt/include/sqlserver/macros/materializations/models/table/table.sql @@ -143,6 +143,10 @@ {{ drop_relation_if_exists(backup_relation) }} {% endif %} + {#-- Named model-level constraints, on every build path; guarded, so a no-op + where they already exist. See sqlserver__build_model_constraints. --#} + {{ build_model_constraints(target_relation) }} + {{ run_hooks(post_hooks, inside_transaction=False) }} {{ return({'relations': [target_relation]}) }} 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 218bebfb1..6adeb3b2c 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 @@ -32,7 +32,11 @@ {%- elif not is_nested_cte and contract_config.enforced %} CREATE TABLE {{relation}} - {{ build_columns_constraints(relation) }} + {#- only_not_null: the fixture rows are hand-written stand-ins for the + model's real data, so a UNIQUE / PRIMARY KEY / FOREIGN KEY copied + off the contract would fail the unit test on data that was never + meant to satisfy it. -#} + {{ build_columns_constraints(relation, only_not_null=True) }} {{ get_assert_columns_equivalent(sql) }} {% set listColumns %} diff --git a/tests/functional/adapter/dbt/test_constraints.py b/tests/functional/adapter/dbt/test_constraints.py index e628805e6..e5344cf2b 100644 --- a/tests/functional/adapter/dbt/test_constraints.py +++ b/tests/functional/adapter/dbt/test_constraints.py @@ -473,7 +473,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), check ((id > 0)), check (id >= 1) ) INSERT INTO WITH (TABLOCK) ( "id", "color", "date_day" ) SELECT "id", "color", "date_day" FROM ') EXEC('DROP VIEW IF EXISTS """ # EXEC('DROP view IF EXISTS @@ -593,7 +593,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), check ((id > 0)), check (id >= 1) ) 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): diff --git a/tests/functional/adapter/mssql/test_constraints_applied.py b/tests/functional/adapter/mssql/test_constraints_applied.py new file mode 100644 index 000000000..f88e7c47f --- /dev/null +++ b/tests/functional/adapter/mssql/test_constraints_applied.py @@ -0,0 +1,484 @@ +"""Constraints declared in yaml must reach the database. + +The dbt-owned tests in tests/functional/adapter/dbt/test_constraints.py assert +the *generated SQL*; these assert what actually exists in the catalog after a +run, which is what #579 was about - the DDL was simply never emitted. +""" + +import pytest + +from dbt.tests.util import run_dbt, run_dbt_and_capture, write_file + +# A model-level constraint carrying `name:` is applied by ALTER TABLE after the +# build swaps the new table in; an unnamed one rides the CREATE TABLE column +# list and is named by SQL Server. Both shapes appear here. +# +# Every model below selects the same two columns and differs only in its config, +# so they are built from one body. +MODEL_BODY = "select 1 as id, 'blue' as color" + + +def model_sql(**config): + settings = ["materialized='table'"] + settings += [f"{key}={value!r}" for key, value in config.items()] + return "{{ config(" + ", ".join(settings) + ") }}\n" + MODEL_BODY + + +named_model_sql = model_sql() + +named_schema_yml = """ +version: 2 +models: + - name: named_model + config: + contract: + enforced: true + constraints: + - type: primary_key + name: PK_named_model + columns: [id] + - type: unique + name: UQ_named_model_color + columns: [color] + - type: check + name: CK_named_model_id + expression: id > 0 + columns: + - name: id + data_type: int + constraints: + - type: not_null + - name: color + data_type: varchar(100) +""" + +anonymous_model_sql = model_sql() + +anonymous_schema_yml = """ +version: 2 +models: + - name: anonymous_model + config: + contract: + enforced: true + constraints: + - type: unique + columns: [color] + columns: + - name: id + data_type: int + constraints: + - type: not_null + - type: primary_key + - type: check + expression: id > 0 + - name: color + data_type: varchar(100) +""" + +# as_columnstore=False leaves the clustered slot free, so `expression: clustered` +# has something to claim. +clustered_model_sql = model_sql(as_columnstore=False) + +clustered_schema_yml = """ +version: 2 +models: + - name: clustered_model + config: + contract: + enforced: true + constraints: + - type: primary_key + name: PK_clustered_model + columns: [id] + expression: clustered + columns: + - name: id + data_type: int + constraints: + - type: not_null + - name: color + data_type: varchar(100) +""" + +incremental_model_sql = """ +{{ config(materialized='incremental', unique_key='id', on_schema_change='append_new_columns') }} +select 1 as id, 'blue' as color +""" + +incremental_schema_without_pk_yml = """ +version: 2 +models: + - name: incremental_model + config: + contract: + enforced: true + columns: + - name: id + data_type: int + constraints: + - type: not_null + - name: color + data_type: varchar(100) +""" + +# The same model with a primary key, so a test can add the constraint to a model +# that already exists in the database. +incremental_schema_yml = incremental_schema_without_pk_yml.replace( + " columns:", + """ constraints: + - type: primary_key + name: PK_incremental_model + columns: [id] + columns:""", +) + +named_column_constraint_schema_yml = """ +version: 2 +models: + - name: anonymous_model + config: + contract: + enforced: true + columns: + - name: id + data_type: int + constraints: + - type: not_null + - type: primary_key + name: PK_you_cannot_have_this + - name: color + data_type: varchar(100) +""" + + +fk_parent_sql = """ +{{ config(materialized='table', as_columnstore=False) }} +select 1 as id +""" + +fk_child_sql = """ +{{ config(materialized='table', as_columnstore=False) }} + +-- depends_on: {{ ref('fk_parent') }} + +select 1 as parent_id, 'blue' as color +""" + +fk_schema_yml = """ +version: 2 +models: + - name: fk_parent + config: + contract: + enforced: true + constraints: + - type: primary_key + name: PK_fk_parent + columns: [id] + columns: + - name: id + data_type: int + constraints: + - type: not_null + - name: fk_child + config: + contract: + enforced: true + constraints: + - type: foreign_key + name: FK_fk_child_parent + columns: [parent_id] + to: ref('fk_parent') + to_columns: [id] + columns: + - name: parent_id + data_type: int + constraints: + - type: not_null + - name: color + data_type: varchar(100) +""" + + +def _constraints(project, table): + """Every constraint object on a table, as {type: [names]}.""" + rows = project.run_sql( + f""" + select o.type_desc, o.name + from sys.objects o + where o.parent_object_id = OBJECT_ID('{project.test_schema}.{table}') + and o.type in ('PK', 'UQ', 'C', 'F') + """, + fetch="all", + ) + grouped: dict = {} + for type_desc, name in rows: + grouped.setdefault(type_desc, []).append(name) + return grouped + + +def _indexes(project, table): + """Every index on a table, as {name: type_desc}. A clustered columnstore + index has no name of its own worth asserting on, so it keys on None.""" + rows = project.run_sql( + f""" + select i.name, i.type_desc + from sys.indexes i + where i.object_id = OBJECT_ID('{project.test_schema}.{table}') + """, + fetch="all", + ) + return {name: type_desc for name, type_desc in rows} + + +def _index_type(project, table, index_name): + return _indexes(project, table).get(index_name) + + +class TestNamedModelConstraints: + @pytest.fixture(scope="class") + def models(self): + return { + "named_model.sql": named_model_sql, + "schema.yml": named_schema_yml, + } + + def test_named_constraints_are_created_with_their_names(self, project): + run_dbt(["run"]) + + constraints = _constraints(project, "named_model") + assert constraints.get("PRIMARY_KEY_CONSTRAINT") == ["PK_named_model"] + assert constraints.get("UNIQUE_CONSTRAINT") == ["UQ_named_model_color"] + assert constraints.get("CHECK_CONSTRAINT") == ["CK_named_model_id"] + + # The default as_columnstore build carries a clustered columnstore index, + # so the key constraints must be nonclustered to coexist with it. + assert _index_type(project, "named_model", "PK_named_model") == "NONCLUSTERED" + + def test_rebuild_reuses_the_same_constraint_names(self, project): + """The names only survive a rebuild because they are applied after the + old table (which held them) is dropped - the point of the ALTER path.""" + run_dbt(["run"]) + run_dbt(["run", "--full-refresh"]) + + constraints = _constraints(project, "named_model") + assert constraints.get("PRIMARY_KEY_CONSTRAINT") == ["PK_named_model"] + assert constraints.get("CHECK_CONSTRAINT") == ["CK_named_model_id"] + + +class TestAnonymousConstraints: + @pytest.fixture(scope="class") + def models(self): + return { + "anonymous_model.sql": anonymous_model_sql, + "schema.yml": anonymous_schema_yml, + } + + def test_unnamed_constraints_are_created_inline(self, project): + run_dbt(["run"]) + + constraints = _constraints(project, "anonymous_model") + # One column-level primary key and check, one model-level unique. SQL + # Server names them itself, so assert the shape, not the names. + assert len(constraints.get("PRIMARY_KEY_CONSTRAINT", [])) == 1 + assert len(constraints.get("UNIQUE_CONSTRAINT", [])) == 1 + assert len(constraints.get("CHECK_CONSTRAINT", [])) == 1 + + def test_rebuild_does_not_collide(self, project): + run_dbt(["run"]) + run_dbt(["run", "--full-refresh"]) + + constraints = _constraints(project, "anonymous_model") + assert len(constraints.get("PRIMARY_KEY_CONSTRAINT", [])) == 1 + + +class TestNamedColumnConstraintWarns: + @pytest.fixture(scope="class") + def models(self): + return { + "anonymous_model.sql": anonymous_model_sql, + "schema.yml": named_column_constraint_schema_yml, + } + + def test_column_level_name_is_ignored_with_a_warning(self, project): + _, log_output = run_dbt_and_capture(["run"]) + + assert "PK_you_cannot_have_this" in log_output + assert "constraints:" in log_output + + constraints = _constraints(project, "anonymous_model") + # The constraint is still created, just not under the requested name. + created = constraints.get("PRIMARY_KEY_CONSTRAINT", []) + assert len(created) == 1 + assert created[0] != "PK_you_cannot_have_this" + + +class TestClusteredOverride: + @pytest.fixture(scope="class") + def models(self): + return { + "clustered_model.sql": clustered_model_sql, + "schema.yml": clustered_schema_yml, + } + + def test_expression_selects_the_clustering(self, project): + run_dbt(["run"]) + + assert _index_type(project, "clustered_model", "PK_clustered_model") == "CLUSTERED" + + +class TestIncrementalConstraints: + @pytest.fixture(scope="class") + def models(self): + return { + "incremental_model.sql": incremental_model_sql, + "schema.yml": incremental_schema_yml, + } + + def test_constraint_survives_a_second_run(self, project): + """A named constraint is added on the build that creates the table and + must not be re-added on the next incremental run - doing so fails with + Msg 2714 (there is already an object named ...).""" + run_dbt(["run"]) + assert _constraints(project, "incremental_model").get("PRIMARY_KEY_CONSTRAINT") == [ + "PK_incremental_model" + ] + + run_dbt(["run"]) + assert _constraints(project, "incremental_model").get("PRIMARY_KEY_CONSTRAINT") == [ + "PK_incremental_model" + ] + + def test_constraint_is_reapplied_on_full_refresh(self, project): + run_dbt(["run"]) + run_dbt(["run", "--full-refresh"]) + + assert _constraints(project, "incremental_model").get("PRIMARY_KEY_CONSTRAINT") == [ + "PK_incremental_model" + ] + + +class TestConstraintAddedToAnExistingModel: + """A constraint added after the model already exists must land on the next + run. Only a build that creates the table applies constraints, so without + the existence-guarded ADD this was a silent no-op until --full-refresh. + + One test per class: these rewrite models/schema.yml and the `project` + fixture is class-scoped, so a second test here would inherit both the + rewritten file and the constraint left behind on the database. + """ + + @pytest.fixture(scope="class") + def models(self): + return { + "incremental_model.sql": incremental_model_sql, + "schema.yml": incremental_schema_without_pk_yml, + } + + def test_it_lands_on_the_next_incremental_run(self, project): + run_dbt(["run"]) + assert _constraints(project, "incremental_model").get("PRIMARY_KEY_CONSTRAINT") is None + + write_file(incremental_schema_yml, "models", "schema.yml") + + # A plain incremental run, no --full-refresh. + run_dbt(["run"]) + assert _constraints(project, "incremental_model").get("PRIMARY_KEY_CONSTRAINT") == [ + "PK_incremental_model" + ] + + # And a further run must not try to add it a second time (Msg 2714). + run_dbt(["run"]) + assert _constraints(project, "incremental_model").get("PRIMARY_KEY_CONSTRAINT") == [ + "PK_incremental_model" + ] + + +class TestForeignKeyToRef: + """`to: ref(...)` is the form dbt-core actually produces, and it resolves to + a fully rendered relation - database included. T-SQL's REFERENCES grammar + takes [schema.]table only, so the database qualifier has to come off.""" + + @pytest.fixture(scope="class") + def models(self): + return { + "fk_parent.sql": fk_parent_sql, + "fk_child.sql": fk_child_sql, + "schema.yml": fk_schema_yml, + } + + def test_the_foreign_key_is_created(self, project): + run_dbt(["run"]) + + assert _constraints(project, "fk_child").get("FOREIGN_KEY_CONSTRAINT") == [ + "FK_fk_child_parent" + ] + + referenced = project.run_sql( + f""" + select OBJECT_NAME(fk.referenced_object_id) + from sys.foreign_keys fk + where fk.name = 'FK_fk_child_parent' + and fk.parent_object_id = OBJECT_ID('{project.test_schema}.fk_child') + """, + fetch="all", + ) + assert referenced[0][0] == "fk_parent" + + +masked_model_sql = model_sql(as_columnstore=False) + +masked_schema_yml = """ +version: 2 +models: + - name: masked_model + config: + contract: + enforced: true + masks: + id: "default()" + constraints: + - type: primary_key + name: PK_masked_model + columns: [id] + columns: + - name: id + data_type: int + constraints: + - type: not_null + - name: color + data_type: varchar(100) +""" + + +class TestNamedConstraintOnAMaskedColumn: + """A named constraint is applied after apply_masks, so its index lands on an + already-masked column. An unnamed one rides the CREATE TABLE and its index + exists before the masks do, which apply_masks refuses - naming it is the + documented way out.""" + + @pytest.fixture(scope="class") + def models(self): + return { + "masked_model.sql": masked_model_sql, + "schema.yml": masked_schema_yml, + } + + def test_the_mask_and_the_constraint_coexist(self, project): + run_dbt(["run"]) + + assert _constraints(project, "masked_model").get("PRIMARY_KEY_CONSTRAINT") == [ + "PK_masked_model" + ] + + masked = project.run_sql( + f""" + select c.name + from sys.masked_columns c + where c.object_id = OBJECT_ID('{project.test_schema}.masked_model') + and c.is_masked = 1 + """, + fetch="all", + ) + assert [row[0] for row in masked] == ["id"] diff --git a/tests/unit/adapters/mssql/test_constraints.py b/tests/unit/adapters/mssql/test_constraints.py new file mode 100644 index 000000000..6b99a1d16 --- /dev/null +++ b/tests/unit/adapters/mssql/test_constraints.py @@ -0,0 +1,202 @@ +from unittest import mock + +import pytest +from dbt_common.contracts.constraints import ( + ColumnLevelConstraint, + ConstraintType, + ModelLevelConstraint, +) +from dbt_common.exceptions import DbtValidationError + +from dbt.adapters.sqlserver import sqlserver_adapter +from dbt.adapters.sqlserver.sqlserver_adapter import SQLServerAdapter + + +def render_column(**kwargs): + return SQLServerAdapter.render_column_constraint(ColumnLevelConstraint(**kwargs)) + + +def render_model(**kwargs): + return SQLServerAdapter.render_model_constraint(ModelLevelConstraint(**kwargs)) + + +class TestRenderColumnConstraint: + @pytest.mark.parametrize( + "constraint,expected", + [ + ({"type": ConstraintType.not_null}, "not null"), + ({"type": ConstraintType.check, "expression": "id > 0"}, "check (id > 0)"), + ({"type": ConstraintType.unique}, "unique nonclustered"), + ({"type": ConstraintType.primary_key}, "primary key nonclustered"), + ({"type": ConstraintType.custom, "expression": "default 0"}, "default 0"), + # A check with no predicate has nothing to render. + ({"type": ConstraintType.check}, None), + ], + ) + def test_renders_inline(self, constraint, expected): + assert render_column(**constraint) == expected + + @pytest.mark.parametrize("clustering", ["clustered", "nonclustered", " CLUSTERED "]) + def test_expression_chooses_the_clustering(self, clustering): + """PRIMARY KEY / UNIQUE default to NONCLUSTERED so they can coexist with + the clustered columnstore index, but dbt's own `expression` overrides.""" + assert ( + render_column(type=ConstraintType.primary_key, expression=clustering) + == f"primary key {clustering.strip()}" + ) + + @pytest.mark.parametrize("expression", ["clustered_thing", "with (fillfactor = 90)"]) + def test_anything_but_the_two_keywords_is_rejected(self, expression): + """Those two are the only thing T-SQL accepts between the keyword and + the column list, so a typo fails at compile rather than as a syntax + error inside an EXEC() string.""" + with pytest.raises(DbtValidationError, match="Invalid expression"): + render_column(type=ConstraintType.primary_key, expression=expression) + + def test_foreign_key_from_to_and_to_columns(self): + """dbt-core renders `to: ref(...)` to a fully qualified relation, which + for this adapter carries the database. SQL Server accepts a three-part + REFERENCES target naming the current database, so it is passed through + unchanged.""" + assert ( + render_column( + type=ConstraintType.foreign_key, + to='"mydb"."dbo"."dim"', + to_columns=["id"], + ) + == 'references "mydb"."dbo"."dim" ("id")' + ) + + def test_a_name_on_not_null_is_ignored_with_a_warning(self): + """not_null is a column attribute with no name of its own; the README + promises a warning for a name on any column-level constraint. + + AdapterLogger goes through dbt's event system rather than stdlib + logging, so the logger is patched instead of capturing output. + """ + with mock.patch.object(sqlserver_adapter, "logger") as patched: + assert render_column(type=ConstraintType.not_null, name="NN_id") == "not null" + patched.warning.assert_called_once() + assert "NN_id" in patched.warning.call_args.args[0] + + def test_foreign_key_from_expression(self): + assert ( + render_column(type=ConstraintType.foreign_key, expression="dbo.dim (id)") + == "references dbo.dim (id)" + ) + + def test_foreign_key_without_a_target_renders_nothing(self): + assert render_column(type=ConstraintType.foreign_key) is None + + def test_a_name_is_ignored_but_the_constraint_is_still_rendered(self): + """Column-level constraints are always anonymous: the build creates the + table alongside the one it replaces, and constraint names are unique per + schema, so a name here would collide on the next build.""" + assert ( + render_column(type=ConstraintType.primary_key, name="PK_x") + == "primary key nonclustered" + ) + + +class TestRenderRawColumnsConstraints: + raw_columns = { + "id": { + "name": "id", + "data_type": "int", + "constraints": [ + {"type": "not_null"}, + {"type": "primary_key"}, + {"type": "check", "expression": "id > 0"}, + ], + }, + "color": {"name": "color", "data_type": "varchar(100)", "constraints": []}, + } + + def test_renders_every_constraint(self): + assert SQLServerAdapter.render_raw_columns_constraints(self.raw_columns) == [ + "id int not null primary key nonclustered", + "color varchar(100)", + # CHECK is hoisted to a table-level clause: SQL Server allows only + # one column-level CHECK per column. + "check (id > 0)", + ] + + def test_not_null_only_drops_the_rest(self): + """Unit-test fixture tables take this path: their rows are stand-ins, so + a UNIQUE or FOREIGN KEY off the real contract would fail on data that was + never meant to satisfy it.""" + assert SQLServerAdapter.render_raw_columns_constraints( + self.raw_columns, only_not_null=True + ) == ["id int not null", "color varchar(100)"] + + +class TestRenderModelConstraint: + def test_unnamed_constraints_render_inline(self): + assert ( + render_model(type=ConstraintType.primary_key, columns=["id", "region"]) + == 'primary key nonclustered ("id", "region")' + ) + + def test_named_constraints_render_nothing_inline(self): + """They are applied by ALTER after the swap instead.""" + assert render_model(type=ConstraintType.primary_key, name="PK_x", columns=["id"]) is None + + +class TestRenderRawModelAlterConstraints: + def test_only_named_constraints_are_altered_in(self): + clauses = SQLServerAdapter.render_raw_model_alter_constraints( + [ + {"type": "primary_key", "columns": ["id"]}, + {"type": "primary_key", "name": "PK_m", "columns": ["id", "region"]}, + {"type": "unique", "name": "UQ_m", "columns": ["email"]}, + {"type": "check", "name": "CK_m", "expression": "id > 0", "columns": []}, + { + "type": "foreign_key", + "name": "FK_m", + "columns": ["dim_id"], + "to": "dbo.dim", + "to_columns": ["id"], + }, + { + "type": "foreign_key", + "name": "FK_legacy", + "columns": ["dim_id"], + "expression": "dbo.dim (id)", + }, + ] + ) + assert clauses == [ + { + "name": "PK_m", + "clause": 'add constraint "PK_m" primary key nonclustered ("id", "region")', + }, + {"name": "UQ_m", "clause": 'add constraint "UQ_m" unique nonclustered ("email")'}, + {"name": "CK_m", "clause": 'add constraint "CK_m" check (id > 0)'}, + { + "name": "FK_m", + "clause": 'add constraint "FK_m" foreign key ("dim_id") references dbo.dim ("id")', + }, + { + "name": "FK_legacy", + "clause": ( + 'add constraint "FK_legacy" foreign key ("dim_id") references dbo.dim (id)' + ), + }, + ] + + def test_the_bare_name_is_returned_for_the_existence_guard(self): + """The macro tests sys.objects for the name before adding it, so it + needs the name unquoted as well as inside the clause.""" + clauses = SQLServerAdapter.render_raw_model_alter_constraints( + [{"type": "primary_key", "name": "PK_m", "columns": ["id"]}] + ) + assert clauses[0]["name"] == "PK_m" + assert clauses[0]["clause"].startswith('add constraint "PK_m" ') + + def test_clustering_override_applies_to_the_alter_form_too(self): + assert SQLServerAdapter.render_raw_model_alter_constraints( + [{"type": "primary_key", "name": "PK_m", "columns": ["id"], "expression": "clustered"}] + ) == [{"name": "PK_m", "clause": 'add constraint "PK_m" primary key clustered ("id")'}] + + def test_no_constraints_renders_nothing(self): + assert SQLServerAdapter.render_raw_model_alter_constraints([]) == [] From e0f1730cbfce721826e0f00d74d9d92608d3a1c4 Mon Sep 17 00:00:00 2001 From: Leon Lee Date: Sun, 30 Aug 2026 19:35:36 +0800 Subject: [PATCH 2/2] test(constraints): cover the dml refresh's rename-swap keeping constraints table_refresh_method: dml falls back to a rename-swap whenever the model's schema changes, and that swap used to land a table built by SELECT * INTO, which carries no constraint and no NOT NULL. The rebuild that fixes it is a separate change; this asserts what it means for a contract-enforced model - that the named PRIMARY KEY, the inline CHECK and the NOT NULLs are all still on the table after a column is added. Also extends the columnstore/NOT NULL note in the as_columnstore section, and the corresponding changelog entry, to name the inline constraints that only carry across the swap once this change is in. Co-Authored-By: Claude Opus 5 --- CHANGELOG.md | 2 +- README.md | 2 +- .../adapter/mssql/test_constraints_applied.py | 94 +++++++++++++++++++ 3 files changed, 96 insertions(+), 2 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f23b4cec7..3f7a9ad7e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,7 +24,7 @@ - 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) - Fix the `sqlserver__openquery` macro to quote linked-server names through `adapter.quote()`, keeping its generated identifier style consistent with the rest of the adapter. -- Fix a model with `table_refresh_method: dml` losing its clustered columnstore index - and, under an enforced contract, its `NOT NULL`s as well - the first time its schema changed. That path builds a scratch table and, when the columns no longer match, renames it into position; the scratch was built with `SELECT * INTO`, which copies no index and no constraint and infers nullability from the query rather than from the contract, and `create_indexes` only builds what the `indexes` config names, never the `as_columnstore` CCI. So the model came back stripped and stayed that way, since every later run matched the new schema and took the DELETE+INSERT path. That branch now rebuilds the scratch table through `create_table_as` - the way every other build path in the adapter creates a table - which carries the full column DDL and the columnstore index across the swap. On a schema-change run - and only there - that costs a second execution of the model's SQL (once for the `SELECT ... INTO` schema probe, once for the rebuild) and one extra columnstore build. It gives up the minimally-logged `SELECT ... INTO` for that run, though `INSERT ... WITH (TABLOCK)` is itself minimally logged under the simple and bulk-logged recovery models, so the extra log volume lands on full-recovery databases only. Steady-state refreshes are unchanged. +- Fix a model with `table_refresh_method: dml` losing its clustered columnstore index - and, under an enforced contract, its constraints and `NOT NULL`s as well - the first time its schema changed. That path builds a scratch table and, when the columns no longer match, renames it into position; the scratch was built with `SELECT * INTO`, which copies no index and no constraint and infers nullability from the query rather than from the contract, and `create_indexes` only builds what the `indexes` config names, never the `as_columnstore` CCI. So the model came back stripped and stayed that way, since every later run matched the new schema and took the DELETE+INSERT path. That branch now rebuilds the scratch table through `create_table_as` - the way every other build path in the adapter creates a table - which carries the full column DDL and the columnstore index across the swap. On a schema-change run - and only there - that costs a second execution of the model's SQL (once for the `SELECT ... INTO` schema probe, once for the rebuild) and one extra columnstore build. It gives up the minimally-logged `SELECT ... INTO` for that run, though `INSERT ... WITH (TABLOCK)` is itself minimally logged under the simple and bulk-logged recovery models, so the extra log volume lands on full-recovery databases only. Steady-state refreshes are unchanged. #### Under the hood diff --git a/README.md b/README.md index e7e023cbd..981487f54 100644 --- a/README.md +++ b/README.md @@ -267,7 +267,7 @@ You can also set it per model: {{ config(materialized="table", as_columnstore=false) }} ``` -With `table_refresh_method: dml`, a schema change makes the refresh fall back to a rename-swap. On that run — and only that run — the scratch table is rebuilt through `CREATE TABLE … INSERT … WITH (TABLOCK)`, so it carries the model's columnstore index, and under an enforced contract its `NOT NULL`s, into the swap. That run therefore executes the model's SQL twice — once for the `SELECT … INTO` that probes for the schema change, once for the rebuild. Steady-state refreshes are unaffected and keep the single, cheaper `SELECT … INTO`. +With `table_refresh_method: dml`, a schema change makes the refresh fall back to a rename-swap. On that run — and only that run — the scratch table is rebuilt through `CREATE TABLE … INSERT … WITH (TABLOCK)`, so it carries the model's columnstore index, and under an enforced contract its `NOT NULL`s and inline constraints, into the swap. That run therefore executes the model's SQL twice — once for the `SELECT … INTO` that probes for the schema change, once for the rebuild. Steady-state refreshes are unaffected and keep the single, cheaper `SELECT … INTO`. ### Constraints diff --git a/tests/functional/adapter/mssql/test_constraints_applied.py b/tests/functional/adapter/mssql/test_constraints_applied.py index f88e7c47f..c9991453a 100644 --- a/tests/functional/adapter/mssql/test_constraints_applied.py +++ b/tests/functional/adapter/mssql/test_constraints_applied.py @@ -152,6 +152,42 @@ def model_sql(**config): """ +dml_model_sql = model_sql(table_refresh_method="dml") + +# Adds a column, which is what pushes the DML refresh onto its rename-swap +# fallback. +dml_model_wider_sql = dml_model_sql + ", 'x' as extra" + +dml_schema_yml = """ +version: 2 +models: + - name: dml_model + config: + contract: + enforced: true + constraints: + - type: primary_key + name: PK_dml_model + columns: [id] + - type: check + expression: id > 0 + columns: + - name: id + data_type: int + constraints: + - type: not_null + - name: color + data_type: varchar(100) +""" + +dml_schema_wider_yml = ( + dml_schema_yml + + """ - name: extra + data_type: varchar(100) +""" +) + + fk_parent_sql = """ {{ config(materialized='table', as_columnstore=False) }} select 1 as id @@ -236,6 +272,23 @@ def _index_type(project, table, index_name): return _indexes(project, table).get(index_name) +def _not_null_columns(project, table): + rows = project.run_sql( + f""" + select c.name + from sys.columns c + where c.object_id = OBJECT_ID('{project.test_schema}.{table}') + and c.is_nullable = 0 + """, + fetch="all", + ) + return sorted(row[0] for row in rows) + + +def _has_columnstore(project, table): + return "CLUSTERED COLUMNSTORE" in _indexes(project, table).values() + + class TestNamedModelConstraints: @pytest.fixture(scope="class") def models(self): @@ -395,6 +448,47 @@ def test_it_lands_on_the_next_incremental_run(self, project): ] +class TestDmlRefreshKeepsConstraints: + """table_refresh_method='dml' rebuilds through a rename-swap whenever the + schema changes. That rebuild used to land a table built by SELECT * INTO, + which carries no constraints, no NOT NULL and no columnstore index. + + One test per class - see TestConstraintAddedToAnExistingModel. + """ + + @pytest.fixture(scope="class") + def models(self): + return { + "dml_model.sql": dml_model_sql, + "schema.yml": dml_schema_yml, + } + + def test_constraints_survive_the_schema_change_fallback(self, project): + run_dbt(["run"]) + before = _constraints(project, "dml_model") + assert before.get("PRIMARY_KEY_CONSTRAINT") == ["PK_dml_model"] + assert len(before.get("CHECK_CONSTRAINT", [])) == 1 + assert _not_null_columns(project, "dml_model") == ["id"] + # Default as_columnstore, so the table is built on a CCI. + assert _has_columnstore(project, "dml_model") + + # A steady-state refresh keeps the table object and everything on it. + run_dbt(["run"]) + assert _constraints(project, "dml_model").get("PRIMARY_KEY_CONSTRAINT") == ["PK_dml_model"] + + # Add a column: the DML refresh cannot swap by DELETE+INSERT any more + # and falls back to rename-swap. + write_file(dml_model_wider_sql, "models", "dml_model.sql") + write_file(dml_schema_wider_yml, "models", "schema.yml") + run_dbt(["run"]) + + after = _constraints(project, "dml_model") + assert after.get("PRIMARY_KEY_CONSTRAINT") == ["PK_dml_model"] + assert len(after.get("CHECK_CONSTRAINT", [])) == 1 + assert _not_null_columns(project, "dml_model") == ["id"] + assert _has_columnstore(project, "dml_model") + + class TestForeignKeyToRef: """`to: ref(...)` is the form dbt-core actually produces, and it resolves to a fully rendered relation - database included. T-SQL's REFERENCES grammar