From a6bcbbbda5bf821af5fdb50ce2ac09156728b106 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Mon, 23 Nov 2020 17:42:58 -0500 Subject: [PATCH] Automation rules: allow passing extra arguments to match and action Some times we may want to pass additional data around to the match and action functions. This is data that isn't available in the version object. Like the extra data needed to match external versions. We also need to return the response of the action, this is to allow the caller know what happened in the action. This was extracted from https://github.com/readthedocs/readthedocs.org/pull/7664 --- readthedocs/api/v2/utils.py | 4 +- readthedocs/builds/models.py | 93 ++++++++----------- readthedocs/builds/utils.py | 45 +++++++-- .../rtd_tests/tests/test_automation_rules.py | 20 ++-- 4 files changed, 87 insertions(+), 75 deletions(-) diff --git a/readthedocs/api/v2/utils.py b/readthedocs/api/v2/utils.py index ad69ee822ff..595a16fd733 100644 --- a/readthedocs/api/v2/utils.py +++ b/readthedocs/api/v2/utils.py @@ -14,7 +14,7 @@ STABLE_VERBOSE_NAME, TAG, ) -from readthedocs.builds.models import RegexAutomationRule, Version +from readthedocs.builds.models import Version, VersionAutomationRule log = logging.getLogger(__name__) @@ -210,7 +210,7 @@ def run_automation_rules(project, added_versions, deleted_active_versions): Currently the versions aren't sorted in any way, the same order is keeped. """ - class_ = RegexAutomationRule + class_ = VersionAutomationRule actions = [ (added_versions, class_.allowed_actions_on_create), (deleted_active_versions, class_.allowed_actions_on_delete), diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index 3e12d5b8f60..7da0eb4b946 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -6,7 +6,6 @@ import re from shutil import rmtree -import regex from django.conf import settings from django.core.files.storage import get_storage_class from django.db import models @@ -62,6 +61,7 @@ get_bitbucket_username_repo, get_github_username_repo, get_gitlab_username_repo, + match_regex, ) from readthedocs.builds.version_slug import VersionSlugField from readthedocs.config import LATEST_CONFIGURATION_VERSION @@ -979,8 +979,17 @@ class VersionAutomationRule(PolymorphicModel, TimeStampedModel): (DELETE_VERSION_ACTION, _('Delete version (on branch/tag deletion)')), ) - allowed_actions_on_create = {} - allowed_actions_on_delete = {} + allowed_actions_on_create = { + ACTIVATE_VERSION_ACTION: actions.activate_version, + HIDE_VERSION_ACTION: actions.hide_version, + MAKE_VERSION_PUBLIC_ACTION: actions.set_public_privacy_level, + MAKE_VERSION_PRIVATE_ACTION: actions.set_private_privacy_level, + SET_DEFAULT_VERSION_ACTION: actions.set_default_version, + } + + allowed_actions_on_delete = { + DELETE_VERSION_ACTION: actions.delete_version, + } project = models.ForeignKey( Project, @@ -1047,32 +1056,35 @@ def get_match_arg(self): ) return match_arg or self.match_arg - def run(self, version, *args, **kwargs): + def run(self, version, **kwargs): """ Run an action if `version` matches the rule. :type version: readthedocs.builds.models.Version - :returns: True if the action was performed + :param kwargs: All extra keywords will be passed to the match and action functions. + :returns: A tuple of (boolean, ANY), where the first element + indicates if the action was performed, and the second is the result + returned by the action. """ if version.type == self.version_type: - match, result = self.match(version, self.get_match_arg()) + match, result = self.match(version, self.get_match_arg(), **kwargs) if match: - self.apply_action(version, result) - return True - return False + action_result = self.apply_action(version, result, **kwargs) + return True, action_result + return False, None - def match(self, version, match_arg): + def match(self, version, match_arg, **kwargs): """ Returns True and the match result if the version matches the rule. :type version: readthedocs.builds.models.Version :param str match_arg: Additional argument to perform the match - :returns: A tuple of (boolean, match_resul). + :returns: A tuple of (boolean, match_result). The result will be passed to `apply_action`. """ return False, None - def apply_action(self, version, match_result): + def apply_action(self, version, match_result, **kwargs): """ Apply the action from allowed_actions_on_*. @@ -1087,7 +1099,12 @@ def apply_action(self, version, match_result): ) if action is None: raise NotImplementedError - action(version, match_result, self.action_arg) + return action( + version=version, + match_result=match_result, + action_arg=self.action_arg, + **kwargs, + ) def move(self, steps): """ @@ -1189,52 +1206,16 @@ def __str__(self): class RegexAutomationRule(VersionAutomationRule): - TIMEOUT = 1 # timeout in seconds - - allowed_actions_on_create = { - VersionAutomationRule.ACTIVATE_VERSION_ACTION: actions.activate_version, - VersionAutomationRule.HIDE_VERSION_ACTION: actions.hide_version, - VersionAutomationRule.MAKE_VERSION_PUBLIC_ACTION: actions.set_public_privacy_level, - VersionAutomationRule.MAKE_VERSION_PRIVATE_ACTION: actions.set_private_privacy_level, - VersionAutomationRule.SET_DEFAULT_VERSION_ACTION: actions.set_default_version, - } - - allowed_actions_on_delete = { - VersionAutomationRule.DELETE_VERSION_ACTION: actions.delete_version, - } - class Meta: proxy = True - def match(self, version, match_arg): - """ - Find a match using regex.search. - - .. note:: - - We use the regex module with the timeout - arg to avoid ReDoS. - - We could use a finite state machine type of regex too, - but there isn't a stable library at the time of writting this code. - """ - try: - match = regex.search( - match_arg, - version.verbose_name, - # Compatible with the re module - flags=regex.VERSION0, - timeout=self.TIMEOUT, - ) - return bool(match), match - except TimeoutError: - log.warning( - 'Timeout while parsing regex. pattern=%s, input=%s', - match_arg, version.verbose_name, - ) - except Exception as e: - log.info('Error parsing regex: %s', e) - return False, None + def match(self, version, match_arg, **kwargs): + version_name = version.verbose_name + if version.is_external: + version_data = kwargs['version_data'] + version_name = version_data.source_branch + result = match_regex(match_arg, version_name) + return bool(result), result def get_edit_url(self): return reverse( diff --git a/readthedocs/builds/utils.py b/readthedocs/builds/utils.py index 0702e460690..6b6a978acf5 100644 --- a/readthedocs/builds/utils.py +++ b/readthedocs/builds/utils.py @@ -1,7 +1,8 @@ """Utilities for the builds app.""" - +import logging from contextlib import contextmanager +import regex from celery.five import monotonic from django.core.cache import cache @@ -11,13 +12,15 @@ GITLAB_REGEXS, ) +log = logging.getLogger(__name__) + LOCK_EXPIRE = 60 * 180 # Lock expires in 3 hours def get_github_username_repo(url): if 'github' in url: - for regex in GITHUB_REGEXS: - match = regex.search(url) + for pattern in GITHUB_REGEXS: + match = pattern.search(url) if match: return match.groups() return (None, None) @@ -25,8 +28,8 @@ def get_github_username_repo(url): def get_bitbucket_username_repo(url=None): if 'bitbucket' in url: - for regex in BITBUCKET_REGEXS: - match = regex.search(url) + for pattern in BITBUCKET_REGEXS: + match = pattern.search(url) if match: return match.groups() return (None, None) @@ -34,8 +37,8 @@ def get_bitbucket_username_repo(url=None): def get_gitlab_username_repo(url=None): if 'gitlab' in url: - for regex in GITLAB_REGEXS: - match = regex.search(url) + for pattern in GITLAB_REGEXS: + match = pattern.search(url) if match: return match.groups() return (None, None) @@ -61,3 +64,31 @@ def memcache_lock(lock_id, oid): # to lessen the chance of releasing an expired lock # owned by someone else. cache.delete(lock_id) + + +def match_regex(pattern, text, timeout=1): + """ + Find a match using regex.search. + + .. note:: + We use the regex module with the timeout arg to avoid ReDoS. + We could use a finite state machine type of regex too, + but there isn't a stable library at the time of writting this code. + """ + try: + match = regex.search( + pattern, + text, + # Compatible with the re module + flags=regex.VERSION0, + timeout=timeout, + ) + return match + except TimeoutError: + log.exception( + 'Timeout while parsing regex. pattern=%s, input=%s', + pattern, text, + ) + except Exception as e: + log.exception('Error parsing regex: %s', e) + return None diff --git a/readthedocs/rtd_tests/tests/test_automation_rules.py b/readthedocs/rtd_tests/tests/test_automation_rules.py index 0e87977685f..9c915e607ab 100644 --- a/readthedocs/rtd_tests/tests/test_automation_rules.py +++ b/readthedocs/rtd_tests/tests/test_automation_rules.py @@ -93,7 +93,7 @@ def test_match( action=VersionAutomationRule.ACTIVATE_VERSION_ACTION, version_type=version_type, ) - assert rule.run(version) is result + assert rule.run(version) == (result, None) @pytest.mark.parametrize( 'version_name,result', @@ -127,7 +127,7 @@ def test_predefined_match_all_versions(self, trigger_build, version_name, result action=VersionAutomationRule.ACTIVATE_VERSION_ACTION, version_type=version_type, ) - assert rule.run(version) is result + assert rule.run(version) == (result, None) @pytest.mark.parametrize( 'version_name,result', @@ -163,7 +163,7 @@ def test_predefined_match_semver_versions(self, trigger_build, version_name, res action=VersionAutomationRule.ACTIVATE_VERSION_ACTION, version_type=version_type, ) - assert rule.run(version) is result + assert rule.run(version) == (result, None) def test_action_activation(self, trigger_build): version = get( @@ -181,7 +181,7 @@ def test_action_activation(self, trigger_build): action=VersionAutomationRule.ACTIVATE_VERSION_ACTION, version_type=TAG, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert version.active is True trigger_build.assert_called_once() @@ -204,7 +204,7 @@ def test_action_delete_version(self, trigger_build, version_type): action=VersionAutomationRule.DELETE_VERSION_ACTION, version_type=version_type, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert not self.project.versions.filter(slug=slug).exists() @pytest.mark.parametrize('version_type', [BRANCH, TAG]) @@ -229,7 +229,7 @@ def test_action_delete_version_on_default_version(self, trigger_build, version_t action=VersionAutomationRule.DELETE_VERSION_ACTION, version_type=version_type, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert self.project.versions.filter(slug=slug).exists() def test_action_set_default_version(self, trigger_build): @@ -249,7 +249,7 @@ def test_action_set_default_version(self, trigger_build): version_type=TAG, ) assert self.project.get_default_version() == LATEST - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert self.project.get_default_version() == version.slug def test_version_hide_action(self, trigger_build): @@ -269,7 +269,7 @@ def test_version_hide_action(self, trigger_build): action=VersionAutomationRule.HIDE_VERSION_ACTION, version_type=TAG, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert version.active is True assert version.hidden is True trigger_build.assert_called_once() @@ -292,7 +292,7 @@ def test_version_make_public_action(self, trigger_build): action=VersionAutomationRule.MAKE_VERSION_PUBLIC_ACTION, version_type=TAG, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert version.privacy_level == PUBLIC trigger_build.assert_not_called() @@ -314,7 +314,7 @@ def test_version_make_private_action(self, trigger_build): action=VersionAutomationRule.MAKE_VERSION_PRIVATE_ACTION, version_type=TAG, ) - assert rule.run(version) is True + assert rule.run(version) == (True, None) assert version.privacy_level == PRIVATE trigger_build.assert_not_called()