fix(constraints): emit column and model constraints instead of dropping them

CHANGELOG.md

@@ -15,13 +15,16 @@
 
 #### 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 <name> 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.
 - 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. The rebuild bulk-loads the same way the model would on any other build path - `SELECT ... INTO` for an ordinary model, `CREATE TABLE` plus `INSERT ... WITH (TABLOCK)` under an enforced contract - so it gives up no minimal logging to do it. Steady-state refreshes are unchanged. A table already stripped by this bug is not repaired by upgrading: its schema still matches, so it stays on the DELETE+INSERT path, and index reconciliation protects an existing columnstore index without ever creating a missing one. To rebuild it, temporarily set `full_refresh_build: prebuilt` and run with `--full-refresh`.
+- 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. The rebuild bulk-loads the same way the model would on any other build path - `SELECT ... INTO` for an ordinary model, `CREATE TABLE` plus `INSERT ... WITH (TABLOCK)` under an enforced contract - so it gives up no minimal logging to do it. Steady-state refreshes are unchanged. A table already stripped by this bug is not repaired by upgrading: its schema still matches, so it stays on the DELETE+INSERT path, and index reconciliation protects an existing columnstore index without ever creating a missing one. To rebuild it, temporarily set `full_refresh_build: prebuilt` and run with `--full-refresh`.
 
 #### Under the hood
 

README.md

@@ -267,7 +267,103 @@ 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 the way this adapter builds every other table, 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 — and builds the columnstore index once. Steady-state refreshes are unaffected and keep the single `SELECT … INTO`. A table that lost its columnstore index to this bug before you upgraded is not repaired automatically: its schema still matches, so it stays on the cheap path. To rebuild it, temporarily set `full_refresh_build: prebuilt` and run with `--full-refresh`.
+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 the way this adapter builds every other table, 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 — and builds the columnstore index once. Steady-state refreshes are unaffected and keep the single `SELECT … INTO`. A table that lost its columnstore index to this bug before you upgraded is not repaired automatically: its schema still matches, so it stays on the cheap path. To rebuild it, temporarily set `full_refresh_build: prebuilt` and run with `--full-refresh`.
+
+### 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 <parent>`, 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`)