diff --git a/readthedocs/api/v2/views/integrations.py b/readthedocs/api/v2/views/integrations.py index 8a8f0eb4a15..7b93c55e20d 100644 --- a/readthedocs/api/v2/views/integrations.py +++ b/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 diff --git a/readthedocs/core/views/hooks.py b/readthedocs/core/views/hooks.py index f845711ca73..650f4d63e09 100644 --- a/readthedocs/core/views/hooks.py +++ b/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): diff --git a/readthedocs/projects/models.py b/readthedocs/projects/models.py index dd7e7204dab..87fc0bb30e8 100644 --- a/readthedocs/projects/models.py +++ b/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). diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index f9144d9ed9a..cd6ca0da9de 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/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,6 +1703,7 @@ def setUp(self): verbose_name="master", active=True, project=self.project, + type=BRANCH, ) self.version_tag = get( Version, @@ -1707,9 +1711,10 @@ def setUp(self): 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, diff --git a/readthedocs/rtd_tests/tests/test_integrations.py b/readthedocs/rtd_tests/tests/test_integrations.py index 76b39141a3d..c61ebd6381a 100644 --- a/readthedocs/rtd_tests/tests/test_integrations.py +++ b/readthedocs/rtd_tests/tests/test_integrations.py @@ -27,7 +27,7 @@ def test_exchange_json_request_body(self): integration_type=Integration.GITHUB_WEBHOOK, provider_data="", ) - payload = {"ref": "exchange_json"} + payload = {"ref": "refs/heads/exchange_json"} signature = get_signature(integration, payload) resp = client.post( "/api/v2/webhook/github/{}/".format(project.slug), @@ -40,7 +40,7 @@ def test_exchange_json_request_body(self): exchange = HttpExchange.objects.get(integrations=integration) self.assertEqual( exchange.request_body, - '{"ref": "exchange_json"}', + '{"ref": "refs/heads/exchange_json"}', ) self.assertEqual( exchange.request_headers, @@ -77,7 +77,7 @@ def test_exchange_form_request_body(self): provider_data="", secret=None, ) - payload = "payload=%7B%22ref%22%3A+%22exchange_form%22%7D" + payload = "payload=%7B%22ref%22:%20%22refs/heads/exchange_form%22%7D" signature = get_signature(integration, payload) resp = client.post( "/api/v2/webhook/github/{}/".format(project.slug), @@ -90,7 +90,7 @@ def test_exchange_form_request_body(self): exchange = HttpExchange.objects.get(integrations=integration) self.assertEqual( exchange.request_body, - '{"ref": "exchange_form"}', + '{"ref": "refs/heads/exchange_form"}', ) self.assertEqual( exchange.request_headers, diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index 4800b9e021a..75ac0e8c25d 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -463,3 +463,22 @@ def ref_exists(self, ref): "git", "show-ref", "--verify", "--quiet", "--", ref, record=False ) return exit_code == 0 + + +def parse_version_from_ref(ref: str): + """ + Parse the version name and type from a Git ref. + + The ref can be a branch or a tag. + + :param ref: The ref to parse (e.g. refs/heads/main, refs/tags/v1.0.0). + :returns: A tuple with the version name and type. + """ + heads_prefix = "refs/heads/" + tags_prefix = "refs/tags/" + if ref.startswith(heads_prefix): + return ref.removeprefix(heads_prefix), BRANCH + if ref.startswith(tags_prefix): + return ref.removeprefix(tags_prefix), TAG + + raise ValueError(f"Invalid ref: {ref}")