Back/fwd compatibility for renamed metrics attributes

.changes/unreleased/Under the Hood-20220913-111744.yaml

@@ -0,0 +1,7 @@
+kind: Under the Hood
+body: Compatibiltiy for metric attribute renaming
+time: 2022-09-13T11:17:44.953536+02:00
+custom:
+ Author: jtcohen6 callum-mcdata
+ Issue: "5807"
+ PR: "5825"

core/dbt/contracts/graph/parsed.py

@@ -820,11 +820,11 @@ class ParsedMetric(UnparsedBaseNode, HasUniqueID, HasFqn):
  label: str
  calculation_method: str
  expression: str
- timestamp: Optional[str]
+ timestamp: str
  filters: List[MetricFilter]
  time_grains: List[str]
  dimensions: List[str]
- window: Optional[MetricTime]
+ window: Optional[MetricTime] = None
  model: Optional[str] = None
  model_unique_id: Optional[str] = None
  resource_type: NodeType = NodeType.Metric

core/dbt/contracts/graph/unparsed.py

@@ -1,5 +1,10 @@
 from dbt.node_types import NodeType
-from dbt.contracts.util import AdditionalPropertiesMixin, Mergeable, Replaceable
+from dbt.contracts.util import (
+ AdditionalPropertiesMixin,
+ Mergeable,
+ Replaceable,
+ rename_metric_attr,
+)
 
 # trigger the PathEncoder
 import dbt.helper_types # noqa:F401
@@ -483,15 +488,11 @@ class UnparsedMetric(dbtClassMixin, Replaceable):
 
  @classmethod
  def validate(cls, data):
+ data = rename_metric_attr(data, raise_deprecation_warning=True)
  super(UnparsedMetric, cls).validate(data)
  if "name" in data and " " in data["name"]:
  raise ParsingException(f"Metrics name '{data['name']}' cannot contain spaces")
 
- if data.get("calculation_method") == "expression":
- raise ValidationError(
- "The metric calculation method expression has been deprecated and renamed to derived. Please update"
- )
-
  if data.get("model") is None and data.get("calculation_method") != "derived":
  raise ValidationError("Non-derived metrics require a 'model' property")
 

core/dbt/contracts/util.py

@@ -4,6 +4,7 @@
 from typing import List, Tuple, ClassVar, Type, TypeVar, Dict, Any, Optional
 
 from dbt.clients.system import write_json, read_json
+from dbt import deprecations
 from dbt.exceptions import (
  InternalException,
  RuntimeException,
@@ -207,13 +208,51 @@ def get_manifest_schema_version(dct: dict) -> int:
  return int(schema_version.split(".")[-2][-1])
 
 
+# we renamed these properties in v1.3
+# this method allows us to be nice to the early adopters
+def rename_metric_attr(data: dict, raise_deprecation_warning: bool = False) -> dict:
+ metric_name = data["name"]
+ if raise_deprecation_warning and (
+ "sql" in data.keys()
+ or "type" in data.keys()
+ or data.get("calculation_method") == "expression"
+ ):
+ deprecations.warn("metric-attr-renamed", metric_name=metric_name)
+ duplicated_attribute_msg = """\n
+The metric '{}' contains both the deprecated metric property '{}'
+and the up-to-date metric property '{}'. Please remove the deprecated property.
+"""
+ if "sql" in data.keys():
+ if "expression" in data.keys():
+ raise ValidationError(
+ duplicated_attribute_msg.format(metric_name, "sql", "expression")
+ )
+ else:
+ data["expression"] = data.pop("sql")
+ if "type" in data.keys():
+ if "calculation_method" in data.keys():
+ raise ValidationError(
+ duplicated_attribute_msg.format(metric_name, "type", "calculation_method")
+ )
+ else:
+ calculation_method = data.pop("type")
+ data["calculation_method"] = calculation_method
+ # we also changed "type: expression" -> "calculation_method: derived"
+ if data.get("calculation_method") == "expression":
+ data["calculation_method"] = "derived"
+ return data
+
+
 def upgrade_manifest_json(manifest: dict) -> dict:
  for node_content in manifest.get("nodes", {}).values():
  if "raw_sql" in node_content:
  node_content["raw_code"] = node_content.pop("raw_sql")
  if "compiled_sql" in node_content:
  node_content["compiled_code"] = node_content.pop("compiled_sql")
  node_content["language"] = "sql"
+ for metric_content in manifest.get("metrics", {}).values():
+ # handle attr renames + value translation ("expression" -> "derived")
+ metric_content = rename_metric_attr(metric_content)
  return manifest
 
 

core/dbt/deprecations.py

@@ -87,6 +87,19 @@ class AdapterDeprecationWarning(DBTDeprecation):
  deprecations[dep.name] = dep
 
 
+class MetricAttributesRenamed(DBTDeprecation):
+ _name = "metric-attr-renamed"
+ _description = """\
+dbt-core v1.3 renamed attributes for metrics:
+\n 'sql' -> 'expression'
+\n 'type' -> 'calculation_method'
+\n 'type: expression' -> 'calculation_method: derived'
+\nThe old metric parameter names will be fully deprecated in v1.4.
+\nPlease remove them from the metric definition of metric '{metric_name}'
+\nRelevant issue here: https://github.com/dbt-labs/dbt-core/issues/5849
+"""
+
+
 def warn(name, *args, **kwargs):
  if name not in deprecations:
  # this should (hopefully) never happen
@@ -105,6 +118,7 @@ def warn(name, *args, **kwargs):
  ConfigDataPathDeprecation(),
  PackageInstallPathDeprecation(),
  PackageRedirectDeprecation(),
+ MetricAttributesRenamed(),
 ]
 
 deprecations: Dict[str, DBTDeprecation] = {d.name: d for d in deprecations_list}

