diff --git a/metadata/fields/custom/revision.py b/metadata/fields/custom/revision.py index 4ffa6df573..ac1157ca23 100644 --- a/metadata/fields/custom/revision.py +++ b/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 diff --git a/metadata/tests/dependency_metadata_test.py b/metadata/tests/dependency_metadata_test.py index 4c5383bc15..49cc9966fc 100644 --- a/metadata/tests/dependency_metadata_test.py +++ b/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() diff --git a/metadata/tests/type_narrowing_test.py b/metadata/tests/type_narrowing_test.py index 7b9861938f..7f17c1711d 100644 --- a/metadata/tests/type_narrowing_test.py +++ b/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/", - ["https://example.com/", "https://example2.com/"], - "return multiple valid urls") + expect( + "https://example.com/,\nhttps://example2.com/", + ["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"], - "reject unsupported scheme") + expect( + "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, - "understand the this repo is canonical message") + expect( + "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", - "accepts ambiguous MM/DD format (better than no date info at all)") + "02/03/2000", + "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", - "return value as-is if it doesn't mean no modification") + expect( + "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()