aiden
/
depot_tools
mirror of https://chromium.googlesource.com/chromium/tools/depot_tools
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Improve Update Mechanism bug link validation

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

Bug: 421989967
Change-Id: I4e159e6ab8ddd1d6fb253a360db8b48ee5a68c1f
changes/73/6669073/6
Jordan Brown 3 days ago
parent 4c626451ae
commit d03e4f3019
2 changed files with 53 additions and 25 deletions
Show Stats Download Patch File Download Diff File

58
metadata/fields/custom/update_mechanism.py
Unescape Escape View File

@ -25,7 +25,7 @@ UPDATE_MECHANISM_REGEX = re.compile(
([^\s(]+) # Capture the secondary mechanism. ([^\s(]+) # Capture the secondary mechanism.
)? # Indicates 'secondary' is optional. )? # 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. (?: # Use a non-capturing group to catch the parentheses and whitespace.
\s* # Optional whitespace. \s* # Optional whitespace.
\( # ( \( # (
@ -36,7 +36,11 @@ UPDATE_MECHANISM_REGEX = re.compile(
$ # End of the string. $ # End of the string.
""", re.VERBOSE) """, 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. # A set of the fully-qualified, allowed mechanism values.
ALLOWED_MECHANISMS = { ALLOWED_MECHANISMS = {
@ -85,7 +89,7 @@ class UpdateMechanismField(field_types.SingleLineTextField):
reason=f"{self._name} field cannot be empty.", reason=f"{self._name} field cannot be empty.",
additional=[ additional=[
f"Must be one of {util.quoted(sorted(ALLOWED_MECHANISMS))}.", 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) primary, secondary, bug_link = parse_update_mechanism(value)
@ -96,7 +100,7 @@ class UpdateMechanismField(field_types.SingleLineTextField):
additional=[ additional=[
"Expected format: Mechanism[.SubMechanism] [(bug)]", "Expected format: Mechanism[.SubMechanism] [(bug)]",
f"Allowed mechanisms: {util.quoted(sorted(ALLOWED_MECHANISMS))}.", f"Allowed mechanisms: {util.quoted(sorted(ALLOWED_MECHANISMS))}.",
"Example: 'Static.HardFork (crbug.com/12345)'", "Example: 'Static.HardFork (https://crbug.com/12345)'",
]) ])
mechanism = primary mechanism = primary
@ -105,7 +109,7 @@ class UpdateMechanismField(field_types.SingleLineTextField):
# Second, check if the mechanism is a known, allowed value. # Second, check if the mechanism is a known, allowed value.
if mechanism not in ALLOWED_MECHANISMS: if mechanism not in ALLOWED_MECHANISMS:
return vr.ValidationError( return vr.ValidationError(
reason=f"Invalid mechanism '{mechanism}'.", reason=f"{self._name} has invalid mechanism '{mechanism}'.",
additional=[ additional=[
f"Must be one of {util.quoted(sorted(ALLOWED_MECHANISMS))}.", f"Must be one of {util.quoted(sorted(ALLOWED_MECHANISMS))}.",
]) ])
@ -114,24 +118,34 @@ class UpdateMechanismField(field_types.SingleLineTextField):
# Only warn for Static, for now. # Only warn for Static, for now.
elif primary == "Static" and bug_link is None: elif primary == "Static" and bug_link is None:
return vr.ValidationWarning( return vr.ValidationWarning(
reason="No bug link to autoroll exception provided.", reason="{self._name} has no link to autoroll exception.",
additional=[ additional=[
"Please add a link if an exception bug has been filed.", "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. # Validate the bug link format if present.
elif bug_link and not any(x in bug_link for x in BUG_PREFIXES): if bug_link and not BUG_LINK_REGEX.match(bug_link):
return vr.ValidationError( bug_num = bug_link.split("/")[-1]
reason= # If it's a number, then provide a copy pastable correct value.
f"Bug links must begin with {util.quoted(sorted(BUG_PREFIXES))}.", if bug_num.isdigit():
additional=[ return vr.ValidationError(
f"Please add a bug link using {util.quoted(sorted(BUG_PREFIXES))} in parentheses.", reason=f"{self._name} bug link should be `(https://crbug.com/{bug_num})`",
f"Example: '{mechanism} (crbug.com/12345)'" additional=[
]) f"{bug_link} is not a valid crbug link."
"Note that 'http[s]://' and '.com' are optional.",
])
else:
return vr.ValidationError(
reason=f"{self._name} has invalid bug link {bug_link}",
additional=[
"bug links must be like (https://crbug.com/...)",
"'http[s]://' and '.com' are optional.",
f"Example: '{mechanism} (https://crbug.com/12345)'",
])
# The bug link must be for the public tracker or 'b/' for internal. # The bug link must be for the public tracker or 'b/' for internal.
elif primary == "Autoroll" and bug_link: if primary == "Autoroll" and bug_link:
return vr.ValidationError( return vr.ValidationError(
reason="Autoroll does not permit an autoroll exception.", reason="Autoroll does not permit an autoroll exception.",
additional=[ additional=[
@ -154,4 +168,12 @@ class UpdateMechanismField(field_types.SingleLineTextField):
if validation and validation.is_fatal(): if validation and validation.is_fatal():
# It cannot be narrowed if there is a fatal error. # It cannot be narrowed if there is a fatal error.
return None, None, None 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

20
metadata/tests/type_narrowing_test.py
Unescape Escape View File

@ -222,24 +222,30 @@ class FieldValidationTest(unittest.TestCase):
expect = self._test_on_field( expect = self._test_on_field(
metadata.fields.custom.update_mechanism.UpdateMechanismField()) metadata.fields.custom.update_mechanism.UpdateMechanismField())
# Test cases that should successfully parse # Test cases that should successfully parse.
expect("Autoroll", ("Autoroll", None, None), expect("Autoroll", ("Autoroll", None, None),
"parse a valid value with no bug") "parse a valid value with no bug")
expect(" Autoroll ", ("Autoroll", None, None), expect(" Autoroll ", ("Autoroll", None, None),
"parse a valid value and strip whitespace") "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") "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") "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), expect("Manual", ("Manual", None, None),
"allow Manual with no bug (should still warn)") "allow Manual with no bug (should still warn)")
expect("Static.HardFork (crbug.com/12345)", expect("Static.HardFork (crbug.com/12345)",
("Static", "HardFork", "crbug.com/12345"), ("Static", "HardFork", "https://crbug.com/12345"),
"parse a namespaced value with a bug") "parse a namespaced value with a bug")
expect("Static", ("Static", None, None), expect("Static", ("Static", None, None),
"Allow Static with no bug (should still warn)") "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 empty string as None")
expect(" ", (None, None, None), "treat whitespace-only string as None") expect(" ", (None, None, None), "treat whitespace-only string as None")
expect("Invalid", (None, None, None), expect("Invalid", (None, None, None),
@ -249,9 +255,9 @@ class FieldValidationTest(unittest.TestCase):
expect("Static.HardFork (A bug link)", (None, None, None), expect("Static.HardFork (A bug link)", (None, None, None),
"parse a namespaced value with an invalid comment") "parse a namespaced value with an invalid comment")
expect("Manual (b/12345)", (None, None, None), 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), expect("Autoroll (crbug.com/12345)", (None, None, None),
"do not allow autoroll with a bug") "not allow autoroll with a bug")
if __name__ == "__main__": if __name__ == "__main__":

Write Preview
Loading…
Cancel
Save