Skip to content

Commit

Permalink
first pass: put behind legacy flag
Browse files Browse the repository at this point in the history
  • Loading branch information
MichelleArk committed Sep 4, 2024
1 parent ce2b150 commit bad6b92
Show file tree
Hide file tree
Showing 9 changed files with 124 additions and 23 deletions.
15 changes: 10 additions & 5 deletions core/dbt/context/context_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
from dbt.adapters.factory import get_config_class_by_name
from dbt.config import IsFQNResource, Project, RuntimeConfig
from dbt.contracts.graph.model_config import get_config_for
from dbt.flags import get_flags
from dbt.node_types import NodeType
from dbt.utils import fqn_search
from dbt_common.contracts.config.base import BaseConfig, _listify
Expand Down Expand Up @@ -362,13 +363,17 @@ def build_config_dict(
else:
# TODO CT-211
src = UnrenderedConfigGenerator(self._active_project) # type: ignore[assignment]
# TODO: behaviour flag can route behaviour here.
# Prefer _config_call_dict if it is available and _unrendered_config_call_dict is not,
# as _unrendered_config_call_dict is unreliable for non-sql nodes (e.g. no jinja config block rendered for python models, test nodes, etc)
if self._config_call_dict and not self._unrendered_config_call_dict:

# preserve legacy behaviour - using unreliable (potentially rendered) _config_call_dict
if get_flags().require_config_jinja_insensitivity_for_state_modified is False:
config_call_dict = self._config_call_dict
else:
config_call_dict = self._unrendered_config_call_dict
# Prefer _config_call_dict if it is available and _unrendered_config_call_dict is not,
# as _unrendered_config_call_dict is unreliable for non-sql nodes (e.g. no jinja config block rendered for python models, etc)
if self._config_call_dict and not self._unrendered_config_call_dict:
config_call_dict = self._config_call_dict
else:
config_call_dict = self._unrendered_config_call_dict

return src.calculate_node_config_dict(
config_call_dict=config_call_dict,
Expand Down
4 changes: 3 additions & 1 deletion core/dbt/contracts/project.py
Original file line number Diff line number Diff line change
Expand Up @@ -341,14 +341,16 @@ class ProjectFlags(ExtensibleDbtClassMixin):
warn_error_options: Optional[Dict[str, Union[str, List[str]]]] = None
write_json: Optional[bool] = None

# legacy behaviors
# legacy behaviors - https://github.com/dbt-labs/dbt-core/blob/main/docs/guides/behavior-change-flags.md
require_config_jinja_insensitivity_for_state_modified: bool = False
require_explicit_package_overrides_for_builtin_materializations: bool = True
require_resource_names_without_spaces: bool = False
source_freshness_run_project_hooks: bool = False

@property
def project_only_flags(self) -> Dict[str, Any]:
return {
"require_config_jinja_insensitivity_for_state_modified": self.require_config_jinja_insensitivity_for_state_modified,
"require_explicit_package_overrides_for_builtin_materializations": self.require_explicit_package_overrides_for_builtin_materializations,
"require_resource_names_without_spaces": self.require_resource_names_without_spaces,
"source_freshness_run_project_hooks": self.source_freshness_run_project_hooks,
Expand Down
16 changes: 9 additions & 7 deletions core/dbt/parser/base.py
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
DictParseError,
InvalidAccessTypeError,
)
from dbt.flags import get_flags
from dbt.node_types import AccessType, ModelLanguage, NodeType
from dbt.parser.search import FileBlock
from dbt_common.dataclass_schema import ValidationError
Expand Down Expand Up @@ -372,19 +373,20 @@ def update_parsed_node_config(
if hasattr(parsed_node, "contract"):
parsed_node.contract = Contract.from_dict(contract_dct)

# unrendered_config is used to compare the original database/schema/alias
# values and to handle 'same_config' and 'same_contents' calls
# TODO: Behaviour flag can route here
if patch_file_id:
# Use the patch_file.unrendered_configs if available, as provided patch_config_dict may actuallly already be rendered
if patch_file := self.manifest.files.get(patch_file_id, None):
if isinstance(patch_file, SchemaSourceFile):
if get_flags().require_config_jinja_insensitivity_for_state_modified:
# Use the patch_file.unrendered_configs if available to update patch_dict_config,
# as provided patch_config_dict may actuallly already be rendered and thus sensitive to jinja evaluations
if patch_file_id:
patch_file = self.manifest.files.get(patch_file_id, None)
if patch_file and isinstance(patch_file, SchemaSourceFile):
# TODO: do not hardcode "models"
if unrendered_patch_config := patch_file.get_unrendered_config(
"models", parsed_node.name, getattr(parsed_node, "version", None)
):
patch_config_dict = deep_merge(patch_config_dict, unrendered_patch_config)

# unrendered_config is used to compare the original database/schema/alias
# values and to handle 'same_config' and 'same_contents' calls
parsed_node.unrendered_config = config.build_config_dict(
rendered=False, patch_config_dict=patch_config_dict
)
Expand Down
70 changes: 65 additions & 5 deletions tests/functional/configs/test_configs_in_schema_files.py
Original file line number Diff line number Diff line change
Expand Up @@ -110,6 +110,15 @@


class TestSchemaFileConfigs:
@pytest.fixture(scope="class")
def expected_unrendered_config(self):
# my_attr is unrendered when require_config_jinja_insensitivity_for_state_modified: True
return {
"materialized": "view",
"meta": {"my_attr": "{{ var('my_var') }}", "owner": "Julie Smith"},
"tags": ["tag_1_in_model", "tag_2_in_model"],
}

@pytest.fixture(scope="class")
def models(self):
return {
Expand All @@ -125,6 +134,9 @@ def seeds(self):
@pytest.fixture(scope="class")
def project_config_update(self):
return {
"flags": {
"require_config_jinja_insensitivity_for_state_modified": True,
},
"models": {
"+meta": {
"company": "NuMade",
Expand Down Expand Up @@ -160,6 +172,7 @@ def project_config_update(self):
def test_config_layering(
self,
project,
expected_unrendered_config,
):

# run seed
Expand Down Expand Up @@ -219,11 +232,7 @@ def test_config_layering(
model_node = manifest.nodes[model_id]

assert model_node.config.materialized == "view"
model_unrendered_config = {
"materialized": "view",
"meta": {"my_attr": "{{ var('my_var') }}", "owner": "Julie Smith"},
"tags": ["tag_1_in_model", "tag_2_in_model"],
}
model_unrendered_config = expected_unrendered_config
assert model_node.unrendered_config == model_unrendered_config

# look for test meta
Expand Down Expand Up @@ -251,6 +260,57 @@ def test_config_layering(
run_dbt(["run"])


class TestLegacySchemaFileConfigs(TestSchemaFileConfigs):
@pytest.fixture(scope="class")
def expected_unrendered_config(self):
# my_attr is rendered ("TESTING") when require_config_jinja_insensitivity_for_state_modified: False
return {
"materialized": "view",
"meta": {"my_attr": "TESTING", "owner": "Julie Smith"},
"tags": ["tag_1_in_model", "tag_2_in_model"],
}

@pytest.fixture(scope="class")
def project_config_update(self):
return {
# The uncommented below lines can be removed once the default behaviour is flipped.
# require_config_jinja_insensitivity_for_state_modified defaults to false currently
# "flags": {
# "require_config_jinja_insensitivity_for_state_modified": False,
# },
"models": {
"+meta": {
"company": "NuMade",
},
"test": {
"+meta": {
"project": "test",
},
"tagged": {
"+meta": {
"team": "Core Team",
},
"tags": ["tag_in_project"],
"model": {
"materialized": "table",
"+meta": {
"owner": "Julie Dent",
},
},
},
},
},
"vars": {
"test": {
"my_var": "TESTING",
}
},
"seeds": {
"quote_columns": False,
},
}


list_schema_yml = """
- name: my_name
- name: alt_name
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,14 @@ def setup(self):
yield
del os.environ["DBT_TEST_STATE_MODIFIED"]

@pytest.fixture(scope="class")
def project_config_update(self):
return {
"flags": {
"require_config_jinja_insensitivity_for_state_modified": True,
}
}

def test_change_env_var(self, project):
# Generate ./state without changing environment variable value
run_dbt(["run"])
Expand Down Expand Up @@ -80,11 +88,14 @@ def models(self):
@pytest.fixture(scope="class")
def project_config_update(self):
return {
"flags": {
"require_config_jinja_insensitivity_for_state_modified": True,
},
"models": {
"test": {
"+materialized": "{{ env_var('DBT_TEST_STATE_MODIFIED') }}",
}
}
},
}


Expand Down
1 change: 1 addition & 0 deletions tests/unit/contracts/graph/test_manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -84,6 +84,7 @@
"docs",
"checksum",
"unrendered_config",
"unrendered_config_call_dict",
"created_at",
"config_call_dict",
"relation_name",
Expand Down
4 changes: 4 additions & 0 deletions tests/unit/contracts/graph/test_nodes.py
Original file line number Diff line number Diff line change
Expand Up @@ -139,6 +139,7 @@ def basic_uncompiled_dict():
"checksum": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
},
"unrendered_config": {},
"unrendered_config_call_dict": {},
"config_call_dict": {},
}

Expand Down Expand Up @@ -197,6 +198,7 @@ def basic_compiled_dict():
"checksum": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
},
"unrendered_config": {},
"unrendered_config_call_dict": {},
"config_call_dict": {},
"access": "protected",
"constraints": [],
Expand Down Expand Up @@ -458,6 +460,7 @@ def basic_uncompiled_schema_test_dict():
"checksum": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
},
"unrendered_config": {},
"unrendered_config_call_dict": {},
"config_call_dict": {},
}

Expand Down Expand Up @@ -515,6 +518,7 @@ def basic_compiled_schema_test_dict():
"unrendered_config": {
"severity": "warn",
},
"unrendered_config_call_dict": {},
"config_call_dict": {},
}

Expand Down
11 changes: 11 additions & 0 deletions tests/unit/contracts/graph/test_nodes_parsed.py
Original file line number Diff line number Diff line change
Expand Up @@ -197,6 +197,7 @@ def base_parsed_model_dict():
"checksum": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
},
"unrendered_config": {},
"unrendered_config_call_dict": {},
"config_call_dict": {},
"access": AccessType.Protected.value,
"constraints": [],
Expand Down Expand Up @@ -318,6 +319,7 @@ def complex_parsed_model_dict():
"materialized": "ephemeral",
"post_hook": ['insert into blah(a, b) select "1", 1'],
},
"unrendered_config_call_dict": {},
"config_call_dict": {},
"access": AccessType.Protected.value,
"constraints": [],
Expand Down Expand Up @@ -526,6 +528,7 @@ def basic_parsed_seed_dict():
"meta": {},
"checksum": {"name": "path", "checksum": "seeds/seed.csv"},
"unrendered_config": {},
"unrendered_config_call_dict": {},
"config_call_dict": {},
}

Expand Down Expand Up @@ -630,6 +633,7 @@ def complex_parsed_seed_dict():
"unrendered_config": {
"persist_docs": {"relation": True, "columns": True},
},
"unrendered_config_call_dict": {},
"config_call_dict": {},
}

Expand Down Expand Up @@ -828,6 +832,7 @@ def base_parsed_hook_dict():
"checksum": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
},
"unrendered_config": {},
"unrendered_config_call_dict": {},
"config_call_dict": {},
}

Expand Down Expand Up @@ -920,6 +925,7 @@ def complex_parsed_hook_dict():
"column_types": {"a": "text"},
"materialized": "table",
},
"unrendered_config_call_dict": {},
"config_call_dict": {},
}

Expand Down Expand Up @@ -1013,6 +1019,7 @@ def minimal_parsed_schema_test_dict():
"name": "sha256",
"checksum": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
},
"unrendered_config_call_dict": {},
"config_call_dict": {},
}

Expand Down Expand Up @@ -1063,6 +1070,7 @@ def basic_parsed_schema_test_dict():
"checksum": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
},
"unrendered_config": {},
"unrendered_config_call_dict": {},
"config_call_dict": {},
}

Expand Down Expand Up @@ -1151,6 +1159,7 @@ def complex_parsed_schema_test_dict():
"checksum": "e3b0c44298fc1c149afbf4c8996fb92427ae41e4649b934ca495991b7852b855",
},
"unrendered_config": {"materialized": "table", "severity": "WARN"},
"unrendered_config_call_dict": {},
"config_call_dict": {},
}

Expand Down Expand Up @@ -1535,6 +1544,7 @@ def basic_timestamp_snapshot_dict():
"target_database": "some_snapshot_db",
"target_schema": "some_snapshot_schema",
},
"unrendered_config_call_dict": {},
"config_call_dict": {},
}

Expand Down Expand Up @@ -1637,6 +1647,7 @@ def basic_check_snapshot_dict():
"strategy": "check",
"check_cols": "all",
},
"unrendered_config_call_dict": {},
"config_call_dict": {},
}

Expand Down
13 changes: 9 additions & 4 deletions tests/unit/parser/test_parser.py
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,6 @@

import yaml

import dbt.flags
import dbt.parser
from dbt import tracking
from dbt.artifacts.resources import ModelConfig, RefArgs
from dbt.context.context_config import ContextConfig
Expand Down Expand Up @@ -60,7 +58,9 @@
normalize,
)

set_from_args(Namespace(WARN_ERROR=False), None)
set_from_args(
Namespace(warn_error=False, require_config_jinja_insensitivity_for_state_modified=False), None
)


def get_abs_os_path(unix_path):
Expand Down Expand Up @@ -94,7 +94,12 @@ def _generate_macros(self):
yield pm

def setUp(self):
dbt.flags.WARN_ERROR = True
set_from_args(
Namespace(
warn_error=True, require_config_jinja_insensitivity_for_state_modified=False
),
None,
)
# HACK: this is needed since tracking events can
# be sent when using the model parser
tracking.do_not_track()
Expand Down

0 comments on commit bad6b92

Please sign in to comment.