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 #7664

Author: stsewd

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),

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(

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,31 +12,32 @@
     GITLAB_REGEXS,
 )
 
-LOCK_EXPIRE = 60 * 180  # Lock expires in 3 hours
+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)
 
 
 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)
 
 
 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 +63,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

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()