From 54e76232082f326ae64e9ac65a5ed0552f734fab Mon Sep 17 00:00:00 2001 From: Ben Knight Date: Mon, 3 Aug 2026 17:36:20 +0000 Subject: [PATCH] fix: compare view body exactly instead of suffix-matching the whole definition sqlserver__view's skip test compared the stored view definition against the model's compiled SQL with normalized_definition.endswith(normalized_sql). The stored definition is the whole statement (CREATE [OR ALTER] VIEW AS ) while the model is only the body, so any edit whose new body is a tail of the old one - most commonly deleting a leading comment or CTE - satisfied endswith() and was silently skipped. dbt run reported PASS, the change never reached the database, and --full-refresh did not fix it; once in that state every subsequent run re-confirmed the skip. Split the header off at its separating ' AS ' (the first one - the quoted relation contains no other) and compare the remainder verbatim. Also drop the | lower and whitespace stripping: both made genuinely different bodies compare equal (where source = 'MAXIMS' vs 'maxims', or any literal containing spaces), turning a missed rebuild into a correctness bug. The comparison follows the asymmetry that a skip failing to fire costs one rebuild while a skip firing wrongly costs correctness - an unparseable definition (no ' AS ') rebuilds rather than guessing. Also removes the dead normalized_relation local, which was computed but never read. Add regression tests: removing a leading comment now lands in the stored definition, and a change confined to the case of a string literal rebuilds. Co-Authored-By: Claude Opus 4.8 (1M context) --- CHANGELOG.md | 2 +- .../materializations/models/view/view.sql | 28 +++++- .../adapter/mssql/test_materialize_change.py | 92 ++++++++++++++++++- 3 files changed, 116 insertions(+), 6 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index e658860ba..b369d32c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,7 +12,7 @@ #### 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 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 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 diff --git a/dbt/include/sqlserver/macros/materializations/models/view/view.sql b/dbt/include/sqlserver/macros/materializations/models/view/view.sql index 3eea0c2d9..29ede4a93 100644 --- a/dbt/include/sqlserver/macros/materializations/models/view/view.sql +++ b/dbt/include/sqlserver/macros/materializations/models/view/view.sql @@ -43,10 +43,30 @@ {% elif existing_relation is not none and existing_relation.type == 'view' %} {% set current_view_definition_table = run_query(get_view_definition_sql(existing_relation)) %} {% if current_view_definition_table is not none and current_view_definition_table.rows | length > 0 %} - {% set normalized_relation = target_relation.include(database=False) | lower | replace('\n', '') | replace('\r', '') | replace('\t', '') | replace(' ', '') | replace(';', '') %} - {% set normalized_sql = sql | lower | replace('\n', '') | replace('\r', '') | replace('\t', '') | replace(' ', '') | replace(';', '') %} - {% set normalized_definition = current_view_definition_table.rows[0][0] | lower | replace('\n', '') | replace('\r', '') | replace('\t', '') | replace(' ', '') | replace(';', '') %} - {% set should_skip_view_update = normalized_definition.endswith(normalized_sql) %} + {#- Compare the view *body* exactly, not by suffix. The stored definition is + the whole statement (CREATE [OR ALTER] VIEW AS ); the model is + only the body. The header ends at the separating ' AS ' - split there and + compare the remainder verbatim. A suffix test (endswith) would wrongly skip + any edit whose new body is a tail of the old one, e.g. deleting a leading + comment or CTE - it lands as PASS but never reaches the database, and + --full-refresh does not fix it. Do NOT lowercase or strip whitespace: both + make genuinely different bodies compare equal (a string literal differing + only in case, or any literal containing spaces). The asymmetry is deliberate - a skip that + fails to fire costs one rebuild; a skip that fires wrongly costs correctness - + so when we cannot be certain, we rebuild. -#} + {% set stored = current_view_definition_table.rows[0][0] %} + {#- First ' as ' is the header/body separator: CREATE [OR ALTER] VIEW AS has no other, the relation being quoted. -#} + {% set marker = (stored | lower).find(' as ') %} + {% if marker < 0 %} + {% set should_skip_view_update = false %} + {% else %} + {% set stored_body = stored[marker + 4:] | replace('\r\n', '\n') | trim %} + {% set stored_body = (stored_body[:-1] if stored_body.endswith(';') else stored_body) | trim %} + {% set model_body = sql | replace('\r\n', '\n') | trim %} + {% set model_body = (model_body[:-1] if model_body.endswith(';') else model_body) | trim %} + {% set should_skip_view_update = stored_body == model_body %} + {% endif %} {% endif %} {% if should_skip_view_update %} {% set build_sql = 'declare @dbt_sqlserver_noop int;' %} diff --git a/tests/functional/adapter/mssql/test_materialize_change.py b/tests/functional/adapter/mssql/test_materialize_change.py index 5137d8d1a..4db4516a0 100644 --- a/tests/functional/adapter/mssql/test_materialize_change.py +++ b/tests/functional/adapter/mssql/test_materialize_change.py @@ -1,6 +1,6 @@ import pytest -from dbt.tests.util import get_connection, run_dbt +from dbt.tests.util import get_connection, run_dbt, write_file model_sql = """ SELECT 1 AS data @@ -33,6 +33,33 @@ SELECT * FROM missing_relation """ +# Same body with and without a leading comment. Removing the comment leaves the +# new body as a *suffix* of the stored definition - the case the old endswith() +# skip test got wrong, skipping the rebuild so the change never reached the db. +view_with_leading_comment = """ +{{ config(materialized='view') }} +-- leading_marker_comment +SELECT 1 AS data +""" + +view_without_leading_comment = """ +{{ config(materialized='view') }} +SELECT 1 AS data +""" + +# Two bodies that differ only by the case of a string literal. Lowercasing before +# comparing (as the old code did) would treat these as identical and skip the +# rebuild - a correctness bug, not just a missed comment. +view_literal_upper = """ +{{ config(materialized='view') }} +SELECT 'ABC' AS source +""" + +view_literal_lower = """ +{{ config(materialized='view') }} +SELECT 'abc' AS source +""" + schema = """ version: 2 models: @@ -161,3 +188,66 @@ def models(self): def test_passes(self, project): self.create_object(project, f"CREATE VIEW {project.test_schema}.mat_object AS {model_sql}") run_dbt(["run"]) + + +def _stored_view_definition(project): + """The whole stored CREATE ... VIEW ... AS statement, as SQL Server keeps it.""" + return project.run_sql( + f"select object_definition(object_id('{project.test_schema}.mat_object'))", + fetch="one", + )[0] + + +class TestViewLeadingTextRemovalReachesDatabase(BaseTableView): + """Removing text from the *start* of a view body must rebuild the view. + + The old skip test compared with ``normalized_definition.endswith(normalized_sql)``. + The stored definition is the whole statement while the model is only the body, + so any edit whose new body is a tail of the old one (e.g. deleting a leading + comment) satisfied endswith() and was silently skipped - PASS, but the change + never reached the database, and --full-refresh did not fix it. + """ + + @pytest.fixture(scope="class") + def models(self): + return {"mat_object.sql": view_with_leading_comment, "schema.yml": schema} + + def test_removal_of_leading_comment_lands(self, project): + run_dbt(["run"]) + assert "leading_marker_comment" in _stored_view_definition(project) + + # Delete the leading comment - the new body is now a suffix of the old. + write_file(view_without_leading_comment, "models", "mat_object.sql") + results = run_dbt(["run"]) + assert len(results) == 1 + + assert "leading_marker_comment" not in _stored_view_definition(project) + + +class TestViewLiteralCaseChangeRebuilds(BaseTableView): + """A change confined to the case of a string literal must rebuild the view. + + The old skip test lowercased both sides before comparing, so ``'ABC'`` and + ``'abc'`` looked identical and the rebuild was skipped - a correctness bug, + since the two views return different data. The exact comparison rebuilds. + """ + + @pytest.fixture(scope="class") + def models(self): + return {"mat_object.sql": view_literal_upper, "schema.yml": schema} + + def test_case_only_change_lands(self, project): + run_dbt(["run"]) + assert ( + project.run_sql(f"select source from {project.test_schema}.mat_object", fetch="one")[0] + == "ABC" + ) + + write_file(view_literal_lower, "models", "mat_object.sql") + results = run_dbt(["run"]) + assert len(results) == 1 + + assert ( + project.run_sql(f"select source from {project.test_schema}.mat_object", fetch="one")[0] + == "abc" + )