From b477be9eff1333b080d322c5b4a5b1700fe2e251 Mon Sep 17 00:00:00 2001 From: Jeremy Cohen Date: Thu, 12 Aug 2021 17:51:29 -0400 Subject: [PATCH] Feature: state:modified.macros (#3559) (#3736) * First cut at state:modified for macro changes * First cut at state:modified subselectors * Update test_graph_selector_methods * Fix flake8 * Fix mypy * Update 062_defer_state_test/test_modified_state * PR feedback. Update changelog --- CHANGELOG.md | 3 + core/dbt/contracts/graph/parsed.py | 2 +- core/dbt/graph/selector_methods.py | 97 +++++++++----- .../changed_models/table_model.sql | 3 + .../changed_models_bad/table_model.sql | 3 + .../models/table_model.sql | 3 + .../test_modified_state.py | 6 +- test/unit/test_graph_selector_methods.py | 118 +++++++++++++++++- 8 files changed, 193 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index d07a6437472..f14d6c932d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,8 @@ ## dbt 0.21.0 (Release TBD) +### Features +- Capture changes to macros in `state:modified`. Introduce new `state:` sub-selectors: `modified.body`, `modified.configs`, `modified.persisted_descriptions`, `modified.relation`, `modified.macros` ([#2704](https://github.com/dbt-labs/dbt/issues/2704), [#3278](https://github.com/dbt-labs/dbt/issues/3278), [#3559](https://github.com/dbt-labs/dbt/issues/3559)) + ### Fixes - Fix for RPC requests that raise a RecursionError when serializing Undefined values as JSON ([#3464](https://github.com/dbt-labs/dbt/issues/3464), [#3687](https://github.com/dbt-labs/dbt/pull/3687)) diff --git a/core/dbt/contracts/graph/parsed.py b/core/dbt/contracts/graph/parsed.py index d2dbe48c225..2220ffaf9e9 100644 --- a/core/dbt/contracts/graph/parsed.py +++ b/core/dbt/contracts/graph/parsed.py @@ -692,7 +692,7 @@ def depends_on_nodes(self): @property def depends_on(self): - return {'nodes': []} + return DependsOn(macros=[], nodes=[]) @property def refs(self): diff --git a/core/dbt/graph/selector_methods.py b/core/dbt/graph/selector_methods.py index d18d10cbebe..5fa7e21628b 100644 --- a/core/dbt/graph/selector_methods.py +++ b/core/dbt/graph/selector_methods.py @@ -22,13 +22,11 @@ ParsedSourceDefinition, ) from dbt.contracts.state import PreviousState -from dbt.logger import GLOBAL_LOGGER as logger from dbt.exceptions import ( InternalException, RuntimeException, ) from dbt.node_types import NodeType -from dbt.ui import warning_tag SELECTOR_GLOB = '*' @@ -381,7 +379,7 @@ def search( class StateSelectorMethod(SelectorMethod): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self.macros_were_modified: Optional[List[str]] = None + self.modified_macros: Optional[List[str]] = None def _macros_modified(self) -> List[str]: # we checked in the caller! @@ -394,44 +392,74 @@ def _macros_modified(self) -> List[str]: modified = [] for uid, macro in new_macros.items(): - name = f'{macro.package_name}.{macro.name}' if uid in old_macros: old_macro = old_macros[uid] if macro.macro_sql != old_macro.macro_sql: - modified.append(f'{name} changed') + modified.append(uid) else: - modified.append(f'{name} added') + modified.append(uid) for uid, macro in old_macros.items(): if uid not in new_macros: - modified.append(f'{macro.package_name}.{macro.name} removed') + modified.append(uid) + + return modified + + def recursively_check_macros_modified(self, node): + # check if there are any changes in macros the first time + if self.modified_macros is None: + self.modified_macros = self._macros_modified() + + # loop through all macros that this node depends on + for macro_uid in node.depends_on.macros: + # is this macro one of the modified macros? + if macro_uid in self.modified_macros: + return True + # if not, and this macro depends on other macros, keep looping + macro = self.manifest.macros[macro_uid] + if len(macro.depends_on.macros) > 0: + return self.recursively_check_macros_modified(macro) + else: + return False + return False - return modified[:3] + def check_modified(self, old: Optional[SelectorTarget], new: SelectorTarget) -> bool: + different_contents = not new.same_contents(old) # type: ignore + upstream_macro_change = self.recursively_check_macros_modified(new) + return different_contents or upstream_macro_change - def check_modified( - self, - old: Optional[SelectorTarget], - new: SelectorTarget, + def check_modified_body(self, old: Optional[SelectorTarget], new: SelectorTarget) -> bool: + if hasattr(new, "same_body"): + return not new.same_body(old) # type: ignore + else: + return False + + def check_modified_configs(self, old: Optional[SelectorTarget], new: SelectorTarget) -> bool: + if hasattr(new, "same_config"): + return not new.same_config(old) # type: ignore + else: + return False + + def check_modified_persisted_descriptions( + self, old: Optional[SelectorTarget], new: SelectorTarget ) -> bool: - # check if there are any changes in macros, if so, log a warning the - # first time - if self.macros_were_modified is None: - self.macros_were_modified = self._macros_modified() - if self.macros_were_modified: - log_str = ', '.join(self.macros_were_modified) - logger.warning(warning_tag( - f'During a state comparison, dbt detected a change in ' - f'macros. This will not be marked as a modification. Some ' - f'macros: {log_str}' - )) - - return not new.same_contents(old) # type: ignore - - def check_new( - self, - old: Optional[SelectorTarget], - new: SelectorTarget, + if hasattr(new, "same_persisted_description"): + return not new.same_persisted_description(old) # type: ignore + else: + return False + + def check_modified_relation( + self, old: Optional[SelectorTarget], new: SelectorTarget ) -> bool: + if hasattr(new, "same_database_representation"): + return not new.same_database_representation(old) # type: ignore + else: + return False + + def check_modified_macros(self, _, new: SelectorTarget) -> bool: + return self.recursively_check_macros_modified(new) + + def check_new(self, old: Optional[SelectorTarget], new: SelectorTarget) -> bool: return old is None def search( @@ -443,8 +471,15 @@ def search( ) state_checks = { + # it's new if there is no old version + 'new': lambda old, _: old is None, + # use methods defined above to compare properties of old + new 'modified': self.check_modified, - 'new': self.check_new, + 'modified.body': self.check_modified_body, + 'modified.configs': self.check_modified_configs, + 'modified.persisted_descriptions': self.check_modified_persisted_descriptions, + 'modified.relation': self.check_modified_relation, + 'modified.macros': self.check_modified_macros, } if selector in state_checks: checker = state_checks[selector] diff --git a/test/integration/062_defer_state_test/changed_models/table_model.sql b/test/integration/062_defer_state_test/changed_models/table_model.sql index 07fa333386f..65909318bab 100644 --- a/test/integration/062_defer_state_test/changed_models/table_model.sql +++ b/test/integration/062_defer_state_test/changed_models/table_model.sql @@ -1,2 +1,5 @@ {{ config(materialized='table') }} select * from {{ ref('ephemeral_model') }} + +-- establish a macro dependency to trigger state:modified.macros +-- depends on: {{ my_macro() }} \ No newline at end of file diff --git a/test/integration/062_defer_state_test/changed_models_bad/table_model.sql b/test/integration/062_defer_state_test/changed_models_bad/table_model.sql index 07fa333386f..65909318bab 100644 --- a/test/integration/062_defer_state_test/changed_models_bad/table_model.sql +++ b/test/integration/062_defer_state_test/changed_models_bad/table_model.sql @@ -1,2 +1,5 @@ {{ config(materialized='table') }} select * from {{ ref('ephemeral_model') }} + +-- establish a macro dependency to trigger state:modified.macros +-- depends on: {{ my_macro() }} \ No newline at end of file diff --git a/test/integration/062_defer_state_test/models/table_model.sql b/test/integration/062_defer_state_test/models/table_model.sql index 07fa333386f..65909318bab 100644 --- a/test/integration/062_defer_state_test/models/table_model.sql +++ b/test/integration/062_defer_state_test/models/table_model.sql @@ -1,2 +1,5 @@ {{ config(materialized='table') }} select * from {{ ref('ephemeral_model') }} + +-- establish a macro dependency to trigger state:modified.macros +-- depends on: {{ my_macro() }} \ No newline at end of file diff --git a/test/integration/062_defer_state_test/test_modified_state.py b/test/integration/062_defer_state_test/test_modified_state.py index 00a8f910d6e..0db59c2378d 100644 --- a/test/integration/062_defer_state_test/test_modified_state.py +++ b/test/integration/062_defer_state_test/test_modified_state.py @@ -166,7 +166,6 @@ def test_postgres_new_macro(self): results, stdout = self.run_dbt_and_capture(['run', '--models', 'state:modified', '--state', './state'], strict=False) assert len(results) == 0 - assert 'detected a change in macros' in stdout os.remove('macros/second_macro.sql') # add a new macro to the existing file @@ -175,7 +174,6 @@ def test_postgres_new_macro(self): results, stdout = self.run_dbt_and_capture(['run', '--models', 'state:modified', '--state', './state'], strict=False) assert len(results) == 0 - assert 'detected a change in macros' in stdout @use_profile('postgres') def test_postgres_changed_macro_contents(self): @@ -192,9 +190,9 @@ def test_postgres_changed_macro_contents(self): fp.write('{% endmacro %}') fp.write(newline) + # table_model calls this macro results, stdout = self.run_dbt_and_capture(['run', '--models', 'state:modified', '--state', './state'], strict=False) - assert len(results) == 0 - assert 'detected a change in macros' in stdout + assert len(results) == 1 @use_profile('postgres') def test_postgres_changed_exposure(self): diff --git a/test/unit/test_graph_selector_methods.py b/test/unit/test_graph_selector_methods.py index dafde40e05a..c7c6426e877 100644 --- a/test/unit/test_graph_selector_methods.py +++ b/test/unit/test_graph_selector_methods.py @@ -7,7 +7,9 @@ from dbt.contracts.files import FileHash from dbt.contracts.graph.parsed import ( DependsOn, + MacroDependsOn, NodeConfig, + ParsedMacro, ParsedModelNode, ParsedExposure, ParsedSeedNode, @@ -85,7 +87,7 @@ def make_model(pkg, name, sql, refs=None, sources=None, tags=None, path=None, al tags=tags, refs=ref_values, sources=source_values, - depends_on=DependsOn(nodes=depends_on_nodes), + depends_on=DependsOn(nodes=depends_on_nodes, macros=[]), resource_type=NodeType.Model, checksum=FileHash.from_contents(''), ) @@ -157,6 +159,27 @@ def make_source(pkg, source_name, table_name, path=None, loader=None, identifier ) +def make_macro(pkg, name, macro_sql, path=None, depends_on_macros=None): + if path is None: + path = 'macros/macros.sql' + + if depends_on_macros is None: + depends_on_macros = [] + + return ParsedMacro( + name=name, + macro_sql=macro_sql, + unique_id=f'macro.{pkg}.{name}', + package_name=pkg, + root_path='/usr/dbt/some-project', + path=path, + original_file_path=path, + resource_type=NodeType.Macro, + tags=[], + depends_on=MacroDependsOn(macros=depends_on_macros), + ) + + def make_unique_test(pkg, test_model, column_name, path=None, refs=None, sources=None, tags=None): return make_schema_test(pkg, 'unique', test_model, {}, column_name=column_name) @@ -191,10 +214,10 @@ def make_schema_test(pkg, test_name, test_model, test_kwargs, path=None, refs=No if len(name_parts) == 2: namespace, test_name = name_parts - macro_depends = f'model.{namespace}.{test_name}' + macro_depends = f'macro.{namespace}.test_{test_name}' elif len(name_parts) == 1: namespace = None - macro_depends = f'model.dbt.{test_name}' + macro_depends = f'macro.dbt.test_{test_name}' else: assert False, f'invalid test name: {test_name}' @@ -289,7 +312,7 @@ def make_data_test(pkg, name, sql, refs=None, sources=None, tags=None, path=None tags=tags, refs=ref_values, sources=source_values, - depends_on=DependsOn(nodes=depends_on_nodes), + depends_on=DependsOn(nodes=depends_on_nodes, macros=[]), resource_type=NodeType.Test, checksum=FileHash.from_contents(''), ) @@ -319,6 +342,45 @@ def make_exposure(pkg, name, path=None, fqn_extras=None, owner=None): ) + +@pytest.fixture +def macro_test_unique(): + return make_macro( + 'dbt', + 'test_unique', + 'blablabla', + depends_on_macros=['macro.dbt.default__test_unique'] + ) + + +@pytest.fixture +def macro_default_test_unique(): + return make_macro( + 'dbt', + 'default__test_unique', + 'blablabla' + ) + + +@pytest.fixture +def macro_test_not_null(): + return make_macro( + 'dbt', + 'test_not_null', + 'blablabla', + depends_on_macros=['macro.dbt.default__test_not_null'] + ) + + +@pytest.fixture +def macro_default_test_not_null(): + return make_macro( + 'dbt', + 'default__test_not_null', + 'blabla' + ) + + @pytest.fixture def seed(): return make_seed( @@ -494,16 +556,18 @@ def namespaced_union_model(seed, ext_source): @pytest.fixture def manifest(seed, source, ephemeral_model, view_model, table_model, ext_source, ext_model, union_model, ext_source_2, ext_source_other, ext_source_other_2, table_id_unique, table_id_not_null, view_id_unique, ext_source_id_unique, - view_test_nothing, namespaced_seed, namespace_model, namespaced_union_model): + view_test_nothing, namespaced_seed, namespace_model, namespaced_union_model, macro_test_unique, macro_default_test_unique, + macro_test_not_null, macro_default_test_not_null): nodes = [seed, ephemeral_model, view_model, table_model, union_model, ext_model, table_id_unique, table_id_not_null, view_id_unique, ext_source_id_unique, view_test_nothing, namespaced_seed, namespace_model, namespaced_union_model] sources = [source, ext_source, ext_source_2, ext_source_other, ext_source_other_2] + macros = [macro_test_unique, macro_default_test_unique, macro_test_not_null, macro_default_test_not_null] manifest = Manifest( nodes={n.unique_id: n for n in nodes}, sources={s.unique_id: s for s in sources}, - macros={}, + macros={m.unique_id: m for m in macros}, docs={}, files={}, exposures={}, @@ -696,6 +760,11 @@ def test_select_state_no_change(manifest, previous_state): method = statemethod(manifest, previous_state) assert not search_manifest_using_method(manifest, method, 'modified') assert not search_manifest_using_method(manifest, method, 'new') + assert not search_manifest_using_method(manifest, method, 'new') + assert not search_manifest_using_method(manifest, method, 'modified.configs') + assert not search_manifest_using_method(manifest, method, 'modified.persisted_descriptions') + assert not search_manifest_using_method(manifest, method, 'modified.relation') + assert not search_manifest_using_method(manifest, method, 'modified.macros') def test_select_state_nothing(manifest, previous_state): @@ -722,9 +791,19 @@ def test_select_state_added_model(manifest, previous_state): def test_select_state_changed_model_sql(manifest, previous_state, view_model): change_node(manifest, view_model.replace(raw_sql='select 1 as id')) method = statemethod(manifest, previous_state) + + # both of these assert search_manifest_using_method( manifest, method, 'modified') == {'view_model'} + assert search_manifest_using_method( + manifest, method, 'modified.body') == {'view_model'} + + # none of these assert not search_manifest_using_method(manifest, method, 'new') + assert not search_manifest_using_method(manifest, method, 'modified.configs') + assert not search_manifest_using_method(manifest, method, 'modified.persisted_descriptions') + assert not search_manifest_using_method(manifest, method, 'modified.relation') + assert not search_manifest_using_method(manifest, method, 'modified.macros') def test_select_state_changed_model_fqn(manifest, previous_state, view_model): @@ -813,7 +892,11 @@ def test_select_state_changed_seed_relation_documented(manifest, previous_state, method = statemethod(manifest, previous_state) assert search_manifest_using_method( manifest, method, 'modified') == {'seed'} + assert search_manifest_using_method( + manifest, method, 'modified.configs') == {'seed'} assert not search_manifest_using_method(manifest, method, 'new') + assert not search_manifest_using_method(manifest, method, 'modified.body') + assert not search_manifest_using_method(manifest, method, 'modified.persisted_descriptions') def test_select_state_changed_seed_relation_documented_nodocs(manifest, previous_state, seed): @@ -825,7 +908,10 @@ def test_select_state_changed_seed_relation_documented_nodocs(manifest, previous method = statemethod(manifest, previous_state) assert search_manifest_using_method( manifest, method, 'modified') == {'seed'} + assert search_manifest_using_method( + manifest, method, 'modified.persisted_descriptions') == {'seed'} assert not search_manifest_using_method(manifest, method, 'new') + assert not search_manifest_using_method(manifest, method, 'modified.configs') def test_select_state_changed_seed_relation_documented_withdocs(manifest, previous_state, seed): @@ -837,6 +923,8 @@ def test_select_state_changed_seed_relation_documented_withdocs(manifest, previo method = statemethod(manifest, previous_state) assert search_manifest_using_method( manifest, method, 'modified') == {'seed'} + assert search_manifest_using_method( + manifest, method, 'modified.persisted_descriptions') == {'seed'} assert not search_manifest_using_method(manifest, method, 'new') @@ -847,7 +935,10 @@ def test_select_state_changed_seed_columns_documented(manifest, previous_state, method = statemethod(manifest, previous_state) assert search_manifest_using_method( manifest, method, 'modified') == {'seed'} + assert search_manifest_using_method( + manifest, method, 'modified.configs') == {'seed'} assert not search_manifest_using_method(manifest, method, 'new') + assert not search_manifest_using_method(manifest, method, 'modified.persisted_descriptions') def test_select_state_changed_seed_columns_documented_nodocs(manifest, previous_state, seed): @@ -862,7 +953,10 @@ def test_select_state_changed_seed_columns_documented_nodocs(manifest, previous_ method = statemethod(manifest, previous_state) assert search_manifest_using_method( manifest, method, 'modified') == {'seed'} + assert search_manifest_using_method( + manifest, method, 'modified.persisted_descriptions') == {'seed'} assert not search_manifest_using_method(manifest, method, 'new') + assert not search_manifest_using_method(manifest, method, 'modified.configs') def test_select_state_changed_seed_columns_documented_withdocs(manifest, previous_state, seed): @@ -877,4 +971,16 @@ def test_select_state_changed_seed_columns_documented_withdocs(manifest, previou method = statemethod(manifest, previous_state) assert search_manifest_using_method( manifest, method, 'modified') == {'seed'} + assert search_manifest_using_method( + manifest, method, 'modified.persisted_descriptions') == {'seed'} + assert not search_manifest_using_method(manifest, method, 'new') + assert not search_manifest_using_method(manifest, method, 'modified.configs') + +def test_select_state_changed_test_macro_sql(manifest, previous_state, macro_default_test_not_null): + manifest.macros[macro_default_test_not_null.unique_id] = macro_default_test_not_null.replace(macro_sql='lalala') + method = statemethod(manifest, previous_state) + assert search_manifest_using_method( + manifest, method, 'modified') == {'not_null_table_model_id'} + assert search_manifest_using_method( + manifest, method, 'modified.macros') == {'not_null_table_model_id'} assert not search_manifest_using_method(manifest, method, 'new')