Git backend: make default_branch to point to VCS' default branch (#9424)

* Git backend: make `defaul_branch` to point to VCS' default branch

Currently, importing a project manually without setting the `default_branch`,
will make Git VCS to use `master` as the default branch. However, with the last
swap about `master` and `main` branches, repositories defaulting to `main`
branch fail when being imported on Read the Docs. Leaving the user in a blocked
state they can get out.

This commit allows `Version.identifier` to be NULL meaning that "it won't try to
guess what's the default branch of the VCS" and just leave the repository in the
immediate state after being cloned (which is already the default branch). To do
this, I'm removing the command `git checkout --force <branch>` from the steps
executed for this case.

Closes #9367

* Git backend: use `default_branch` to decide whether build latest

Use `default_branch` coming from the Webhook itself to decide if a build for
Latest Version with `identifier=None` has to be triggered or not.

Note that this feature only works for GitHub and GitLab since Bitbucket does not
send the `default_branch` of the repository.

* Update `Version.identifier` based on VCS's `default_branch`

The VCS's `default_branch` comes in the incoming webhooks that GitHub and GitLab
sends to us when the user realizes an action in the repository.

Note that in this commit BitBucket is not supported and will keep having the
issues this PR solves. To solve this problem, we would need to update the
`Version.identifier` from the builder immediately after clonning the default
branch (e.g. `git clone <URL>`). I put some commented code/ideas in place to
come back to this once we feel it's the right time.

* Migrations dependencies

* Lint

* Add note about how to expand use cases for this command

* Revert "Add note about how to expand use cases for this command"

Meh, I made a mistake. This commit should have gone to a different branch.

This reverts commit fc90d36.

* Apply suggestions from code review

Co-authored-by: Eric Holscher <[email protected]>

* Typo

* TODO comment to potentially remove `identifier` from STABLE

* Tests: adjust our tests to match the new changes

* Lint

* Webhooks: add support for `default_branch` in generic webhooks

People is able to pass `default_branch=main` so Read the Docs can decide whether
or not it has to trigger a build for `LATEST` in case its `identifier=None` (the
default branch of the repository)

* Tests: define `default_branch="master"`

All these tests are expecting `master` to be the default branch.
We have to force this now since it was automatically "guess" previously,
but now we are not guessing anymore.

* Tests: remove `allow_deprecated_webhooks` from this check

I don't understand why we were checking for this in this test,
but I think it's not necessary since it's an old feature.

* Docs: mention it's optional

* Test: this feature flag is required

I removed because executing tests with `--nomigrations` made it fail.

Co-authored-by: Eric Holscher <[email protected]>

Author: humitos

docs/user/integrations.rst

@@ -153,10 +153,17 @@ token
     The integration token found on the project's **Integrations** dashboard page
     (:guilabel:`Admin` > :guilabel:`Integrations`).
 
+default_branch
+    This is the default branch of the repository
+    (ie. the one checked out when cloning the repository without arguments)
+
+    *Optional*
+
 For example, the cURL command to build the ``dev`` branch, using the token
 ``1234``, would be::
 
-    curl -X POST -d "branches=dev" -d "token=1234" https://readthedocs.org/api/v2/webhook/example-project/1/
+    curl -X POST -d "branches=dev" -d "token=1234" -d "default_branch=main"
+    https://readthedocs.org/api/v2/webhook/example-project/1/
 
 A command like the one above could be called from a cron job or from a hook
 inside Git_, Subversion_, Mercurial_, or Bazaar_.

readthedocs/api/v2/views/integrations.py

@@ -18,6 +18,7 @@
 from readthedocs.builds.constants import (
     EXTERNAL_VERSION_STATE_CLOSED,
     EXTERNAL_VERSION_STATE_OPEN,
+    LATEST,
 )
 from readthedocs.core.signals import webhook_bitbucket, webhook_github, webhook_gitlab
 from readthedocs.core.views.hooks import (
@@ -280,6 +281,30 @@ def get_closed_external_version_response(self, project):
             "versions": [version_closed] if version_closed else [],
         }
 
+    def update_default_branch(self, default_branch):
+        """
+        Update the `Version.identifer` for `latest` with the VCS's `default_branch`.
+
+        The VCS's `default_branch` is the branch cloned when there is no specific branch specified
+        (e.g. `git clone <URL>`).
+
+        Some VCS providers (GitHub and GitLab) send the `default_branch` via incoming webhooks.
+        We use that data to update our database and keep it in sync.
+
+        This solves the problem about "changing the default branch in GitHub"
+        and also importing repositories with a different `default_branch` than `main` manually:
+        https://github.com/readthedocs/readthedocs.org/issues/9367
+
+        In case the user already selected a `default-branch` from the "Advanced settings",
+        it does not override it.
+        """
+        if not self.project.default_branch:
+            (
+                self.project.versions.filter(slug=LATEST).update(
+                    identifier=default_branch
+                )
+            )
+
 
 class GitHubWebhookView(WebhookMixin, APIView):
 
