From f80b4e1d58bc55c3873f33cb65c6d90dadf15dee Mon Sep 17 00:00:00 2001 From: Quigley Malcolm Date: Thu, 7 Sep 2023 12:45:05 -0700 Subject: [PATCH] Move from `manifest.metrics.get` to `manifest.expect` in metric helpers Previously with `manifest.metrics.get` we were just skipping when `None` was returned. Getting `None` back was expected in that `parent_unique_id`s that didn't belong to metrics should return `None` when calling `manifest.metrics.get`, and these are fine to skip. However, there's an edgecase where a `parent_unique_id` is supposed to be a metric, but isn't found, thus returning `None`. How likely this edge case could get hit, I'm not sure, but it's a possible edge case. Using `manifest.metrics.get` it we can't actually tell if we're in the edge case or not. By moving to `manifest.expect` we get the error handling built in, and the only trade off is that we need to change our conditional to skip returned nodes that aren't metrics. --- core/dbt/contracts/graph/metrics.py | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/core/dbt/contracts/graph/metrics.py b/core/dbt/contracts/graph/metrics.py index d7e65540dec..c95604cb976 100644 --- a/core/dbt/contracts/graph/metrics.py +++ b/core/dbt/contracts/graph/metrics.py @@ -41,9 +41,9 @@ def parent_metrics(cls, metric_node: Metric, manifest: Manifest) -> Iterator[Met yield metric_node for parent_unique_id in metric_node.depends_on.nodes: - metric = manifest.metrics.get(parent_unique_id) - if metric is not None: - yield from cls.parent_metrics(metric, manifest) + node = manifest.expect(parent_unique_id) + if isinstance(node, Metric): + yield from cls.parent_metrics(node, manifest) @classmethod def parent_metrics_names(cls, metric_node: Metric, manifest: Manifest) -> Iterator[str]: @@ -63,9 +63,9 @@ def reverse_dag_parsing( yield {metric_node.name: metric_depth_count} for parent_unique_id in metric_node.depends_on.nodes: - metric = manifest.metrics.get(parent_unique_id) - if metric is not None: - yield from cls.reverse_dag_parsing(metric, manifest, metric_depth_count + 1) + node = manifest.expect(parent_unique_id) + if isinstance(node, Metric): + yield from cls.reverse_dag_parsing(node, manifest, metric_depth_count + 1) def full_metric_dependency(self): """Returns a unique list of all upstream metric names."""