diff --git a/metadata/fields/custom/update_mechanism.py b/metadata/fields/custom/update_mechanism.py index 8e8f2265f..c155b0382 100644 --- a/metadata/fields/custom/update_mechanism.py +++ b/metadata/fields/custom/update_mechanism.py @@ -25,7 +25,7 @@ UPDATE_MECHANISM_REGEX = re.compile( ([^\s(]+) # Capture the secondary mechanism. )? # Indicates 'secondary' is optional. - # Group 3 (optional): A bug link in parentheses (e.g., "(crbug.com/12345)"). + # Group 3 (optional): A bug link in parentheses (e.g., "(https://crbug.com/12345)"). (?: # Use a non-capturing group to catch the parentheses and whitespace. \s* # Optional whitespace. \( # ( @@ -36,7 +36,9 @@ UPDATE_MECHANISM_REGEX = re.compile( $ # End of the string. """, re.VERBOSE) -BUG_PREFIXES = ['crbug.com/'] + +# Regex for validating the format of the bug link and capturing the bug ID. +BUG_LINK_REGEX = re.compile(r"^https://crbug\.com/(\d+)$") # A set of the fully-qualified, allowed mechanism values. ALLOWED_MECHANISMS = { @@ -85,7 +87,7 @@ class UpdateMechanismField(field_types.SingleLineTextField): reason=f"{self._name} field cannot be empty.", additional=[ f"Must be one of {util.quoted(sorted(ALLOWED_MECHANISMS))}.", - "Example: 'Autoroll' or 'Manual (crbug.com/12345)'" + "Example: 'Autoroll' or 'Manual (https://crbug.com/12345)'" ]) primary, secondary, bug_link = parse_update_mechanism(value) @@ -96,7 +98,7 @@ class UpdateMechanismField(field_types.SingleLineTextField): additional=[ "Expected format: Mechanism[.SubMechanism] [(bug)]", f"Allowed mechanisms: {util.quoted(sorted(ALLOWED_MECHANISMS))}.", - "Example: 'Static.HardFork (crbug.com/12345)'", + "Example: 'Static.HardFork (https://crbug.com/12345)'", ]) mechanism = primary @@ -105,7 +107,7 @@ class UpdateMechanismField(field_types.SingleLineTextField): # Second, check if the mechanism is a known, allowed value. if mechanism not in ALLOWED_MECHANISMS: return vr.ValidationError( - reason=f"Invalid mechanism '{mechanism}'.", + reason=f"{self._name} has invalid mechanism '{mechanism}'.", additional=[ f"Must be one of {util.quoted(sorted(ALLOWED_MECHANISMS))}.", ]) @@ -114,24 +116,14 @@ class UpdateMechanismField(field_types.SingleLineTextField): # Only warn for Static, for now. elif primary == "Static" and bug_link is None: return vr.ValidationWarning( - reason="No bug link to autoroll exception provided.", + reason="{self._name} has no link to autoroll exception.", additional=[ "Please add a link if an exception bug has been filed.", - f"Example: '{mechanism} (crbug.com/12345)'" - ]) - - # The bug link must be for the public tracker or 'b/' for internal. - elif bug_link and not any(x in bug_link for x in BUG_PREFIXES): - return vr.ValidationError( - reason= - f"Bug links must begin with {util.quoted(sorted(BUG_PREFIXES))}.", - additional=[ - f"Please add a bug link using {util.quoted(sorted(BUG_PREFIXES))} in parentheses.", - f"Example: '{mechanism} (crbug.com/12345)'" + f"Example: '{mechanism} (https://crbug.com/12345)'" ]) - # The bug link must be for the public tracker or 'b/' for internal. - elif primary == "Autoroll" and bug_link: + # Autoroll must not have a bug link. + if primary == "Autoroll" and bug_link: return vr.ValidationError( reason="Autoroll does not permit an autoroll exception.", additional=[ @@ -140,6 +132,26 @@ class UpdateMechanismField(field_types.SingleLineTextField): "You could move it to the description.", ]) + # Validate the bug link format if present. + if bug_link: + bug_num = bug_link.split("/")[-1] + canonical_bug_link = f"https://crbug.com/{bug_num}" + if not BUG_LINK_REGEX.match(bug_link): + # If it ends in a '/number', then provide a copy pastable correct value. + if bug_num.isdigit(): + return vr.ValidationError( + reason=f"{self._name} bug link should be `({canonical_bug_link})`.", + additional=[ + f"{bug_link} is not a valid crbug link." + ]) + # Does not match the expected crbug link format at all. + return vr.ValidationError( + reason=f"{self._name} has invalid bug link format '{bug_link}'.", + additional=[ + "Bug links must be of the form (https://crbug.com/12345).", + f"Example: '{mechanism} (https://crbug.com/12345)'", + ]) + return None def narrow_type(self, @@ -154,4 +166,12 @@ class UpdateMechanismField(field_types.SingleLineTextField): if validation and validation.is_fatal(): # It cannot be narrowed if there is a fatal error. return None, None, None - return parse_update_mechanism(value) + + primary, secondary, bug_link = parse_update_mechanism(value) + + # If a bug link is present, parse it into canonical format. + if bug_link: + bug_num = bug_link.split("/")[-1] + bug_link = f"https://crbug.com/{bug_num}" + + return primary, secondary, bug_link diff --git a/metadata/tests/fields_test.py b/metadata/tests/fields_test.py index f2d5a32ee..d413f9dd5 100644 --- a/metadata/tests/fields_test.py +++ b/metadata/tests/fields_test.py @@ -270,15 +270,25 @@ class FieldValidationTest(unittest.TestCase): valid_values=[ "Autoroll", " Autoroll ", - "Manual (crbug.com/12345)", - "Static (crbug.com/54321)", - "Static.HardFork (crbug.com/98765)", + "Manual (https://crbug.com/12345)", + "Static (https://crbug.com/54321)", + "Static.HardFork (https://crbug.com/98765)", ], error_values=[ "", " ", "Invalid Value", "Custom (crbug.com/123)", + "Custom (https://crbug.com/123)", + "Manual (https://crbug.com/12345 )", + "Manual (https://crbug.com/12345a)", + "Manual (crbug.com/12345)", + "Static (crbug.com/54321)", + "Static (crbug/54321)", + "Static (http://crbug/54321)", + "Static (http://crbug.com/54321)", + "Static (https://crbug/54321)", + "Static.HardFork (crbug.com/98765)", ], warning_values=[ "Static", diff --git a/metadata/tests/type_narrowing_test.py b/metadata/tests/type_narrowing_test.py index 2c09c27f2..ff370c63e 100644 --- a/metadata/tests/type_narrowing_test.py +++ b/metadata/tests/type_narrowing_test.py @@ -222,24 +222,22 @@ class FieldValidationTest(unittest.TestCase): expect = self._test_on_field( metadata.fields.custom.update_mechanism.UpdateMechanismField()) - # Test cases that should successfully parse + # Test cases that should successfully parse. expect("Autoroll", ("Autoroll", None, None), "parse a valid value with no bug") expect(" Autoroll ", ("Autoroll", None, None), "parse a valid value and strip whitespace") - expect("Manual (crbug.com/12345)", ("Manual", None, "crbug.com/12345"), - "parse a valid value with a bug") - expect("Manual(crbug.com/12345)", ("Manual", None, "crbug.com/12345"), - "parse a valid value with no space") + expect("Manual (https://crbug.com/12345)", ("Manual", None, "https://crbug.com/12345"), + "parse a valid value with https://crbug.com/ format") expect("Manual", ("Manual", None, None), "allow Manual with no bug (should still warn)") - expect("Static.HardFork (crbug.com/12345)", - ("Static", "HardFork", "crbug.com/12345"), + expect("Static.HardFork (https://crbug.com/12345)", + ("Static", "HardFork", "https://crbug.com/12345"), "parse a namespaced value with a bug") expect("Static", ("Static", None, None), "Allow Static with no bug (should still warn)") - # Test cases that are invalid and should not be parsed + # Test cases that are invalid and should not be parsed. expect("", (None, None, None), "treat empty string as None") expect(" ", (None, None, None), "treat whitespace-only string as None") expect("Invalid", (None, None, None), @@ -249,9 +247,17 @@ class FieldValidationTest(unittest.TestCase): expect("Static.HardFork (A bug link)", (None, None, None), "parse a namespaced value with an invalid comment") expect("Manual (b/12345)", (None, None, None), - "do not parse a bug with b/") + "not parse a bug with b/") expect("Autoroll (crbug.com/12345)", (None, None, None), - "do not allow autoroll with a bug") + "not allow autoroll with a bug") + expect("Manual (crbug.com/12345 )", (None, None, None), + "not pare a bug with a trailing space") + expect("Manual (crbug.com/12345)", (None, None, None), + "not parse a value without https://") + expect("Manual (https://crbug/12345)", (None, None, None), + "not parse a value without .com") + expect("Manual (http://crbug.com/12345)", (None, None, None), + "not parse a value without https://") if __name__ == "__main__":