@@ -417,6 +442,12 @@ def handle_webhook(self):
             event=event,
         )
 
+        # Always update `latest` branch to point to the default branch in the repository
+        # even if the event is not gonna be handled. This helps us to keep our db in sync.
+        default_branch = self.data.get("repository", {}).get("default_branch", None)
+        if default_branch:
+            self.update_default_branch(default_branch)
+
         if event == GITHUB_PING:
             return {"detail": "Webhook configured correctly"}
 
@@ -467,14 +498,15 @@ def handle_webhook(self):
         # Trigger a build for all branches in the push
         if event == GITHUB_PUSH:
             try:
-                branches = [self._normalize_ref(self.data['ref'])]
-                return self.get_response_push(self.project, branches)
+                branch = self._normalize_ref(self.data["ref"])
+                return self.get_response_push(self.project, [branch])
             except KeyError:
                 raise ParseError('Parameter "ref" is required')
 
         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)
 
@@ -568,6 +600,13 @@ def handle_webhook(self):
             data=self.request.data,
             event=event,
         )
+
+        # Always update `latest` branch to point to the default branch in the repository
+        # even if the event is not gonna be handled. This helps us to keep our db in sync.
+        default_branch = self.data.get("project", {}).get("default_branch", None)
+        if default_branch:
+            self.update_default_branch(default_branch)
+
         # Handle push events and trigger builds
         if event in (GITLAB_PUSH, GITLAB_TAG_PUSH):
             data = self.request.data
@@ -583,8 +622,8 @@ def handle_webhook(self):
                 return self.sync_versions_response(self.project)
             # Normal push to master
             try:
-                branches = [self._normalize_ref(data['ref'])]
-                return self.get_response_push(self.project, branches)
+                branch = self._normalize_ref(data["ref"])
+                return self.get_response_push(self.project, [branch])
             except KeyError:
                 raise ParseError('Parameter "ref" is required')
 
@@ -660,6 +699,11 @@ def handle_webhook(self):
             data=self.request.data,
             event=event,
         )
+
+        # NOTE: we can't call `self.update_default_branch` here because
+        # BitBucket does not tell us what is the `default_branch` for a
+        # repository in these incoming webhooks.
+
         if event == BITBUCKET_PUSH:
             try:
                 data = self.request.data
@@ -676,8 +720,11 @@ def handle_webhook(self):
                 # we don't trigger the sync versions, because that
                 # will be triggered with the normal push.
                 if branches:
-                    return self.get_response_push(self.project, branches)
-                log.debug('Triggered sync_versions.')
+                    return self.get_response_push(
+                        self.project,
+                        branches,
+                    )
+                log.debug("Triggered sync_versions.")
                 return self.sync_versions_response(self.project)
             except KeyError:
                 raise ParseError('Invalid request')
