Back/fwd compatibility for renamed metrics attributes (dbt-labs#5825)

* Naive handling for metric attr renames

* Add tests for bwd/fwd compatibility

* Add deprecation

* Add changelog entry

* PR feedback

* Small fixups

* emmyoop's suggestions

Co-authored-by: Callum McCann <cmccann51@gmail.com>

.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,
+        }