Automation rules: allow passing extra arguments to match and action

readthedocs/api/v2/utils.py

@@ -1,8 +1,8 @@
 """Utility functions that are used by both views and celery tasks."""
 
 import itertools
-import structlog
 
+import structlog
 from rest_framework.pagination import PageNumberPagination
 
 from readthedocs.builds.constants import (
@@ -15,7 +15,7 @@
     STABLE_VERBOSE_NAME,
     TAG,
 )
-from readthedocs.builds.models import RegexAutomationRule, Version
+from readthedocs.builds.models import Version, VersionAutomationRule
 
 log = structlog.get_logger(__name__)
 
@@ -246,7 +246,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

@@ -5,7 +5,6 @@
 import re
 from functools import partial
 
-import regex
 import structlog
 from django.conf import settings
 from django.db import models
@@ -14,10 +13,7 @@
 from django.utils import timezone
 from django.utils.translation import gettext
 from django.utils.translation import gettext_lazy as _
-from django_extensions.db.fields import (
-    CreationDateTimeField,
-    ModificationDateTimeField,
-)
+from django_extensions.db.fields import CreationDateTimeField, ModificationDateTimeField
 from django_extensions.db.models import TimeStampedModel
 from polymorphic.models import PolymorphicModel
 
@@ -61,6 +57,7 @@
     get_github_username_repo,
     get_gitlab_username_repo,
     get_vcs_url,
+    match_regex,
 )
 from readthedocs.builds.version_slug import VersionSlugField
 from readthedocs.config import LATEST_CONFIGURATION_VERSION
@@ -1064,8 +1061,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,
@@ -1137,33 +1143,36 @@ 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:
             return False
 
-        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)
+            action_result = self.apply_action(version, result, **kwargs)
             AutomationRuleMatch.objects.register_match(
                 rule=self,
                 version=version,
             )
-            return True
-        return False
+            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_*.
 
@@ -1178,7 +1187,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):
         """
@@ -1280,53 +1294,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 writing 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=match_arg,
-                version_slug=version.slug,
-            )
-        except Exception as e:
-            log.exception('Error parsing regex.', exc_info=True)
-        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,8 +1,10 @@
 """Utilities for the builds app."""
-from time import monotonic
 
 from contextlib import contextmanager
+from time import monotonic
 
+import regex
+import structlog
 from django.core.cache import cache
 
 from readthedocs.builds.constants import EXTERNAL
@@ -14,31 +16,33 @@
     GITLAB_REGEXS,
 )
 
+log = structlog.get_logger(__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)
@@ -100,3 +104,33 @@ def memcache_lock(lock_id, oid):
             # owned by someone else
             # also don't release the lock if we didn't acquire it
             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 writing 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=pattern,
+            input=text,
+        )
+    except Exception:
+        log.exception("Error parsing regex.", exc_info=True)
+    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)
         assert rule.matches.all().count() == (1 if result else 0)
 
     @pytest.mark.parametrize(
@@ -128,7 +128,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',
@@ -164,7 +164,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(
@@ -182,7 +182,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()
 
@@ -205,7 +205,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])
@@ -230,7 +230,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):
@@ -250,7 +250,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):
@@ -270,7 +270,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()
@@ -293,7 +293,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()
 
@@ -315,7 +315,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()
 
@@ -338,7 +338,7 @@ def test_matches_history(self, trigger_build):
             version_type=TAG,
         )
 
-        assert rule.run(version) is True
+        assert rule.run(version) == (True, None)
         assert rule.matches.all().count() == 1
 
         match = rule.matches.first()
@@ -350,7 +350,7 @@ def test_matches_history(self, trigger_build):
         for i in range(1, 31):
             version.verbose_name = f'test {i}'
             version.save()
-            assert rule.run(version) is True
+            assert rule.run(version) == (True, None)
 
         assert rule.matches.all().count() == 15