@@ -710,7 +757,8 @@ class APIWebhookView(WebhookMixin, APIView):
     Expects the following JSON::
 
         {
-            "branches": ["master"]
+            "branches": ["master"],
+            "default_branch": "main"
         }
     """
 
@@ -753,8 +801,13 @@ def handle_webhook(self):
                 'branches',
                 [self.project.get_default_branch()],
             )
+            default_branch = self.request.data.get("default_branch", None)
             if isinstance(branches, str):
                 branches = [branches]
+
+            if default_branch and isinstance(default_branch, str):
+                self.update_default_branch(default_branch)
+
             return self.get_response_push(self.project, branches)
         except TypeError:
             raise ParseError('Invalid request')

readthedocs/builds/managers.py

@@ -1,7 +1,6 @@
 """Build and Version class model Managers."""
 
 import structlog
-
 from django.core.exceptions import ObjectDoesNotExist
 from django.db import models
 from polymorphic.managers import PolymorphicManager
@@ -50,6 +49,12 @@ def create_stable(self, **kwargs):
             'verbose_name': STABLE_VERBOSE_NAME,
             'machine': True,
             'active': True,
+            # TODO: double-check if we still require the `identifier: STABLE` field.
+            # At the time of creation, we don't really know what's the branch/tag identifier
+            # for the STABLE version. It makes sense to be `None`, probably.
+            #
+            # Note that we removed the `identifier: LATEST` from `create_latest` as a way to
+            # use the default branch.
             'identifier': STABLE,
             'type': TAG,
         }
@@ -62,7 +67,6 @@ def create_latest(self, **kwargs):
             'verbose_name': LATEST_VERBOSE_NAME,
             'machine': True,
             'active': True,
-            'identifier': LATEST,
             'type': BRANCH,
         }
         defaults.update(kwargs)

readthedocs/builds/migrations/0046_identifier_null.py

@@ -0,0 +1,20 @@
+# Generated by Django 3.2.13 on 2022-07-11 08:53
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("builds", "0045_alter_build_status"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="version",
+            name="identifier",
+            field=models.CharField(
+                blank=True, max_length=255, null=True, verbose_name="Identifier"
+            ),
+        ),
+    ]

readthedocs/builds/models.py

@@ -119,11 +119,15 @@ class Version(TimeStampedModel):
     )
     # used by the vcs backend
 
-    #: The identifier is the ID for the revision this is version is for. This
-    #: might be the revision number (e.g. in SVN), or the commit hash (e.g. in
-    #: Git). If the this version is pointing to a branch, then ``identifier``
-    #: will contain the branch name.
-    identifier = models.CharField(_('Identifier'), max_length=255)
+    #: The identifier is the ID for the revision this is version is for.
+    #: This might be the revision number (e.g. in SVN),
+    #: or the commit hash (e.g. in Git).
+    #: If the this version is pointing to a branch,
+    #: then ``identifier`` will contain the branch name.
+    #: `None`/`null` means it will use the VCS default branch.
+    identifier = models.CharField(
+        _("Identifier"), max_length=255, null=True, blank=True
+    )
 
     #: This is the actual name that we got for the commit stored in
     #: ``identifier``. This might be the tag or branch name like ``"v1.0.4"``.
@@ -363,6 +367,11 @@ def delete(self, *args, **kwargs):  # pylint: disable=arguments-differ
     @property
     def identifier_friendly(self):
         """Return display friendly identifier."""
+        if not self.identifier:
+            # Text shown to user when we don't know yet what's the ``identifier`` for this version.
+            # This usually happens when we haven't pulled the ``default_branch`` for LATEST.
+            return "Unknown yet"
+
         if re.match(r'^[0-9a-f]{40}$', self.identifier, re.I):
             return self.identifier[:8]
         return self.identifier

readthedocs/core/views/hooks.py

@@ -52,7 +52,6 @@ def build_branches(project, branch_list):
     not_building = set()
     for branch in branch_list:
         versions = project.versions_from_branch_name(branch)
-
         for version in versions:
             log.debug(
                 'Processing.',

readthedocs/projects/migrations/0095_default_branch_helptext.py

@@ -0,0 +1,37 @@
+# Generated by Django 3.2.13 on 2022-07-11 09:06
+
+from django.db import migrations, models
+
+
+class Migration(migrations.Migration):
+
+    dependencies = [
+        ("projects", "0094_auto_20221221_1045"),
+    ]
+
+    operations = [
+        migrations.AlterField(
+            model_name="historicalproject",
+            name="default_branch",
+            field=models.CharField(
+                blank=True,
+                default=None,
+                help_text='What branch "latest" points to. Leave empty to use the default value for your VCS.',
+                max_length=255,
+                null=True,
+                verbose_name="Default branch",
+            ),
+        ),
+        migrations.AlterField(
+            model_name="project",
+            name="default_branch",
+            field=models.CharField(
+                blank=True,
+                default=None,
+                help_text='What branch "latest" points to. Leave empty to use the default value for your VCS.',
+                max_length=255,
+                null=True,
+                verbose_name="Default branch",
+            ),
+        ),
+    ]

readthedocs/projects/models.py

@@ -175,8 +175,8 @@ class Project(models.Model):
         default=LATEST,
         help_text=_('The version of your project that / redirects to'),
     )
-    # In default_branch, None means the backend should choose the
-    # appropriate branch. Eg 'master' for git
+    # In default_branch, ``None`` means the backend will use the default branch
+    # cloned for each backend.
     default_branch = models.CharField(
         _('Default branch'),
         max_length=255,
@@ -185,8 +185,7 @@ class Project(models.Model):
         blank=True,
         help_text=_(
             'What branch "latest" points to. Leave empty '
-            'to use the default value for your VCS (eg. '
-            '<code>trunk</code> or <code>master</code>).',
+            "to use the default value for your VCS.",
         ),
     )
     requirements_file = models.CharField(
@@ -480,21 +479,21 @@ def save(self, *args, **kwargs):  # pylint: disable=arguments-differ
             if not self.slug:
                 raise Exception(_('Model must have slug'))
         super().save(*args, **kwargs)
-        try:
-            latest = self.versions.filter(slug=LATEST).first()
-            default_branch = self.get_default_branch()
-            if latest and latest.identifier != default_branch:
-                latest.identifier = default_branch
-                latest.save()
-        except Exception:
-            log.exception('Failed to update latest identifier')
 
         try:
-            branch = self.get_default_branch()
             if not self.versions.filter(slug=LATEST).exists():
-                self.versions.create_latest(identifier=branch)
+                self.versions.create_latest()
         except Exception:
-            log.exception('Error creating default branches')
+            log.exception("Error creating default branches")
+
+        # Update `Version.identifier` for `latest` with the default branch the user has selected,
+        # even if it's `None` (meaning to match the `default_branch` of the repository)
+        # NOTE: this code is required to be *after* ``create_latest()``.
+        # It has to be updated after creating LATEST originally.
+        log.debug(
+            "Updating default branch.", slug=LATEST, identifier=self.default_branch
+        )
+        self.versions.filter(slug=LATEST).update(identifier=self.default_branch)
 
     def delete(self, *args, **kwargs):  # pylint: disable=arguments-differ
         from readthedocs.projects.tasks.utils import clean_project_resources