diff --git a/metadata/fields/custom/update_mechanism.py b/metadata/fields/custom/update_mechanism.py index 8e8f2265f..310ed81aa 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,11 @@ 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. +# It matches optional http(s)://, followed by crbug, optional .com, a slash, +# and captures the digits that follow. +BUG_LINK_REGEX = re.compile(r"^(?:https?://)?crbug(?:\.com)?/(\d+)$") # A set of the fully-qualified, allowed mechanism values. ALLOWED_MECHANISMS = { @@ -85,7 +89,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 +100,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 +109,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 +118,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 +134,30 @@ 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 bug_link != canonical_bug_link or not bug_num.isdigit(): + # If it ends in a '/number', then provide a copy pastable correct value. + if bug_num.isdigit(): + validation_result_type = vr.ValidationError + # If they've only missed 'http(s)://', or '.com', then just warn. + if BUG_LINK_REGEX.match(bug_link): + validation_result_type = vr.ValidationWarning + return validation_result_type( + 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 +172,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..50762ce65 100644 --- a/metadata/tests/fields_test.py +++ b/metadata/tests/fields_test.py @@ -270,19 +270,29 @@ 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)", ], warning_values=[ "Static", "Static.HardFork", + "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)", ], ) diff --git a/metadata/tests/type_narrowing_test.py b/metadata/tests/type_narrowing_test.py index 2c09c27f2..66a7f3a6f 100644 --- a/metadata/tests/type_narrowing_test.py +++ b/metadata/tests/type_narrowing_test.py @@ -222,24 +222,30 @@ 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"), + expect("Manual (crbug.com/12345)", ("Manual", None, "https://crbug.com/12345"), "parse a valid value with a bug") - expect("Manual(crbug.com/12345)", ("Manual", None, "crbug.com/12345"), + expect("Manual(crbug.com/12345)", ("Manual", None, "https://crbug.com/12345"), "parse a valid value with no space") + expect("Manual (https://crbug/12345)", ("Manual", None, "https://crbug.com/12345"), + "parse a valid value with https://crbug/ format") + expect("Manual (http://crbug.com/12345)", ("Manual", None, "https://crbug.com/12345"), + "parse a valid value with http://crbug.com/ format") + 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"), + ("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 +255,11 @@ 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") if __name__ == "__main__":