Update metadata validation logic (issue #924) (#1463)

* Validate metadata with attr

* Update and add tests

* Add check if file exists and update tests

* Format code and update validate metadata

* Revert changes to is_metadata_file()

* Remove format error: whitespaces

* Format test files

Co-authored-by: Anna Scholtz <anna@scholtzan.net>

bigquery_etl/metadata/parse_metadata.py

@@ -3,23 +3,54 @@
 import re
 import yaml
 import os
+import attr
+from typing import List, Optional, Dict
 
+from bigquery_etl.query_scheduling.utils import is_email
 
 METADATA_FILE = "metadata.yaml"
 
 
+@attr.s(auto_attribs=True)
 class Metadata:
-    """Representation of metadata file content."""
+    """
+    Representation of a Metadata configuration.
 
-    def __init__(
-        self, friendly_name=None, description=None, owners=[], labels={}, scheduling={}
-    ):
-        """Create a new Metadata instance."""
-        self.friendly_name = friendly_name
-        self.description = description
-        self.labels = labels
-        self.scheduling = scheduling
-        self.owners = owners
+    Uses attrs to simplify the class definition and provide validation.
+    Docs: https://www.attrs.org
+    """
+
+    friendly_name: str = attr.ib()
+    description: str = attr.ib()
+    owners: List[str] = attr.ib()
+    labels: Dict = attr.ib({})
+    scheduling: Optional[Dict] = attr.ib({})
+
+    @owners.validator
+    def validate_owners(self, attribute, value):
+        """Check that provided email addresses for owners are valid."""
+        if not all(map(lambda e: is_email(e), value)):
+            raise ValueError(f"Invalid email for owners: {value}.")
+
+    @labels.validator
+    def validate_labels(self, attribute, value):
+        """Check that labels are valid."""
+        for key, label in value.items():
+            if not isinstance(label, bool):
+                if not Metadata.is_valid_label(str(key)):
+                    raise ValueError(
+                        f"""Invalid label key format: {key}.
+                                    Key cannot be empty. Only hyphens(-), underscores(_),
+                                    lowercase characters, and numbers are allowed.
+                                    International characters are not allowed."""
+                    )
+                elif not Metadata.is_valid_label(str(label)) and not label == "":
+                    raise ValueError(
+                        f"""Invalid label value format: {label}.
+                                    Value be empty. Only hyphens(-), underscores(_),
+                                    lowercase characters, and numbers are allowed.
+                                    International characters are not allowed."""
+                    )
 
     @staticmethod
     def is_valid_label(label):
@@ -78,27 +109,13 @@ class Metadata:
                     labels = {}
 
                     for key, label in metadata["labels"].items():
-                        if isinstance(label, bool) and Metadata.is_valid_label(
-                            str(key)
-                        ):
+                        if isinstance(label, bool):
                             # publish key-value pair with bool value as tag
                             if label:
                                 labels[str(key)] = ""
-                        elif Metadata.is_valid_label(
-                            str(key)
-                        ) and Metadata.is_valid_label(str(label)):
+                        else:
                             # all other pairs get published as key-value pair label
                             labels[str(key)] = str(label)
-                        else:
-                            print(
-                                """
-                                Invalid label format: {}: {}. Only hyphens (-),
-                                underscores (_), lowercase characters, and numbers
-                                are allowed. International characters are not allowed.
-                                """.format(
-                                    key, label
-                                )
-                            )
 
                 if "scheduling" in metadata:
                     scheduling = metadata["scheduling"]

tests/data/metadata.yaml

@@ -7,8 +7,6 @@ labels:
   public_json: true
   incremental: true
   incremental_export: true
-  invalid.label: foo
-  invalid_value: Fo.
   1232341234: valid
   1234_abcd: valid
   number_value: 1234234

tests/metadata/test_parse_metadata.py

@@ -7,6 +7,37 @@ TEST_DIR = Path(__file__).parent.parent
 
 
 class TestParseMetadata(object):
+    def test_metadata_instantiation(self):
+        metadata = Metadata(
+            "Test metadata", "test description", ["test@example.org"], {}
+        )
+
+        assert metadata.friendly_name == "Test metadata"
+        assert metadata.description == "test description"
+        assert metadata.owners == ["test@example.org"]
+        assert metadata.labels == {}
+        assert metadata.scheduling == {}
+
+    def test_invalid_owners(self):
+        with pytest.raises(ValueError):
+            Metadata("Test metadata", "test description", ["testexample.org"])
+
+    def test_invalid_label(self):
+        with pytest.raises(ValueError):
+            Metadata(
+                "Test metadata",
+                "test description",
+                ["test@example.org"],
+                {"INVALID-KEY": "foo"},
+            )
+        with pytest.raises(ValueError):
+            Metadata(
+                "Test metadata",
+                "test description",
+                ["test@example.org"],
+                {"foo": "INVALID-VALUE"},
+            )
+
     def test_is_valid_label(self):
         assert Metadata.is_valid_label("valid_label")
         assert Metadata.is_valid_label("valid-label1")
@@ -33,8 +64,6 @@ class TestParseMetadata(object):
         assert metadata.is_incremental()
         assert metadata.is_incremental_export()
         assert metadata.review_bug() is None
-        assert "invalid_value" not in metadata.labels
-        assert "invalid.label" not in metadata.labels
         assert "1232341234" in metadata.labels
         assert "1234_abcd" in metadata.labels
         assert "number_value" in metadata.labels