Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Safely remove external nodes from manifest #8495

Merged
merged 2 commits into from
Aug 28, 2023
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
7 changes: 7 additions & 0 deletions .changes/unreleased/Fixes-20230828-125858.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,7 @@
kind: Fixes
body: 'Fix "Internal Error: Expected node <unique-id> not found in manifest" when
depends_on set on ModelNodeArgs'
time: 2023-08-28T12:58:58.061228-04:00
custom:
Author: michelleark
Issue: "8506"
7 changes: 5 additions & 2 deletions core/dbt/parser/manifest.py
Original file line number Diff line number Diff line change
Expand Up @@ -559,7 +559,7 @@ def load(self) -> Manifest:
)
# parent and child maps will be rebuilt by write_manifest

if not skip_parsing:
if not skip_parsing or external_nodes_modified:
# write out the fully parsed manifest
self.write_manifest_for_partial_parse()

Expand Down Expand Up @@ -757,10 +757,13 @@ def write_manifest_for_partial_parse(self):
def inject_external_nodes(self) -> bool:
# Remove previously existing external nodes since we are regenerating them
manifest_nodes_modified = False
# Remove all dependent nodes before removing referencing nodes
for unique_id in self.manifest.external_node_unique_ids:
self.manifest.nodes.pop(unique_id)
remove_dependent_project_references(self.manifest, unique_id)
manifest_nodes_modified = True
for unique_id in self.manifest.external_node_unique_ids:
# remove external nodes from manifest only after dependent project references safely removed
self.manifest.nodes.pop(unique_id)

# Inject any newly-available external nodes
pm = plugins.get_plugin_manager(self.root_project.project_name)
Expand Down
42 changes: 40 additions & 2 deletions tests/functional/partial_parsing/test_partial_parsing.py
Original file line number Diff line number Diff line change
Expand Up @@ -760,20 +760,45 @@ def external_model_node_versioned(self):
version=1,
)

@pytest.fixture(scope="class")
def external_model_node_depends_on(self):
return ModelNodeArgs(
name="external_model_depends_on",
package_name="external",
identifier="test_identifier_depends_on",
schema="test_schema",
depends_on_nodes=["model.external.external_model_depends_on_parent"],
)

@pytest.fixture(scope="class")
def external_model_node_depends_on_parent(self):
return ModelNodeArgs(
name="external_model_depends_on_parent",
package_name="external",
identifier="test_identifier_depends_on_parent",
schema="test_schema",
)

@pytest.fixture(scope="class")
def models(self):
return {"model_one.sql": model_one_sql}

@mock.patch("dbt.plugins.get_plugin_manager")
def test_pp_external_models(
self, get_plugin_manager, project, external_model_node, external_model_node_versioned
self,
get_plugin_manager,
project,
external_model_node,
external_model_node_versioned,
external_model_node_depends_on,
external_model_node_depends_on_parent,
):
# initial plugin - one external model
external_nodes = PluginNodes()
external_nodes.add_model(external_model_node)
get_plugin_manager.return_value.get_nodes.return_value = external_nodes

# initial run
# initial parse
manifest = run_dbt(["parse"])
assert len(manifest.nodes) == 2
assert set(manifest.nodes.keys()) == {
Expand Down Expand Up @@ -803,8 +828,21 @@ def test_pp_external_models(
)
manifest = run_dbt(["--partial-parse", "parse"])
assert len(manifest.nodes) == 5
assert len(manifest.external_node_unique_ids) == 2

# remove a model file that depends on external model
rm_file(project.project_root, "models", "model_depends_on_external.sql")
manifest = run_dbt(["--partial-parse", "parse"])
assert len(manifest.nodes) == 4

# add an external node with depends on
external_nodes.add_model(external_model_node_depends_on)
external_nodes.add_model(external_model_node_depends_on_parent)
manifest = run_dbt(["--partial-parse", "parse"])
assert len(manifest.nodes) == 6
assert len(manifest.external_node_unique_ids) == 4

# skip files parsing - ensure no issues
run_dbt(["--partial-parse", "parse"])
assert len(manifest.nodes) == 6
assert len(manifest.external_node_unique_ids) == 4