Skip to content

Commit

Permalink
Fix: ensure DbtGraph.update_node_dependency is called for all load …
Browse files Browse the repository at this point in the history
…methods (#803)

This resolves #798 where when using `LoadMode.DBT_LS_FILE`, the
`DbtGraph.update_node_dependency` was not called resulting in filtered
nodes not having `DbtNode.has_test` set as expected.

Closes: #798
  • Loading branch information
jbandoro authored Jan 18, 2024
1 parent d3ea329 commit b313544
Show file tree
Hide file tree
Showing 2 changed files with 9 additions and 17 deletions.
20 changes: 5 additions & 15 deletions cosmos/dbt/graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -181,6 +181,11 @@ def load(
else:
load_method[method]()

self.update_node_dependency()

logger.info("Total nodes: %i", len(self.nodes))
logger.info("Total filtered nodes: %i", len(self.nodes))

def run_dbt_ls(
self, dbt_cmd: str, project_path: Path, tmp_dir: Path, env_vars: dict[str, str]
) -> dict[str, DbtNode]:
Expand Down Expand Up @@ -276,11 +281,6 @@ def load_via_dbt_ls(self) -> None:
self.nodes = nodes
self.filtered_nodes = nodes

self.update_node_dependency()

logger.info("Total nodes: %i", len(self.nodes))
logger.info("Total filtered nodes: %i", len(self.nodes))

def load_via_dbt_ls_file(self) -> None:
"""
This is between dbt ls and full manifest. It allows to use the output (needs to be json output) of the dbt ls as a
Expand Down Expand Up @@ -364,11 +364,6 @@ def load_via_custom_parser(self) -> None:
exclude=self.render_config.exclude,
)

self.update_node_dependency()

logger.info("Total nodes: %i", len(self.nodes))
logger.info("Total filtered nodes: %i", len(self.nodes))

def load_from_dbt_manifest(self) -> None:
"""
This approach accurately loads `dbt` projects using the `manifest.yml` file.
Expand Down Expand Up @@ -418,11 +413,6 @@ def load_from_dbt_manifest(self) -> None:
exclude=self.render_config.exclude,
)

self.update_node_dependency()

logger.info("Total nodes: %i", len(self.nodes))
logger.info("Total filtered nodes: %i", len(self.nodes))

def update_node_dependency(self) -> None:
"""
This will update the property `has_text` if node has `dbt` test
Expand Down
6 changes: 4 additions & 2 deletions tests/dbt/test_graph.py
Original file line number Diff line number Diff line change
Expand Up @@ -258,6 +258,7 @@ def test_load_manifest_with_manifest(mock_load_from_dbt_manifest):
(ExecutionMode.LOCAL, LoadMode.CUSTOM, "mock_load_via_custom_parser"),
],
)
@patch("cosmos.dbt.graph.DbtGraph.update_node_dependency")
@patch("cosmos.dbt.graph.DbtGraph.load_via_custom_parser", return_value=None)
@patch("cosmos.dbt.graph.DbtGraph.load_via_dbt_ls", return_value=None)
@patch("cosmos.dbt.graph.DbtGraph.load_from_dbt_manifest", return_value=None)
Expand All @@ -267,6 +268,7 @@ def test_load(
mock_load_via_dbt_ls_file,
mock_load_via_dbt_ls,
mock_load_via_custom_parser,
mock_update_node_dependency,
exec_mode,
method,
expected_function,
Expand All @@ -282,6 +284,7 @@ def test_load(
dbt_graph.load(method=method, execution_mode=exec_mode)
load_function = locals()[expected_function]
assert load_function.called
assert mock_update_node_dependency.called


@pytest.mark.integration
Expand Down Expand Up @@ -675,8 +678,7 @@ def test_tag_selected_node_test_exist():
profile_config=profile_config,
render_config=render_config,
)
dbt_graph.load_from_dbt_manifest()

dbt_graph.load()
assert len(dbt_graph.filtered_nodes) > 0

for _, node in dbt_graph.filtered_nodes.items():
Expand Down

0 comments on commit b313544

Please sign in to comment.