Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -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 <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 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
Expand Down
28 changes: 24 additions & 4 deletions dbt/include/sqlserver/macros/materializations/models/view/view.sql
Original file line number Diff line number Diff line change
Expand Up @@ -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 <name> AS <body>); 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 <quoted
relation> 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;' %}
Expand Down
92 changes: 91 additions & 1 deletion tests/functional/adapter/mssql/test_materialize_change.py
Original file line number Diff line number Diff line change
@@ -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
Expand Down Expand Up @@ -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:
Expand Down Expand Up @@ -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 <body> 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"
)