Updating Revision presubmit to check for valid commit hash

Bug:b/349274008
Change-Id: I6ce8f1993bcf78514111d709b69b109e481d18b7
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/5652700
Reviewed-by: Jiewei Qian <qjw@chromium.org>
Reviewed-by: Rachael Newitt <renewitt@google.com>
Commit-Queue: Jordan Brown <rop@google.com>

metadata/fields/custom/revision.py

@@ -20,6 +20,8 @@ import metadata.fields.custom.version as version_field
 import metadata.fields.util as util
 import metadata.validation_result as vr
 
+HEX_PATTERN = re.compile(r"^[a-fA-F0-9]{7,40}$")
+
 
 class RevisionField(field_types.SingleLineTextField):
     """Custom field for the revision."""
@@ -38,4 +40,37 @@ class RevisionField(field_types.SingleLineTextField):
         if util.is_known_invalid_value(value):
             return None
 
+        if not HEX_PATTERN.match(value):
+            return None
+
         return value
+
+    def validate(self, value: str) -> Optional[vr.ValidationResult]:
+        """Validates the revision string.
+
+        Checks:
+          - Non-empty value.
+          - Valid hexadecimal format (length 7-40 characters).
+        """
+
+        if util.is_unknown(value):
+            return vr.ValidationWarning(
+                reason=f"{self._name} is invalid.",
+                additional=[
+                    "Revision is required for dependencies which have a git repository "
+                    "as an upstream, OPTIONAL if the upstream is not a git repository "
+                    "and either Version or Date is supplied.",
+                    "'{value}' is not a valid commit hash.",
+                ],
+            )
+
+        if not HEX_PATTERN.match(value):
+            return vr.ValidationError(
+                reason=f"{self._name} is not a valid hexadecimal revision.",
+                additional=[
+                    "Revisions must be hexadecimal strings with a length of 7 to 40 characters."
+                ],
+            )
+
+        # Valid revision.
+        return None

metadata/tests/dependency_metadata_test.py