core/dbt/parser/schema_renderer.py

@@ -67,7 +67,8 @@ def should_render_keypath(self, keypath: Keypath) -> bool:
  elif self._is_norender_key(keypath[0:]):
  return False
  elif self.key == "metrics":
- if keypath[0] == "expression":
+ # back compat: "expression" is new name, "sql" is old name
+ if keypath[0] in ("expression", "sql"):
  return False
  elif self._is_norender_key(keypath[0:]):
  return False

tests/functional/artifacts/test_previous_version_state.py

@@ -10,6 +10,20 @@
 select 1 as id
 """
 
+# Use old attribute names (v1.0-1.2) to test forward/backward compatibility with the rename in v1.3
+models__metric_yml = """
+version: 2
+metrics:
+ - name: my_metric
+ label: Count records
+ model: ref('my_model')
+
+ type: count
+ sql: "*"
+ timestamp: updated_at
+ time_grains: [day]
+"""
+
 # SETUP: Using this project, we have run past minor versions of dbt
 # to generate each contracted version of `manifest.json`.
 
@@ -32,7 +46,7 @@ class TestPreviousVersionState:
 
  @pytest.fixture(scope="class")
  def models(self):
- return {"my_model.sql": models__my_model_sql}
+ return {"my_model.sql": models__my_model_sql, "metric.yml": models__metric_yml}
 
  # Use this method when generating a new manifest version for the first time.
  # Once generated, we shouldn't need to re-generate or modify the manifest.

tests/functional/deprecations/test_deprecations.py

@@ -17,6 +17,19 @@
 select 1 as id
 """
 
+metrics_old_metric_names__yml = """
+version: 2
+metrics:
+ - name: my_metric
+ label: My metric
+ model: ref('my_model')
+
+ type: count
+ sql: "*"
+ timestamp: updated_at
+ time_grains: [day]
+"""
+
 
 class TestConfigPathDeprecation:
  @pytest.fixture(scope="class")
@@ -113,3 +126,29 @@ def test_package_redirect_fail(self, project):
  exc_str = " ".join(str(exc.value).split()) # flatten all whitespace
  expected_msg = "The `fishtown-analytics/dbt_utils` package is deprecated in favor of `dbt-labs/dbt_utils`"
  assert expected_msg in exc_str
+
+
+class TestMetricAttrRenameDeprecation:
+ @pytest.fixture(scope="class")
+ def models(self):
+ return {
+ "my_model.sql": models_trivial__model_sql,
+ "metrics.yml": metrics_old_metric_names__yml,
+ }
+
+ def test_metric_handle_rename(self, project):
+ deprecations.reset_deprecations()
+ assert deprecations.active_deprecations == set()
+ run_dbt(["parse"])
+ expected = {"metric-attr-renamed"}
+ assert expected == deprecations.active_deprecations
+
+ def test_metric_handle_rename_fail(self, project):
+ deprecations.reset_deprecations()
+ assert deprecations.active_deprecations == set()
+ with pytest.raises(dbt.exceptions.CompilationException) as exc:
+ # turn off partial parsing to ensure that the metric is re-parsed
+ run_dbt(["--warn-error", "--no-partial-parse", "parse"])
+ exc_str = " ".join(str(exc.value).split()) # flatten all whitespace
+ expected_msg = "renamed attributes for metrics"
+ assert expected_msg in exc_str

tests/functional/metrics/test_metrics.py

@@ -455,3 +455,52 @@ def test_derived_metric(
  ]:
  expected_value = getattr(parsed_metric_node, property)
  assert f"{property}: {expected_value}" in compiled_code
+
+
+derived_metric_old_attr_names_yml = """
+version: 2
+metrics:
+ - name: count_orders
+ label: Count orders
+ model: ref('mock_purchase_data')
+
+ type: count
+ sql: "*"
+ timestamp: purchased_at
+ time_grains: [day, week, month, quarter, year]
+
+ dimensions:
+ - payment_type
+
+ - name: sum_order_revenue
+ label: Total order revenue
+ model: ref('mock_purchase_data')
+
+ type: sum
+ sql: "payment_total"
+ timestamp: purchased_at
+ time_grains: [day, week, month, quarter, year]
+
+ dimensions:
+ - payment_type
+
+ - name: average_order_value
+ label: Average Order Value
+
+ type: expression
+ sql: "{{metric('sum_order_revenue')}} / {{metric('count_orders')}} "
+ timestamp: purchased_at
+ time_grains: [day, week, month, quarter, year]
+
+ dimensions:
+ - payment_type
+"""
+
+
+class TestDerivedMetricOldAttrNames(TestDerivedMetric):
+ @pytest.fixture(scope="class")
+ def models(self):
+ return {
+ "derived_metric.yml": derived_metric_old_attr_names_yml,
+ "downstream_model.sql": downstream_model_sql,
+ }