Webhooks: refactor branch/tag building (#12014)

- Webhooks can trigger builds to tags, not just branches
- Since webhooks already know the type of the version, we use it to
filter by results.
- verbose_name is the value we use to match branches or tags.

Extracted from
#11942

Author: stsewd

readthedocs/api/v2/views/integrations.py

@@ -3,7 +3,6 @@
 import hashlib
 import hmac
 import json
-import re
 from functools import namedtuple
 from textwrap import dedent
 
@@ -19,18 +18,22 @@
 from rest_framework.status import HTTP_400_BAD_REQUEST
 from rest_framework.views import APIView
 
+from readthedocs.builds.constants import BRANCH
 from readthedocs.builds.constants import LATEST
+from readthedocs.builds.constants import TAG
 from readthedocs.core.signals import webhook_bitbucket
 from readthedocs.core.signals import webhook_github
 from readthedocs.core.signals import webhook_gitlab
-from readthedocs.core.views.hooks import build_branches
+from readthedocs.core.views.hooks import VersionInfo
 from readthedocs.core.views.hooks import build_external_version
+from readthedocs.core.views.hooks import build_versions_from_names
 from readthedocs.core.views.hooks import close_external_version
 from readthedocs.core.views.hooks import get_or_create_external_version
 from readthedocs.core.views.hooks import trigger_sync_versions
 from readthedocs.integrations.models import HttpExchange
 from readthedocs.integrations.models import Integration
 from readthedocs.projects.models import Project
+from readthedocs.vcs_support.backends.git import parse_version_from_ref
 
 
 log = structlog.get_logger(__name__)
@@ -205,7 +208,7 @@ def get_integration(self):
         )
         return self.integration
 
-    def get_response_push(self, project, branches):
+    def get_response_push(self, project, versions_info: list[VersionInfo]):
         """
         Build branches on push events and return API response.
 
@@ -219,14 +222,12 @@ def get_response_push(self, project, branches):
 
         :param project: Project instance
         :type project: Project
-        :param branches: List of branch/tag names to build
-        :type branches: list(str)
         """
