[ssci] Modify metadata versioning info validation

This CL changes what is considered valid versioning info. Instead of
both Date and Revision being required if Version was unknown, now only
one of Date or Revision has to be specified.

Bug: b:277147404
Change-Id: Iedb06e2d55f0cd0ef0a2931013a2a52b15befd75
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/tools/depot_tools/+/4852699
Reviewed-by: Gavin Mak <gavinmak@google.com>
Commit-Queue: Anne Redulla <aredulla@google.com>
Reviewed-by: Rachael Newitt <renewitt@google.com>

metadata/dependency_metadata.py

@@ -68,13 +68,6 @@ class DependencyMetadata:
         """
         required = set(self._MANDATORY_FIELDS)
 
-        # The date and revision are required if the version has not
-        # been specified.
-        version_value = self._metadata.get(known_fields.VERSION)
-        if version_value is None or version_util.is_unknown(version_value):
-            required.add(known_fields.DATE)
-            required.add(known_fields.REVISION)
-
         # Assume the dependency is shipped if not specified.
         shipped_value = self._metadata.get(known_fields.SHIPPED)
         is_shipped = (shipped_value is None
@@ -138,6 +131,25 @@ class DependencyMetadata:
                     reason=f"Required field '{field_name}' is missing.")
                 results.append(error)
 
+        # At least one of the fields Version, Date or Revision must be
+        # provided.
+        version_value = self._metadata.get(known_fields.VERSION)
+        date_value = self._metadata.get(known_fields.DATE)
+        revision_value = self._metadata.get(known_fields.REVISION)
+        if ((not version_value or version_util.is_unknown(version_value))
+                and (not date_value or util.is_unknown(date_value))
+                and (not revision_value or util.is_unknown(revision_value))):
+            versioning_fields = [
+                known_fields.VERSION, known_fields.DATE, known_fields.REVISION
+            ]
+            names = util.quoted(
+                [field.get_name() for field in versioning_fields])
+            error = vr.ValidationError(
+                reason="Versioning fields are insufficient.",
+                additional=[f"Provide at least one of [{names}]."],
+            )
+            results.append(error)
+
         # Validate values for all present fields.
         for field, value in self._metadata.items():
             field_result = field.validate(value)

metadata/tests/dependency_metadata_test.py

@@ -44,6 +44,30 @@ class DependencyValidationTest(unittest.TestCase):
         self.assertTrue(isinstance(results[0], vr.ValidationError))
         self.assertEqual(results[0].get_reason(), "There is a repeated field.")
 
+    def test_versioning_field(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(), "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")
+        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(),
+                         "Versioning fields are insufficient.")
+
     def test_required_field(self):
         """Check that a validation error is returned for a missing field."""
         dependency = dm.DependencyMetadata()
@@ -132,6 +156,25 @@ class DependencyValidationTest(unittest.TestCase):
         )
         self.assertEqual(len(results), 4)
 
+    def test_valid_metadata(self):
+        """Check valid metadata returns no validation issues."""
+        dependency = dm.DependencyMetadata()
+        dependency.add_entry(known_fields.NAME.get_name(),
+                             "Test valid metadata")
+        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,
+        )
+        self.assertEqual(len(results), 0)
+
 
 if __name__ == "__main__":
     unittest.main()

metadata/tests/validate_test.py

@@ -53,7 +53,7 @@ class ValidateContentTest(unittest.TestCase):
             source_file_dir=_SOURCE_FILE_DIR,
             repo_root_dir=_THIS_DIR,
         )
-        self.assertEqual(len(results), 11)
+        self.assertEqual(len(results), 9)
         error_count = 0
         warning_count = 0
         for result in results:
@@ -61,7 +61,7 @@ class ValidateContentTest(unittest.TestCase):
                 error_count += 1
             else:
                 warning_count += 1
-        self.assertEqual(error_count, 9)
+        self.assertEqual(error_count, 7)
         self.assertEqual(warning_count, 2)
 
 
@@ -91,7 +91,7 @@ class ValidateFileTest(unittest.TestCase):
             filepath=_INVALID_METADATA_FILEPATH,
             repo_root_dir=_THIS_DIR,
         )
-        self.assertEqual(len(results), 11)
+        self.assertEqual(len(results), 9)
         error_count = 0
         warning_count = 0
         for result in results:
@@ -99,7 +99,7 @@ class ValidateFileTest(unittest.TestCase):
                 error_count += 1
             else:
                 warning_count += 1
-        self.assertEqual(error_count, 9)
+        self.assertEqual(error_count, 7)
         self.assertEqual(warning_count, 2)
 
 
@@ -135,10 +135,10 @@ class CheckFileTest(unittest.TestCase):
         )
         # TODO(aredulla): update this test once validation errors can be
         # returned as errors. Bug: b/285453019.
-        # self.assertEqual(len(errors), 9)
+        # self.assertEqual(len(errors), 7)
         # self.assertEqual(len(warnings), 2)
         self.assertEqual(len(errors), 0)
-        self.assertEqual(len(warnings), 11)
+        self.assertEqual(len(warnings), 9)
 
 
 if __name__ == "__main__":

tests/presubmit_canned_checks_test.py

@@ -390,13 +390,14 @@ class ChromiumDependencyMetadataCheckTest(unittest.TestCase):
         results = presubmit_canned_checks.CheckChromiumDependencyMetadata(
             input_api, MockOutputApi())
 
-        # There should be 10 results due to
+        # There should be 9 results due to
         # - missing 5 mandatory fields: Name, URL, Version, License, and
         #                               Security Critical
-        # - missing 4 required fields: Date, Revision, License File, and
+        # - 1 error for insufficent versioning info
+        # - missing 2 required fields: License File, and
         #                              License Android Compatible
         # - Shipped should be only 'yes' or 'no'.
-        self.assertEqual(len(results), 10)
+        self.assertEqual(len(results), 9)
 
         # Check each presubmit result is associated with the test file.
         for result in results: