Webhooks: refactor branch/tag building

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
 
@@ -17,17 +16,18 @@
 from rest_framework.status import HTTP_400_BAD_REQUEST
 from rest_framework.views import APIView
 
-from readthedocs.builds.constants import LATEST
+from readthedocs.builds.constants import BRANCH, LATEST, TAG
 from readthedocs.core.signals import webhook_bitbucket, webhook_github, webhook_gitlab
 from readthedocs.core.views.hooks import (
-    build_branches,
     build_external_version,
+    build_versions_from_names,
     close_external_version,
     get_or_create_external_version,
     trigger_sync_versions,
 )
 from readthedocs.integrations.models import HttpExchange, Integration
 from readthedocs.projects.models import Project
+from readthedocs.vcs_support.backends.git import parse_version_from_ref
 
 log = structlog.get_logger(__name__)
 
@@ -202,7 +202,7 @@ def get_integration(self):
         )
         return self.integration
 
-    def get_response_push(self, project, branches):
+    def get_response_push(self, project, version_names: list[tuple[str, str | None]]):
         """
         Build branches on push events and return API response.
 
@@ -219,11 +219,11 @@ def get_response_push(self, project, branches):
         :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, version_names)
         if not_building:
             log.info(
-                "Skipping project branches.",
-                branches=branches,
+                "Skipping project versions.",
+                versions=not_building,
             )
         triggered = bool(to_build)
         return {
@@ -520,18 +520,13 @@ 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 = parse_version_from_ref(self.data["ref"])
+                return self.get_response_push(self.project, [version_name])
             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):
 
@@ -641,8 +636,8 @@ 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 = parse_version_from_ref(self.data["ref"])
+                return self.get_response_push(self.project, [version_name])
             except KeyError as exc:
                 raise ParseError('Parameter "ref" is required') from exc
 
@@ -660,10 +655,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):
 
@@ -724,21 +715,22 @@ def handle_webhook(self):
             try:
                 data = self.request.data
                 changes = data["push"]["changes"]
-                branches = []
+                version_names = []
                 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
+                        version_names.append((new["name"], 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 version_names:
                     return self.get_response_push(
                         self.project,
-                        branches,
+                        version_names,
                     )
                 log.debug("Triggered sync_versions.")
                 return self.sync_versions_response(self.project)
@@ -837,7 +829,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.
+            version_names = [(branch, None) for branch in branches]
+            return self.get_response_push(self.project, version_names)
         except TypeError as exc:
             raise ParseError("Invalid request") from exc
 

readthedocs/core/views/hooks.py

@@ -15,7 +15,7 @@
 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.
 
@@ -27,44 +27,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.",
             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, version_names: list[tuple[str, str | None]]):
     """