-        to_build, not_building = build_branches(project, branches)
+        to_build, not_building = build_versions_from_names(project, versions_info)
         if not_building:
             log.info(
-                "Skipping project branches.",
-                branches=branches,
+                "Skipping project versions.",
+                versions=not_building,
             )
         triggered = bool(to_build)
         return {
@@ -520,18 +521,15 @@ def handle_webhook(self):
         # Trigger a build for all branches in the push
         if event == GITHUB_PUSH:
             try:
-                branch = self._normalize_ref(self.data["ref"])
-                return self.get_response_push(self.project, [branch])
+                version_name, version_type = parse_version_from_ref(self.data["ref"])
+                return self.get_response_push(
+                    self.project, [VersionInfo(name=version_name, type=version_type)]
+                )
             except KeyError as exc:
                 raise ParseError('Parameter "ref" is required') from exc
 
         return None
 
-    def _normalize_ref(self, ref):
-        """Remove `ref/(heads|tags)/` from the reference to match a Version on the db."""
-        pattern = re.compile(r"^refs/(heads|tags)/")
-        return pattern.sub("", ref)
-
 
 class GitLabWebhookView(WebhookMixin, APIView):
     """
@@ -640,8 +638,10 @@ def handle_webhook(self):
                 return self.sync_versions_response(self.project)
             # Normal push to master
             try:
-                branch = self._normalize_ref(data["ref"])
-                return self.get_response_push(self.project, [branch])
+                version_name, version_type = parse_version_from_ref(self.data["ref"])
+                return self.get_response_push(
+                    self.project, [VersionInfo(name=version_name, type=version_type)]
+                )
             except KeyError as exc:
                 raise ParseError('Parameter "ref" is required') from exc
 
@@ -659,10 +659,6 @@ def handle_webhook(self):
                 return self.get_closed_external_version_response(self.project)
         return None
 
-    def _normalize_ref(self, ref):
-        pattern = re.compile(r"^refs/(heads|tags)/")
-        return pattern.sub("", ref)
-
 
 class BitbucketWebhookView(WebhookMixin, APIView):
     """
@@ -722,21 +718,22 @@ def handle_webhook(self):
             try:
                 data = self.request.data
                 changes = data["push"]["changes"]
-                branches = []
+                versions_info = []
                 for change in changes:
                     old = change["old"]
                     new = change["new"]
                     # Normal push to master
                     if old is not None and new is not None:
-                        branches.append(new["name"])
+                        version_type = BRANCH if new["type"] == "branch" else TAG
+                        versions_info.append(VersionInfo(name=new["name"], type=version_type))
                 # BitBuck returns an array of changes rather than
                 # one webhook per change. If we have at least one normal push
                 # we don't trigger the sync versions, because that
                 # will be triggered with the normal push.
-                if branches:
+                if versions_info:
                     return self.get_response_push(
                         self.project,
-                        branches,
+                        versions_info,
                     )
                 log.debug("Triggered sync_versions.")
                 return self.sync_versions_response(self.project)
@@ -833,7 +830,9 @@ def handle_webhook(self):
             if default_branch and isinstance(default_branch, str):
                 self.update_default_branch(default_branch)
 
-            return self.get_response_push(self.project, branches)
+            # branches can be a branch or a tag, so we set it to None, so both types are considered.
+            versions_info = [VersionInfo(name=branch, type=None) for branch in branches]
+            return self.get_response_push(self.project, versions_info)
         except TypeError as exc:
             raise ParseError("Invalid request") from exc
 

readthedocs/core/views/hooks.py

@@ -1,5 +1,8 @@
 """Views pertaining to builds."""
 
+from dataclasses import dataclass
+from typing import Literal
+
 import structlog
 
 from readthedocs.api.v2.models import BuildAPIKey
@@ -12,12 +15,24 @@
 from readthedocs.projects.tasks.builds import sync_repository_task
 
 
+@dataclass
+class VersionInfo:
+    """
+    Version information.
+
+    If type is None, it means that the version can be either a branch or a tag.
+    """
+
+    name: str
+    type: Literal["branch", "tag", None]
+
+
 log = structlog.get_logger(__name__)
 
 
-def _build_version(project, slug, already_built=()):
+def _build_version(project, version):
     """
-    Where we actually trigger builds for a project and slug.
+    Where we actually trigger builds for a project and version.
 
     All webhook logic should route here to call ``trigger_build``.
     """
@@ -27,44 +42,43 @@ def _build_version(project, slug, already_built=()):
     # Previously we were building the latest version (inactive or active)
     # when building the default version,
     # some users may have relied on this to update the version list #4450
-    version = project.versions.filter(active=True, slug=slug).first()
-    if version and slug not in already_built:
+    if version.active:
         log.info(
-            "Building.",
+            "Triggering build.",
             project_slug=project.slug,
             version_slug=version.slug,
         )
         trigger_build(project=project, version=version)
-        return slug
+        return True
 
-    log.info("Not building.", version_slug=slug)
-    return None
+    log.info("Not building.", version_slug=version.slug)
+    return False
 
 
-def build_branches(project, branch_list):
+def build_versions_from_names(project, versions_info: list[VersionInfo]):
     """
-    Build the branches for a specific project.
+    Build the branches or tags from the project.
 
-    Returns:
-        to_build - a list of branches that were built
-        not_building - a list of branches that we won't build
+    :param project: Project instance
+    :returns: A tuple with the versions that were built and the versions that were not built.
     """
     to_build = set()
     not_building = set()
-    for branch in branch_list:
-        versions = project.versions_from_branch_name(branch)
-        for version in versions:
+    for version_info in versions_info:
+        for version in project.versions_from_name(version_info.name, version_info.type):
             log.debug(
                 "Processing.",
                 project_slug=project.slug,
                 version_slug=version.slug,
             )
-            ret = _build_version(project, version.slug, already_built=to_build)
-            if ret:
-                to_build.add(ret)
+            if version.slug in to_build:
+                continue
+            version_built = _build_version(project, version)
+            if version_built:
+                to_build.add(version.slug)
             else:
                 not_building.add(version.slug)
-    return (to_build, not_building)
+    return to_build, not_building
 
 
 def trigger_sync_versions(project):

readthedocs/projects/models.py

@@ -14,6 +14,7 @@
 from django.core.validators import MaxValueValidator
 from django.core.validators import MinValueValidator
 from django.db import models
+from django.db.models import Q
 from django.urls import reverse
 from django.utils import timezone
 from django.utils.crypto import get_random_string
@@ -1230,14 +1231,32 @@ def update_stable_version(self):
                 )
                 return new_stable
 
-    def versions_from_branch_name(self, branch):
-        return (
-            self.versions.filter(identifier=branch)
-            | self.versions.filter(identifier="remotes/origin/%s" % branch)
-            | self.versions.filter(identifier="origin/%s" % branch)
-            | self.versions.filter(verbose_name=branch)
+    def versions_from_name(self, name, type=None):
+        """
+        Get all versions attached to the branch or tag name.
+
+        Normally, only one version should be returned, but since LATEST and STABLE
+        are aliases for the branch/tag, they may be returned as well.
+
+        If type is None, both, tags and branches will be taken into consideration.
+        """
+        queryset = self.versions(manager=INTERNAL)
+        queryset = queryset.filter(
+            # Normal branches
+            Q(verbose_name=name, machine=False)
+            # Latest and stable have the name of the branch/tag in the identifier.
+            # NOTE: if stable is a branch, identifier will be the commit hash,
+            # so we don't have a way to match it by name.
+            # We should do another lookup to get the original stable version,
+            # or change our logic to store the tags name in the identifier of stable.
+            | Q(identifier=name, machine=True)
         )
 
+        if type:
+            queryset = queryset.filter(type=type)
+
+        return queryset.distinct()
+
     def get_default_version(self):
         """
         Get the default version (slug).