Improve Update Mechanism bug link validation

Includes parsing the bug number and returning a standard format e.g.
`https://crbug.com/421989967`

Bug: 421989967
Change-Id: I4e159e6ab8ddd1d6fb253a360db8b48ee5a68c1f

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)'"
+                    f"Example: '{mechanism} (https://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)'"
                 ])
 
-        # The bug link must be for the public tracker or 'b/' for internal.
+        # Autoroll must not have a bug link.
-        elif primary == "Autoroll" and 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

metadata/tests/fields_test.py

@@ -270,15 +270,25 @@ class FieldValidationTest(unittest.TestCase):
             valid_values=[
                 "Autoroll",
                 "  Autoroll  ",
-                "Manual (crbug.com/12345)",
+                "Manual (https://crbug.com/12345)",
-                "Static (crbug.com/54321)",
+                "Static (https://crbug.com/54321)",
-                "Static.HardFork (crbug.com/98765)",
+                "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",

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"),
+        expect("Manual (https://crbug.com/12345)", ("Manual", None, "https://crbug.com/12345"),
-               "parse a valid value with a bug")
+               "parse a valid value with https://crbug.com/ format")
-        expect("Manual(crbug.com/12345)", ("Manual", None, "crbug.com/12345"),
-               "parse a valid value with no space")
         expect("Manual", ("Manual", None, None),
                "allow Manual with no bug (should still warn)")
-        expect("Static.HardFork (crbug.com/12345)",
+        expect("Static.HardFork (https://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 +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__":