-    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
+    :param version_names: A list of tuples with the version name and type.
+    :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_name, version_type in version_names:
+        for version in project.versions_from_name(version_name, version_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
+            if _build_version(project, version):
+                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

@@ -12,6 +12,7 @@
 from django.contrib.contenttypes.fields import GenericRelation
 from django.core.validators import MaxValueValidator, 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
@@ -1256,14 +1257,30 @@ 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.
+        """
+        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).

readthedocs/rtd_tests/tests/test_api.py

@@ -41,12 +41,14 @@
     WebhookMixin,
 )
 from readthedocs.builds.constants import (
+    BRANCH,
     BUILD_STATE_CLONING,
     BUILD_STATE_FINISHED,
     BUILD_STATE_TRIGGERED,
     EXTERNAL,
     EXTERNAL_VERSION_STATE_CLOSED,
     LATEST,
+    TAG,
 )
 from readthedocs.builds.models import APIVersion, Build, BuildCommandResult, Version
 from readthedocs.doc_builder.exceptions import BuildCancelled, BuildMaxConcurrencyError
@@ -69,6 +71,7 @@
 )
 from readthedocs.subscriptions.constants import TYPE_CONCURRENT_BUILDS
 from readthedocs.subscriptions.products import RTDProductFeature
+from readthedocs.vcs_support.backends.git import parse_version_from_ref
 
 
 def get_signature(integration, payload):
@@ -1700,16 +1703,18 @@ def setUp(self):
             verbose_name="master",
             active=True,
             project=self.project,
+            type=BRANCH,
         )
         self.version_tag = get(
             Version,
             slug="v1.0",
             verbose_name="v1.0",
             active=True,
             project=self.project,
+            type=TAG,
         )
         self.github_payload = {
-            "ref": "master",
+            "ref": "refs/heads/master",
         }
         self.commit = "ec26de721c3235aad62de7213c562f8c821"
         self.github_pull_request_payload = {
@@ -1737,7 +1742,7 @@ def setUp(self):
         }
         self.gitlab_payload = {
             "object_kind": GITLAB_PUSH,
-            "ref": "master",
+            "ref": "refs/heads/master",
             "before": "95790bf891e76fee5e1747ab589903a6a1f80f22",
             "after": "95790bf891e76fee5e1747ab589903a6a1f80f23",
         }
@@ -1846,7 +1851,7 @@ def test_github_webhook_for_branches(self, trigger_build):
         """GitHub webhook API."""
         client = APIClient()
 
-        data = {"ref": "master"}
+        data = {"ref": "refs/heads/master"}
         client.post(
             "/api/v2/webhook/github/{}/".format(self.project.slug),
             data,
@@ -1859,7 +1864,7 @@ def test_github_webhook_for_branches(self, trigger_build):
             [mock.call(version=self.version, project=self.project)],
         )
 
-        data = {"ref": "non-existent"}
+        data = {"ref": "refs/heads/non-existent"}
         client.post(
             "/api/v2/webhook/github/{}/".format(self.project.slug),
             data,
@@ -1888,7 +1893,7 @@ def test_github_webhook_for_branches(self, trigger_build):
     def test_github_webhook_for_tags(self, trigger_build):
         """GitHub webhook API."""
         client = APIClient()
-        data = {"ref": "v1.0"}
+        data = {"ref": "refs/tags/v1.0"}
 
         client.post(
             "/api/v2/webhook/github/{}/".format(self.project.slug),
@@ -2243,14 +2248,18 @@ def test_github_delete_event(self, sync_repository_task, trigger_build):
         )
 
     def test_github_parse_ref(self, trigger_build):
-        wh = GitHubWebhookView()
-
-        self.assertEqual(wh._normalize_ref("refs/heads/master"), "master")
-        self.assertEqual(wh._normalize_ref("refs/heads/v0.1"), "v0.1")
-        self.assertEqual(wh._normalize_ref("refs/tags/v0.1"), "v0.1")
-        self.assertEqual(wh._normalize_ref("refs/tags/tag"), "tag")
-        self.assertEqual(wh._normalize_ref("refs/heads/stable/2018"), "stable/2018")
-        self.assertEqual(wh._normalize_ref("refs/tags/tag/v0.1"), "tag/v0.1")
+        self.assertEqual(
+            parse_version_from_ref("refs/heads/master"), ("master", BRANCH)
+        )
+        self.assertEqual(parse_version_from_ref("refs/heads/v0.1"), ("v0.1", BRANCH))
+        self.assertEqual(parse_version_from_ref("refs/tags/v0.1"), ("v0.1", TAG))
+        self.assertEqual(parse_version_from_ref("refs/tags/tag"), ("tag", TAG))
+        self.assertEqual(
+            parse_version_from_ref("refs/heads/stable/2018"), ("stable/2018", BRANCH)
+        )
+        self.assertEqual(
+            parse_version_from_ref("refs/tags/tag/v0.1"), ("tag/v0.1", TAG)
+        )
 
     def test_github_invalid_webhook(self, trigger_build):
         """GitHub webhook unhandled event."""
@@ -2289,7 +2298,7 @@ def test_github_invalid_payload(self, trigger_build):
 
     def test_github_valid_payload(self, trigger_build):
         client = APIClient()
-        payload = '{"ref":"master"}'
+        payload = '{"ref":"refs/heads/master"}'
         signature = get_signature(
             self.github_integration,
             payload,
@@ -2431,7 +2440,7 @@ def test_gitlab_webhook_for_tags(self, trigger_build):
         client = APIClient()
         self.gitlab_payload.update(
             object_kind=GITLAB_TAG_PUSH,
-            ref="v1.0",
+            ref="refs/tags/v1.0",
         )
         headers = {
             GITLAB_TOKEN_HEADER: self.gitlab_integration.secret,