@@ -114,14 +114,13 @@ class DependencyValidationTest(unittest.TestCase):
 
     def test_versioning_field(self):
         """Check that a validation error is returned for insufficient
-        versioning info."""
+        versioning info. No Date/Revision and Version is N/A."""
         dependency = dm.DependencyMetadata()
         dependency.add_entry(known_fields.NAME.get_name(),
                              "Test metadata missing versioning info")
         dependency.add_entry(known_fields.URL.get_name(),
                              "https://www.example.com")
         dependency.add_entry(known_fields.VERSION.get_name(), "N/A")
-        dependency.add_entry(known_fields.REVISION.get_name(), "unknown")
         dependency.add_entry(known_fields.LICENSE.get_name(), "Public Domain")
         dependency.add_entry(known_fields.LICENSE_FILE.get_name(), "LICENSE")
         dependency.add_entry(known_fields.SECURITY_CRITICAL.get_name(), "no")
@@ -136,6 +135,89 @@ class DependencyValidationTest(unittest.TestCase):
         self.assertEqual(results[0].get_reason(),
                          "Versioning fields are insufficient.")
 
+    def test_versioning_with_invalid_revision(self):
+        """Check that a validation error is returned for insufficient
+        versioning info."""
+        dependency = dm.DependencyMetadata()
+        dependency.add_entry(known_fields.NAME.get_name(),
+                             "Test metadata missing versioning info")
+        dependency.add_entry(known_fields.URL.get_name(),
+                             "https://www.example.com")
+        dependency.add_entry(known_fields.VERSION.get_name(), "N/A")
+        dependency.add_entry(known_fields.REVISION.get_name(), "N/A")
+        dependency.add_entry(known_fields.LICENSE.get_name(), "Public Domain")
+        dependency.add_entry(known_fields.LICENSE_FILE.get_name(), "LICENSE")
+        dependency.add_entry(known_fields.SECURITY_CRITICAL.get_name(), "no")
+        dependency.add_entry(known_fields.SHIPPED.get_name(), "no")
+
+        results = dependency.validate(
+            source_file_dir=os.path.join(_THIS_DIR, "data"),
+            repo_root_dir=_THIS_DIR,
+        )
+        self.assertEqual(len(results), 2)
+        self.assertTrue(isinstance(results[0], vr.ValidationError))
+        self.assertTrue(isinstance(results[1], vr.ValidationError))
+        self.assertEqual(results[0].get_reason(),
+                         "Revision is not a valid hexadecimal revision.")
+        self.assertEqual(results[1].get_reason(),
+                         "Versioning fields are insufficient.")
+
+    def test_invalid_revision(self):
+        """Check invalid revision formats return validation errors."""
+
+        # Test invalid revision format (non-hexadecimal).
+        dependency = dm.DependencyMetadata()
+        dependency.add_entry(known_fields.NAME.get_name(), "Invalid Revision")
+        dependency.add_entry(known_fields.URL.get_name(),
+                             "https://www.example.com")
+        dependency.add_entry(known_fields.VERSION.get_name(), "1.0.0")
+        dependency.add_entry(known_fields.REVISION.get_name(),
+                             "invalid_revision")
+        dependency.add_entry(known_fields.LICENSE.get_name(), "Public Domain")
+        dependency.add_entry(known_fields.LICENSE_FILE.get_name(), "LICENSE")
+        dependency.add_entry(known_fields.SECURITY_CRITICAL.get_name(), "no")
+        dependency.add_entry(known_fields.SHIPPED.get_name(), "no")
+
+        results = dependency.validate(
+            source_file_dir=os.path.join(_THIS_DIR, "data"),
+            repo_root_dir=_THIS_DIR,
+        )
+        self.assertEqual(len(results), 1)
+        self.assertTrue(isinstance(results[0], vr.ValidationError))
+        self.assertEqual(
+            results[0].get_reason(),
+            "Revision is not a valid hexadecimal revision.",
+        )
+
+    def test_valid_revision(self):
+        """Check valid revision formats return no validation issues."""
+
+        dependency = dm.DependencyMetadata()
+        dependency.add_entry(known_fields.NAME.get_name(), "Valid Revision")
+        dependency.add_entry(known_fields.URL.get_name(),
+                             "https://www.example.com")
+        dependency.add_entry(known_fields.VERSION.get_name(), "1.0.0")
+        dependency.add_entry(known_fields.LICENSE.get_name(), "Public Domain")
+        dependency.add_entry(known_fields.LICENSE_FILE.get_name(), "LICENSE")
+        dependency.add_entry(known_fields.SECURITY_CRITICAL.get_name(), "no")
+        dependency.add_entry(known_fields.SHIPPED.get_name(), "no")
+
+        results = dependency.validate(
+            source_file_dir=os.path.join(_THIS_DIR, "data"),
+            repo_root_dir=_THIS_DIR,
+        )
+        # No errors for no revision.
+        self.assertEqual(len(results), 0)
+
+        dependency.add_entry(known_fields.REVISION.get_name(),
+                             "abcdef1")  # Valid.
+        results = dependency.validate(
+            source_file_dir=os.path.join(_THIS_DIR, "data"),
+            repo_root_dir=_THIS_DIR,
+        )
+        # No errors for valid revision.
+        self.assertEqual(len(results), 0)
+
     def test_required_field(self):
         """Check that a validation error is returned for a missing field."""
         dependency = dm.DependencyMetadata()

metadata/tests/type_narrowing_test.py

@@ -28,9 +28,11 @@ class FieldValidationTest(unittest.TestCase):
         def expect(value: str, expected_value: Any, reason: str):
             output = field.narrow_type(value)
             self.assertEqual(
-                output, expected_value,
+                output,
+                expected_value,
                 f'Field "{field.get_name()}" should {reason}. Input value'
-                f' was: "{value}", but got coerced into {repr(output)}')
+                f' was: "{value}", but got coerced into {repr(output)}',
+            )
 
         return expect
 
@@ -49,17 +51,25 @@ class FieldValidationTest(unittest.TestCase):
         expect("", None, "treat empty string as None")
         expect("https://example.com/", ["https://example.com/"],
                "return valid url")
-        expect("https://example.com/,\nhttps://example2.com/",
+        expect(
-               ["https://example.com/", "https://example2.com/"],
+            "https://example.com/,\nhttps://example2.com/",
-               "return multiple valid urls")
+            ["https://example.com/", "https://example2.com/"],
+            "return multiple valid urls",
+        )
         expect("file://test", [], "reject unsupported scheme")
-        expect("file://test,\nhttps://example.com", ["https://example.com"],
+        expect(
-               "reject unsupported scheme")
+            "file://test,\nhttps://example.com",
+            ["https://example.com"],
+            "reject unsupported scheme",
+        )
         expect("HTTPS://example.com", ["https://example.com"],
                "canonicalize url")
         expect("http", [], "reject invalid url")
-        expect("This is the canonical repo.", None,
+        expect(
-               "understand the this repo is canonical message")
+            "This is the canonical repo.",
+            None,
+            "understand the this repo is canonical message",
+        )
 
     def test_version(self):
         expect = self._test_on_field(fields.VERSION)
@@ -80,8 +90,10 @@ class FieldValidationTest(unittest.TestCase):
                "accepts ISO 8601 date time")
         expect("Jan 2 2024", "2024-01-02", "accepts locale format")
         expect(
-            "02/03/2000", "2000-03-02",
+            "02/03/2000",
-            "accepts ambiguous MM/DD format (better than no date info at all)")
+            "2000-03-02",
+            "accepts ambiguous MM/DD format (better than no date info at all)",
+        )
         expect("11/30/2000", "2000-11-30", "accepts unambiguous MM/DD format")
 
     def test_revision(self):
@@ -92,6 +104,24 @@ class FieldValidationTest(unittest.TestCase):
         expect("see deps", None, "treat invalid value as None")
         expect("N/A", None, "N/A is treated as None")
         expect("Not applicable.", None, "N/A is treated as None")
+        expect("invalid", None, "treat invalid hex as None")
+        expect("123456", None, "treat too short hex as None")
+        expect(
+            "0123456789abcdef0123456789abcdef01234567abcabc",
+            None,
+            "treat long hex (>40) as None",
+        )
+        expect("varies", None, "treat varies as None")
+        expect("see deps", None, "treat see deps as None")
+        expect("N/A", None, "treat N/A as None")
+        expect("Not applicable.", None, "treat 'Not applicable.' as None")
+        expect("Head", None, "treat 'Head' as None")
+        expect("abcdef1", "abcdef1", "leave valid hex unchanged")
+        expect(
+            "abcdef1abcdef1",
+            "abcdef1abcdef1",
+            "leave valid hex unchanged if between 7-40 chars",
+        )
 
     def test_license(self):
         expect = self._test_on_field(fields.LICENSE)
@@ -157,8 +187,11 @@ class FieldValidationTest(unittest.TestCase):
         expect("(none)", False, "understands none")
         expect("not applicable", False, "understands N/A")
         expect("", False, "treat empty string as False")
-        expect("modified X file", "modified X file",
+        expect(
-               "return value as-is if it doesn't mean no modification")
+            "modified X file",
+            "modified X file",
+            "return value as-is if it doesn't mean no modification",
+        )
 
     def test_dependency_data_return_as_property(self):
         dm = DependencyMetadata()