From 885a8a96527bf1951e6f0b8e072728fa5c5a0240 Mon Sep 17 00:00:00 2001 From: "github-actions[bot]" <41898282+github-actions[bot]@users.noreply.github.com> Date: Tue, 10 Oct 2023 16:23:51 -0500 Subject: [PATCH] Fix uncaught exception for group updates (#8792) (#8813) * add test * write test * fix test * updating test * add clean * cleanup * more tests, fix comment * add new test, move fixtures (cherry picked from commit 4f9bd0cb3850f035fdb162a0434bc748c784760f) Co-authored-by: Emily Rockman --- .../unreleased/Fixes-20231010-125948.yaml | 6 + core/dbt/contracts/graph/manifest.py | 9 +- tests/functional/defer_state/fixtures.py | 63 +++++++++ .../defer_state/test_group_updates.py | 121 ++++++++++++++++++ 4 files changed, 198 insertions(+), 1 deletion(-) create mode 100644 .changes/unreleased/Fixes-20231010-125948.yaml create mode 100644 tests/functional/defer_state/test_group_updates.py diff --git a/.changes/unreleased/Fixes-20231010-125948.yaml b/.changes/unreleased/Fixes-20231010-125948.yaml new file mode 100644 index 00000000000..ac6d237fe2f --- /dev/null +++ b/.changes/unreleased/Fixes-20231010-125948.yaml @@ -0,0 +1,6 @@ +kind: Fixes +body: Group updates on unmodified nodes are handled gracefully for state:modified +time: 2023-10-10T12:59:48.390113-05:00 +custom: + Author: emmyoop + Issue: "8371" diff --git a/core/dbt/contracts/graph/manifest.py b/core/dbt/contracts/graph/manifest.py index 4534adb3f80..47abff2d0db 100644 --- a/core/dbt/contracts/graph/manifest.py +++ b/core/dbt/contracts/graph/manifest.py @@ -853,7 +853,14 @@ def build_group_map(self): group_map = {group.name: [] for group in self.groups.values()} for node in groupable_nodes: if node.group is not None: - group_map[node.group].append(node.unique_id) + # group updates are not included with state:modified and + # by ignoring the groups that aren't in the group map we + # can avoid hitting errors for groups that are not getting + # updated. This is a hack but any groups that are not + # valid will be caught in + # parser.manifest.ManifestLoader.check_valid_group_config_node + if node.group in group_map: + group_map[node.group].append(node.unique_id) self.group_map = group_map def writable_manifest(self): diff --git a/tests/functional/defer_state/fixtures.py b/tests/functional/defer_state/fixtures.py index aef6986dde0..44607c8fa25 100644 --- a/tests/functional/defer_state/fixtures.py +++ b/tests/functional/defer_state/fixtures.py @@ -247,3 +247,66 @@ {% endsnapshot %} """ + +model_1_sql = """ +select * from {{ ref('seed') }} +""" + +modified_model_1_sql = """ +select * from {{ ref('seed') }} +order by 1 +""" + +model_2_sql = """ +select id from {{ ref('model_1') }} +""" + +modified_model_2_sql = """ +select * from {{ ref('model_1') }} +order by 1 +""" + + +group_schema_yml = """ +groups: + - name: finance + owner: + email: finance@jaffleshop.com + +models: + - name: model_1 + config: + group: finance + - name: model_2 + config: + group: finance +""" + + +group_modified_schema_yml = """ +groups: + - name: accounting + owner: + email: finance@jaffleshop.com +models: + - name: model_1 + config: + group: accounting + - name: model_2 + config: + group: accounting +""" + +group_modified_fail_schema_yml = """ +groups: + - name: finance + owner: + email: finance@jaffleshop.com +models: + - name: model_1 + config: + group: accounting + - name: model_2 + config: + group: finance +""" diff --git a/tests/functional/defer_state/test_group_updates.py b/tests/functional/defer_state/test_group_updates.py new file mode 100644 index 00000000000..884909cf649 --- /dev/null +++ b/tests/functional/defer_state/test_group_updates.py @@ -0,0 +1,121 @@ +import os + +import pytest + +from dbt.tests.util import run_dbt, write_file, copy_file +from dbt.exceptions import ParsingError + + +from tests.functional.defer_state.fixtures import ( + seed_csv, + model_1_sql, + modified_model_1_sql, + model_2_sql, + modified_model_2_sql, + group_schema_yml, + group_modified_schema_yml, + group_modified_fail_schema_yml, +) + + +class GroupSetup: + @pytest.fixture(scope="class") + def models(self): + return { + "model_1.sql": model_1_sql, + "model_2.sql": model_2_sql, + "schema.yml": group_schema_yml, + } + + @pytest.fixture(scope="class") + def seeds(self): + return { + "seed.csv": seed_csv, + } + + def group_setup(self): + # save initial state + run_dbt(["seed"]) + results = run_dbt(["compile"]) + + # add sanity checks for first result + assert len(results) == 3 + seed_result = results[0].node + assert seed_result.unique_id == "seed.test.seed" + model_1_result = results[1].node + assert model_1_result.unique_id == "model.test.model_1" + assert model_1_result.group == "finance" + model_2_result = results[2].node + assert model_2_result.unique_id == "model.test.model_2" + assert model_2_result.group == "finance" + + +class TestFullyModifiedGroups(GroupSetup): + def test_changed_groups(self, project): + self.group_setup() + + # copy manifest.json to "state" directory + os.makedirs("state") + target_path = os.path.join(project.project_root, "target") + copy_file(target_path, "manifest.json", project.project_root, ["state", "manifest.json"]) + + # update group name, modify model so it gets picked up + write_file(modified_model_1_sql, "models", "model_1.sql") + write_file(modified_model_2_sql, "models", "model_2.sql") + write_file(group_modified_schema_yml, "models", "schema.yml") + + # this test is flaky if you don't clean first before the build + run_dbt(["clean"]) + # only thing in results should be model_1 + results = run_dbt(["build", "-s", "state:modified", "--defer", "--state", "./state"]) + + assert len(results) == 2 + model_1_result = results[0].node + assert model_1_result.unique_id == "model.test.model_1" + assert model_1_result.group == "accounting" # new group name! + model_2_result = results[1].node + assert model_2_result.unique_id == "model.test.model_2" + assert model_2_result.group == "accounting" # new group name! + + +class TestPartiallyModifiedGroups(GroupSetup): + def test_changed_groups(self, project): + self.group_setup() + + # copy manifest.json to "state" directory + os.makedirs("state") + target_path = os.path.join(project.project_root, "target") + copy_file(target_path, "manifest.json", project.project_root, ["state", "manifest.json"]) + + # update group name, modify model so it gets picked up + write_file(modified_model_1_sql, "models", "model_1.sql") + write_file(group_modified_schema_yml, "models", "schema.yml") + + # this test is flaky if you don't clean first before the build + run_dbt(["clean"]) + # only thing in results should be model_1 + results = run_dbt(["build", "-s", "state:modified", "--defer", "--state", "./state"]) + + assert len(results) == 1 + model_1_result = results[0].node + assert model_1_result.unique_id == "model.test.model_1" + assert model_1_result.group == "accounting" # new group name! + + +class TestBadGroups(GroupSetup): + def test_changed_groups(self, project): + self.group_setup() + + # copy manifest.json to "state" directory + os.makedirs("state") + target_path = os.path.join(project.project_root, "target") + copy_file(target_path, "manifest.json", project.project_root, ["state", "manifest.json"]) + + # update group with invalid name, modify model so it gets picked up + write_file(modified_model_1_sql, "models", "model_1.sql") + write_file(group_modified_fail_schema_yml, "models", "schema.yml") + + # this test is flaky if you don't clean first before the build + run_dbt(["clean"]) + with pytest.raises(ParsingError, match="Invalid group 'accounting'"): + run_dbt(["build", "-s", "state:modified", "--defer", "--state", "./state"])