Merge pull request #821 from Benjamin-Knight/fix/view-skip-refreshview

fix(view): sp_refreshview when a view rebuild is skipped

Author: axellpadilla

CHANGELOG.md

@@ -15,6 +15,7 @@
 
 #### Bugfixes
 
+- 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)

dbt/include/sqlserver/macros/materializations/models/view/view.sql

@@ -69,7 +69,21 @@
       {% endif %}
     {% endif %}
     {% if should_skip_view_update %}
-      {% set build_sql = 'declare @dbt_sqlserver_noop int;' %}
+      {#- The view's SQL text is unchanged, so the CREATE/ALTER is skipped -
+          but a *referenced* table can still have changed shape (columns
+          added, dropped, or reordered) since this view was last built. SQL
+          Server resolves an unqualified `select *` at CREATE/ALTER time and
+          caches the result; skipping that statement here means the cached
+          column list silently goes stale relative to the underlying table,
+          even though this view's own definition never changed. sp_refreshview
+          re-derives that cached metadata from the table's current shape
+          without re-running the CREATE, so a skip stays a skip (no DDL, no
+          grant/deny churn) while the view keeps reporting the right columns. -#}
+      {#- sp_refreshview resolves its argument in the *current* database, so this needs
+          the same USE prefix every other name-resolving statement here carries -
+          without it a cross-database view model would refresh nothing and error. -#}
+      {% set object_name = "quotename('" ~ target_relation.schema ~ "') + '.' + quotename('" ~ target_relation.identifier ~ "')" %}
+      {% set build_sql = get_use_database_sql(target_relation.database) ~ " declare @dbt_sqlserver_refresh_target nvarchar(max) = " ~ object_name ~ "; exec sp_refreshview @dbt_sqlserver_refresh_target;" %}
     {% else %}
       {% set build_sql = get_create_view_as_sql(target_relation, sql) %}
     {% endif %}

tests/functional/adapter/mssql/test_materialize_change.py

@@ -1,6 +1,6 @@
 import pytest
 
-from dbt.tests.util import get_connection, run_dbt, write_file
+from dbt.tests.util import get_connection, run_dbt, run_dbt_and_capture, write_file
 
 model_sql = """
 SELECT 1 AS data
@@ -60,6 +60,14 @@
 SELECT 'abc' AS source
 """
 
+# A view over a table created outside dbt, so the table survives between runs and
+# can change shape underneath the view. `select *` is expanded and cached at
+# CREATE/ALTER VIEW time, which is what the skip path has to keep in sync.
+select_star_view = """
+{{ config(materialized='view') }}
+SELECT * FROM {{ this.schema }}.refresh_source
+"""
+
 schema = """
 version: 2
 models:
@@ -141,7 +149,14 @@ def test_public_select_grant_survives_swap(self, project):
 
 
 class TestViewMaterializationNoOp(BaseTableView):
-    """Test that rerunning an unchanged view avoids altering the view."""
+    """Test that rerunning an unchanged view avoids rebuilding the view.
+
+    ``modify_date`` used to be the signal here, but the skip path now runs
+    ``sp_refreshview`` (see TestSkippedViewRefreshesSelectStarColumns), which bumps
+    ``modify_date`` just as an ``ALTER`` would - so that column no longer separates a
+    skip from a rebuild. The emitted SQL does: a skip issues ``sp_refreshview`` and
+    no ``CREATE OR ALTER VIEW``, and leaves the stored definition byte-identical.
+    """
 
     @pytest.fixture(scope="class")
     def models(self):
@@ -150,32 +165,16 @@ def models(self):
     def test_unchanged_view_does_not_alter(self, project):
         self.create_object(project, f"CREATE VIEW {project.test_schema}.mat_object AS {model_sql}")
 
-        before_modify_date = project.run_sql(
-            f"""
-            select modify_date
-            from sys.objects o
-            join sys.schemas s on o.schema_id = s.schema_id
-            where upper(s.name) = upper('{project.test_schema}')
-              and upper(o.name) = upper('mat_object')
-            """,
-            fetch="one",
-        )[0]
+        before_definition = _stored_view_definition(project)
 
-        results = run_dbt(["run"])
+        results, log_output = run_dbt_and_capture(["--debug", "run"])
         assert len(results) == 1
 
-        after_modify_date = project.run_sql(
-            f"""
-            select modify_date
-            from sys.objects o
-            join sys.schemas s on o.schema_id = s.schema_id
-            where upper(s.name) = upper('{project.test_schema}')
-              and upper(o.name) = upper('mat_object')
-            """,
-            fetch="one",
-        )[0]
+        emitted_sql = log_output.lower()
+        assert "sp_refreshview" in emitted_sql
+        assert "create or alter view" not in emitted_sql
 
-        assert after_modify_date == before_modify_date
+        assert _stored_view_definition(project) == before_definition
 
 
 class TestViewtoTable(BaseTableView):
@@ -251,3 +250,48 @@ def test_case_only_change_lands(self, project):
             project.run_sql(f"select source from {project.test_schema}.mat_object", fetch="one")[0]
             == "abc"
         )
+
+
+def _view_columns(project):
+    """The view's cached column list, in order, as SQL Server currently reports it."""
+    rows = project.run_sql(
+        f"""
+        select c.name
+        from sys.columns c
+        where c.object_id = object_id('{project.test_schema}.mat_object')
+        order by c.column_id
+        """,
+        fetch="all",
+    )
+    return [row[0] for row in rows]
+
+
+class TestSkippedViewRefreshesSelectStarColumns(BaseTableView):
+    """A skipped rebuild must still re-derive a cached ``select *`` column list.
+
+    SQL Server expands an unqualified ``select *`` at CREATE/ALTER VIEW time and caches
+    the result. When the model SQL is unchanged the CREATE is skipped, so a column added
+    to a referenced table never reached the view: it kept serving the old shape - old
+    names in old positions - while every dbt run reported success. The skip path runs
+    ``sp_refreshview``, which re-derives that metadata without any DDL on the view.
+    """
+
+    @pytest.fixture(scope="class")
+    def models(self):
+        return {"mat_object.sql": select_star_view, "schema.yml": schema}
+
+    def test_added_column_reaches_skipped_view(self, project):
+        self.create_object(project, f"CREATE TABLE {project.test_schema}.refresh_source (a int)")
+
+        run_dbt(["run"])
+        assert _view_columns(project) == ["a"]
+
+        project.run_sql(f"alter table {project.test_schema}.refresh_source add b int")
+
+        # The model SQL is unchanged, so the CREATE/ALTER is skipped ...
+        results, log_output = run_dbt_and_capture(["--debug", "run"])
+        assert len(results) == 1
+        assert "create or alter view" not in log_output.lower()
+
+        # ... but the view still reports the table's current shape.
+        assert _view_columns(project) == ["a", "b"]