Merge pull request #184 from dbt-labs/qmalcolm--181-validate-metric-n…

…ames

Begin Validating Metric Names

.changes/unreleased/Breaking Changes-20231023-103143.yaml

@@ -0,0 +1,6 @@
+kind: Breaking Changes
+body: Fixes bug where we weren't validating the names of metrics (which may break peoples current metric names)
+time: 2023-10-23T10:31:43.522179-07:00
+custom:
+  Author: QMalcolm
+  Issue: "181"

dbt_semantic_interfaces/validations/unique_valid_name.py

@@ -2,10 +2,11 @@
 
 import enum
 import re
-from typing import Dict, Generic, List, Optional, Sequence, Tuple
+from typing import Dict, Generic, List, Optional, Sequence, Tuple, Union
 
 from dbt_semantic_interfaces.enum_extension import assert_values_exhausted
 from dbt_semantic_interfaces.protocols import (
+    Metric,
     SemanticManifest,
     SemanticManifestT,
     SemanticModel,
@@ -168,58 +169,68 @@ def _validate_semantic_model_elements(semantic_model: SemanticModel) -> List[Val
         return issues
 
     @staticmethod
-    @validate_safely(whats_being_done="checking model top level element names are sufficiently unique")
-    def _validate_top_level_objects(semantic_manifest: SemanticManifest) -> List[ValidationIssue]:
-        """Checks names of objects that are not nested."""
-        object_info_tuples = []
-        if semantic_manifest.semantic_models:
-            for semantic_model in semantic_manifest.semantic_models:
-                object_info_tuples.append(
-                    (
-                        semantic_model.name,
-                        "semantic model",
-                        SemanticModelContext(
-                            file_context=FileContext.from_metadata(metadata=semantic_model.metadata),
-                            semantic_model=SemanticModelReference(semantic_model_name=semantic_model.name),
-                        ),
-                    )
-                )
-
-        name_to_type: Dict[str, str] = {}
-
+    @validate_safely(whats_being_done="checking top level elements of a specific type have unique and valid names")
+    def _validate_top_level_objects_of_type(
+        object_context_tuples: Union[
+            List[Tuple[SemanticModel, SemanticModelContext]], List[Tuple[Metric, MetricContext]]
+        ],
+        object_type: str,
+    ) -> List[ValidationIssue]:
+        """Validates uniqeness and validaty of top level objects of singular type."""
         issues: List[ValidationIssue] = []
+        object_names = set()
 
-        for name, type_, context in object_info_tuples:
-            if name in name_to_type:
+        for object, context in object_context_tuples:
+            issues += UniqueAndValidNameRule.check_valid_name(name=object.name, context=context)
+            if object.name in object_names:
                 issues.append(
                     ValidationError(
                         context=context,
-                        message=f"Can't use name `{name}` for a {type_} when it was already used for a "
-                        f"{name_to_type[name]}",
+                        message=f"Can't use name `{object.name}` for a {object_type} when it was already "
+                        f"used for another {object_type}",
                     )
                 )
             else:
-                name_to_type[name] = type_
+                object_names.add(object.name)
+        return issues
 
-        if semantic_manifest.metrics:
-            metric_names = set()
-            for metric in semantic_manifest.metrics:
-                if metric.name in metric_names:
-                    issues.append(
-                        ValidationError(
-                            context=MetricContext(
-                                file_context=FileContext.from_metadata(metadata=metric.metadata),
-                                metric=MetricModelReference(metric_name=metric.name),
-                            ),
-                            message=f"Can't use name `{metric.name}` for a metric when it was already used for "
-                            "a metric",
-                        )
-                    )
-                else:
-                    metric_names.add(metric.name)
+    @staticmethod
+    @validate_safely(whats_being_done="checking model top level element names are sufficiently unique")
+    def _validate_top_level_objects(semantic_manifest: SemanticManifest) -> List[ValidationIssue]:
+        """Checks names of objects that are not nested."""
+        issues: List[ValidationIssue] = []
 
-        for name, _, context in object_info_tuples:
-            issues += UniqueAndValidNameRule.check_valid_name(name=name, context=context)
+        if semantic_manifest.semantic_models:
+            # TODO: We should clean up this pattern of precompiling object contexts
+            semantic_model_context_tuples = [
+                (
+                    semantic_model,
+                    SemanticModelContext(
+                        file_context=FileContext.from_metadata(metadata=semantic_model.metadata),
+                        semantic_model=SemanticModelReference(semantic_model_name=semantic_model.name),
+                    ),
+                )
+                for semantic_model in semantic_manifest.semantic_models
+            ]
+            issues.extend(
+                UniqueAndValidNameRule._validate_top_level_objects_of_type(
+                    semantic_model_context_tuples, "semantic model"
+                )
+            )
+
+        if semantic_manifest.metrics:
+            # TODO: We should clean up this pattern of precompiling object contexts
+            metric_context_tuples = [
+                (
+                    metric,
+                    MetricContext(
+                        file_context=FileContext.from_metadata(metadata=metric.metadata),
+                        metric=MetricModelReference(metric_name=metric.name),
+                    ),
+                )
+                for metric in semantic_manifest.metrics
+            ]
+            issues.extend(UniqueAndValidNameRule._validate_top_level_objects_of_type(metric_context_tuples, "metric"))
 
         return issues
 

tests/validations/test_unique_valid_name.py

@@ -24,15 +24,34 @@
     Top level elements include
     - Semantic Models
     - Metrics
-    - PydanticMetric Sets
-    - Dimension Sets
 
-    A name for any of these elements must be unique to all other top level elements
-    except metrics. PydanticMetric names only need to be unique in comparison to other metric
-    names.
+    For each top level element type we test for
+    - Name validity checking
+    - Name uniquness checking
 """
 
 
+def test_semantic_model_name_validity(  # noqa: D
+    simple_semantic_manifest__with_primary_transforms: PydanticSemanticManifest,
+):
+    validator = SemanticManifestValidator[PydanticSemanticManifest](
+        [UniqueAndValidNameRule[PydanticSemanticManifest]()]
+    )
+
+    # Shouldn't raise an exception
+    validator.checked_validations(simple_semantic_manifest__with_primary_transforms)
+
+    # Should raise an exception
+    copied_manifest = deepcopy(simple_semantic_manifest__with_primary_transforms)
+    semantic_model = copied_manifest.semantic_models[0]
+    semantic_model.name = f"@{semantic_model.name}"
+    with pytest.raises(
+        SemanticManifestValidationException,
+        match=rf"Invalid name `{semantic_model.name}",
+    ):
+        validator.checked_validations(copied_manifest)
+
+
 def test_duplicate_semantic_model_name(  # noqa: D
     simple_semantic_manifest__with_primary_transforms: PydanticSemanticManifest,
 ) -> None:
@@ -42,13 +61,34 @@ def test_duplicate_semantic_model_name(  # noqa: D
     with pytest.raises(
         SemanticManifestValidationException,
         match=rf"Can't use name `{duplicated_semantic_model.name}` for a semantic model when it was already used for "
-        "a semantic model",
+        "another semantic model",
     ):
         SemanticManifestValidator[PydanticSemanticManifest](
             [UniqueAndValidNameRule[PydanticSemanticManifest]()]
         ).checked_validations(model)
 
 
+def test_metric_name_validity(  # noqa: D
+    simple_semantic_manifest__with_primary_transforms: PydanticSemanticManifest,
+):
+    validator = SemanticManifestValidator[PydanticSemanticManifest](
+        [UniqueAndValidNameRule[PydanticSemanticManifest]()]
+    )
+
+    # Shouldn't raise an exception
+    validator.checked_validations(simple_semantic_manifest__with_primary_transforms)
+
+    # Should raise an exception
+    copied_manifest = deepcopy(simple_semantic_manifest__with_primary_transforms)
+    metric = copied_manifest.metrics[0]
+    metric.name = f"@{metric.name}"
+    with pytest.raises(
+        SemanticManifestValidationException,
+        match=rf"Invalid name `{metric.name}",
+    ):
+        validator.checked_validations(copied_manifest)
+
+
 def test_duplicate_metric_name(  # noqa:D
     simple_semantic_manifest__with_primary_transforms: PydanticSemanticManifest,
 ) -> None:
@@ -57,7 +97,7 @@ def test_duplicate_metric_name(  # noqa:D
     model.metrics.append(duplicated_metric)
     with pytest.raises(
         SemanticManifestValidationException,
-        match=rf"Can't use name `{duplicated_metric.name}` for a metric when it was already used for a metric",
+        match=rf"Can't use name `{duplicated_metric.name}` for a metric when it was already used for another metric",
     ):
         SemanticManifestValidator[PydanticSemanticManifest]([UniqueAndValidNameRule()]).checked_validations(model)