From 6740da7661b7d5347b1d4ba2513faea55d627e29 Mon Sep 17 00:00:00 2001 From: Kamil Páral Date: Dec 16 2020 10:20:35 +0000 Subject: pagure_bot: don't double-parse commands Before, we filtered commands from a comment text, but still kept them in a text form, and so when we wanted to process them in a later stage, we needed to parse them again (or rely on unsafe modifications). This patch avoids that by using data containers for parsed commands, and so the later stages can directly access structured data instead of parsing the text again. Merges: https://pagure.io/fedora-qa/blockerbugs/pull-request/164 Related: https://pagure.io/fedora-qa/blockerbugs/pull-request/163 --- diff --git a/blockerbugs/util/pagure_bot.py b/blockerbugs/util/pagure_bot.py index 3da93cc..91128d9 100644 --- a/blockerbugs/util/pagure_bot.py +++ b/blockerbugs/util/pagure_bot.py @@ -2,6 +2,7 @@ import datetime import re +import collections from blockerbugs import app from blockerbugs.util import pagure_interface @@ -24,6 +25,20 @@ OUTCOME_MATCHER = re.compile(OUTCOME_RE) SUMMARY_HEADER = "" CLOSED_VOTES_HEADER = "The following votes have been closed:" +# Containers for parsed commands +VoteCommand = collections.namedtuple('VoteCommand', ['tracker', 'vote']) +VoteCommand.__doc__ += ( + ': A parsed vote command, e.g. tracker="betablocker", vote="+1"') + +AgreedCommand = collections.namedtuple( + 'AgreedCommand', ['tracker', 'outcome', 'summary'], defaults=[None]) +AgreedCommand.__doc__ += ( + ': A parsed AGREED command, e.g. tracker="betablocker", ' + 'outcome="accepted", summary="Breaks boot."') + +RevoteCommand = collections.namedtuple('RevoteCommand', ['tracker']) +RevoteCommand.__doc__ += ': A parsed REVOTE command, e.g. tracker="betablocker"' + def agreed_revote_parser(line): """Parse an AGREED or a REVOTE line. If there's anything violating the @@ -32,38 +47,46 @@ def agreed_revote_parser(line): and return []. All instances of "fe" get expanded to "freezeexception". :param str line: a single line - :return: A list of commands with one tracker per line (or an empty list). - E.g. for line 'agreed acceptedbetablocker rejectedfinalfe' this returns: - ['agreed acceptedbetablocker', - 'agreed rejectedfinalfreezeexception'] - For line 'revote finalblocker 0day' this returns: - ['revote finalblocker', - 'revote 0day'] + :return: A list of :class:`AgreedCommand` or :class:`RevoteCommand` instances. + E.g. for the line 'agreed acceptedbetablocker rejectedfinalfe' this returns: + [AgreedCommand(tracker='betablocker', outcome='accepted'), + AgreedCommand(tracker='finalfreezeexception', outcome='rejected')] + For the line 'revote finalblocker 0day' this returns: + [RevoteCommand(tracker='finalblocker'), + RevoteCommand(tracker='0day')] """ # parsing word by word avoids complex and fragile regex syntax words = line.split() - # first word must be 'agreed' or 'revote' if len(words) <= 0: return [] - command = words[0] - if command == 'agreed': - matcher = OUTCOME_MATCHER - summary = True - elif command == 'revote': - matcher = TRACKER_MATCHER - summary = False out = [] + command = words[0] for word in words[1:]: - if matcher.fullmatch(word): - # if we get a match, save it as a valid command - word = expand_fe(word) - out.append(f'{command} {word}') - else: - # if not, we break and return the matches we got so far if summary - # is allowed, return empty if not - if summary: + if command == 'agreed': + match = OUTCOME_MATCHER.fullmatch(word) + if (match): + ac = AgreedCommand( + tracker=expand_fe(match.group(2)), + outcome=match.group(1), + ) + out.append(ac) + else: + # this must be the summary now + # FIXME: implement summary parsing break + elif command == 'revote': + match = TRACKER_MATCHER.fullmatch(word) + if (match): + rc = RevoteCommand( + tracker=expand_fe(word), + ) + out.append(rc) + else: + # this is not a valid REVOTE line + return [] + else: + # not an AGREED or REVOTE line return [] return out @@ -74,26 +97,32 @@ def vote_parser(line): invalidates the line. All instances of "fe" get expanded to "freezeexception". :param str line: a line which might contain a vote - :return: A list of votes with one tracker per line (or an empty list). - E.g. for line 'betafe +1 finalblocker -1' this returns: - ['betafreezeexception +1', - 'finalblocker -1'] + :return: A list of :class:`VoteCommand` instances. + E.g. for the line 'betafe +1 finalblocker -1' this returns: + [VoteCommand(tracker='betafreezeexception', vote='+1'), + VoteCommand(tracker='finalblocker', vote='-1')] """ - out = [] words = line.split() # if we have an odd number of words we don't have a clean vote line if (len(words) % 2) != 0: - return out + return [] + + out = [] # split list into pairs: # https://stackoverflow.com/questions/312443 for pair in (words[i:i + 2] for i in range(0, len(words), 2)): if TRACKER_MATCHER.fullmatch(pair[0]) and pair[1] in VOTES: - pair[0] = expand_fe(pair[0]) - out.append(" ".join(pair)) + vc = VoteCommand( + tracker=expand_fe(pair[0]), + vote=pair[1] + ) + out.append(vc) elif pair[0] in VOTES and TRACKER_MATCHER.fullmatch(pair[1]): - pair[1] = expand_fe(pair[1]) - pair.reverse() - out.append(" ".join(pair)) + vc = VoteCommand( + tracker=expand_fe(pair[1]), + vote=pair[0] + ) + out.append(vc) else: # we found something other than a vote pair, so per # Kamil's Strict Vote Parsing Regime, we reject the line @@ -101,37 +130,14 @@ def vote_parser(line): return out -def expand_fe(keyword): - '''Expand "FE" into "FreezeException" in tracker-related keywords. - - While we accept "FE" as a handy user abbreviation, we want to process - it as "FreezeException" internally, so that we don't need to handle both - variants everywhere in our code. - - :param str keyword: a tracker or an outcome keyword, lowercased - :return: a keyword expanded to the full "FreezeException" variant if - applicable, or the keyword unchanged) - ''' - tracker_match = TRACKER_MATCHER.fullmatch(keyword) - outcome_match = OUTCOME_MATCHER.fullmatch(keyword) - - if tracker_match: - # the 'fe' string is in group #3 in TRACKER_RE - fe_group_num = 3 - match = tracker_match - elif outcome_match: - # the 'fe' string is in group #4 in OUTCOME_RE - fe_group_num = 4 - match = outcome_match +def expand_fe(tracker): + '''Expand "fe" into "freezeexception" in tracker names.''' + if tracker == 'betafe': + return 'betafreezeexception' + elif tracker == 'finalfe': + return 'finalfreezeexception' else: - return keyword - - if match.group(fe_group_num) != 'fe': - return keyword - - start = match.start(fe_group_num) - end = start + len('fe') - return keyword[:start] + 'freezeexception' + keyword[end:] + return tracker def bot_loop_detected(comments): @@ -168,15 +174,16 @@ class Comment(): # FIXME it would be good to save/cache the data return self.user in pagure_interface.get_group('fedora-qa')['members'] - def keywords(self): - '''Search for command keywords in this comment, purge anything - unrelated and return it. + def commands(self): + '''Search for commands in this comment, purge anything unrelated and + return it parsed. - :return: a list with one vote or command per line, e.g.: - ['betablocker +1', - 'agreed acceptedbetablocker', - 'revote betablocker', - 'betablocker -1'] + :return: a list of :class:`VoteCommand`, :class:`AgreedCommand`, and + :class:`RevoteCommand` instances, e.g.: + [VoteCommand(tracker='betablocker', vote='+1'), + AgreedCommand(tracker='betablocker', outcome='accepted'), + RevoteCommand(tracker='betablocker'), + VoteCommand(tracker='betablocker', vote='-1')] ''' out = [] for line in self.text.lower().split('\n'): @@ -232,22 +239,15 @@ class BugVoteTracker(): if comment.user == app.config['PAGURE_BOT_USERNAME']: return - for words in [line.split() for line in comment.keywords()]: - agreed = (words[0] == 'agreed') - revote = (words[0] == 'revote') - - if agreed: - match = OUTCOME_MATCHER.fullmatch(words[1]) - outcome = match.group(1) - tracker = match.group(2) - elif revote: - tracker = words[1] - else: # a vote - tracker = words[0] - - if tracker != self.tracker: + for command in comment.commands(): + if command.tracker != self.tracker: + # not intended for this tracker, ignore continue + agreed = isinstance(command, AgreedCommand) + revote = isinstance(command, RevoteCommand) + vote = isinstance(command, VoteCommand) + if (agreed or revote) and not comment.user_is_admin(): app.logger.debug( f'A non-admin user {comment.user} tries to perform ' @@ -257,21 +257,24 @@ class BugVoteTracker(): if agreed: self.open = False - self.outcome = outcome + self.outcome = command.outcome self.need_summary_post = True elif revote: self.open = True self.outcome = None self.need_summary_post = False self.votes = {} - else: # a vote + elif vote: if not self.open: continue - assert words[1] in VOTES self.votes[comment.user] = { - 'vote': words[1], + 'vote': command.vote, 'comment_id': comment.id } + else: + msg = f'Unknown command instance: {command}' + app.logger.error(msg) + assert False, msg def enumerate_votes(self): '''Return self.votes in a format: