Repository: dbt-labs/dbt-core
Pull request: Adding metric expression validation
resolves #5871 

### Description

- changing default value for expression to None
- Adding test to confirm behavior

### Checklist

- [X] I have read [the contributing guide](https://github.com/dbt-labs/dbt-core/blob/main/CONTRIBUTING.md) and understand what's expected of me
- [X] I have signed the [CLA](https://docs.getdbt.com/docs/contributor-license-agreements)
- [X] I have run this code in development and it appears to resolve the stated issue
- [X] This PR includes tests, or tests are not required/relevant for this PR
- [ ] I have run `changie new` to [create a changelog entry](https://github.com/dbt-labs/dbt-core/blob/main/CONTRIBUTING.md#adding-a-changelog-entry)


Problem statement:
[CT-1201] [Bug] Metrics don't require `expression` field
### Is this a new bug in dbt-core?

- [X] I believe this is a new bug in dbt-core
- [X] I have searched the existing issues, and I could not find an existing issue for this bug

### Current Behavior

Ran into this behavior when working with a beta customer. Right now, we provide the default value of empty string ("") to the expression/sql field. That then gets provided to ParsedMetric where it recognizes this field as a string and accepts it, even if the field isn't defined in the metric.

### Expected Behavior

If the expression field isn't present, the metric would fail to compile.

### Steps To Reproduce

1. Create a metric without a sql/expression field
2. Compile/parse

Hints from issue discussion:
Thanks for the quick issue + PR Callum!

Commit message: updating for review

Patch to review:
diff --git a/.changes/unreleased/Under the Hood-20220916-154712.yaml b/.changes/unreleased/Under the Hood-20220916-154712.yaml
new file mode 100644
index 00000000000..1d425067d3e
--- /dev/null
+++ b/.changes/unreleased/Under the Hood-20220916-154712.yaml	
@@ -0,0 +1,7 @@
+kind: Under the Hood
+body: Adding validation for metric expression attribute
+time: 2022-09-16T15:47:12.799002-05:00
+custom:
+  Author: callum-mcdata
+  Issue: "5871"
+  PR: "5873"
diff --git a/core/dbt/contracts/graph/parsed.py b/core/dbt/contracts/graph/parsed.py
index 3eee514ed00..772d2abc08c 100644
--- a/core/dbt/contracts/graph/parsed.py
+++ b/core/dbt/contracts/graph/parsed.py
@@ -819,8 +819,8 @@ class ParsedMetric(UnparsedBaseNode, HasUniqueID, HasFqn):
     description: str
     label: str
     calculation_method: str
-    expression: str
     timestamp: str
+    expression: str
     filters: List[MetricFilter]
     time_grains: List[str]
     dimensions: List[str]
diff --git a/core/dbt/contracts/graph/unparsed.py b/core/dbt/contracts/graph/unparsed.py
index 167612b3517..a53ba0a45ad 100644
--- a/core/dbt/contracts/graph/unparsed.py
+++ b/core/dbt/contracts/graph/unparsed.py
@@ -475,8 +475,8 @@ class UnparsedMetric(dbtClassMixin, Replaceable):
     label: str
     calculation_method: str
     timestamp: str
+    expression: str
     description: str = ""
-    expression: Union[str, int] = ""
     time_grains: List[str] = field(default_factory=list)
     dimensions: List[str] = field(default_factory=list)
     window: Optional[MetricTime] = None
diff --git a/tests/functional/metrics/test_metrics.py b/tests/functional/metrics/test_metrics.py
index 933f205c3f6..17798894f7f 100644
--- a/tests/functional/metrics/test_metrics.py
+++ b/tests/functional/metrics/test_metrics.py
@@ -196,6 +196,42 @@ def test_simple_metric(
             run_dbt(["run"])
 
 
+invalid_metrics__missing_expression_yml = """
+version: 2
+metrics:
+  - name: number_of_people
+    label: "Number of people"
+    description: Total count of people
+    calculation_method: count
+    timestamp: created_at
+    time_grains: [day, week, month]
+    dimensions:
+      - favorite_color
+      - loves_dbt
+    meta:
+        my_meta: 'testing'
+"""
+
+
+class TestInvalidMetricMissingExpression:
+    @pytest.fixture(scope="class")
+    def models(self):
+        return {
+            "people_metrics.yml": invalid_metrics__missing_expression_yml,
+            "people.sql": models__people_sql,
+        }
+
+    # tests that we get a ParsingException with an invalid model ref, where
+    # the model name does not have quotes
+    def test_simple_metric(
+        self,
+        project,
+    ):
+        # initial run
+        with pytest.raises(ParsingException):
+            run_dbt(["run"])
+
+
 names_with_spaces_metrics_yml = """
 version: 2
 


Code review comments:
diff_hunk: @@ -196,6 +196,42 @@ def test_simple_metric(
             run_dbt(["run"])
 
 
+invalid_metrics__missing_expression_yml = """
+version: 2
+metrics:
+  - name: number_of_people
+    label: "Number of people"
+    description: Total count of people
+    calculation_method: count
+    timestamp: created_at
+    time_grains: [day, week, month]
+    dimensions:
+      - favorite_color
+      - loves_dbt
+    meta:
+        my_meta: 'testing'
+"""
+
+
+class TestInvalidMetricMissingExpression:
+    @pytest.fixture(scope="class")
+    def models(self):
+        return {
+            "people_metrics.yml": invalid_metrics__missing_expression_yml,
+            "people.sql": models__people_sql,
+        }
+
+    # tests that we get a ParsingException with an invalid model ref, where
+    # the model name does not have quotes
path: tests/functional/metrics/test_metrics.py
text: @user1:
This comment is no longer accurate.

@author:
I think this is still true, no? This PR doesn't change anything with regards to `model` validation, only `expression`. I still get a parsing exception if I remove the model field from a definition and then run `dbt parse`

@user1:
@user2-mcdata  this specific test isn't testing an invalid ref missing quotes, it's testing a missing expression, right?  

You are right that we still expect the ParsingException, but the reason is different for this test.

@author:
AH - I see what you mean 🤦 . Yes I updated the comments in the test to better reflect what the actual issue is AND fixed the metric spec provided to that test because it also contained a missing model which would have error'd out and not actually tested this behavior.