From 30ad791bbaeed405f19c5e41944c3a1f24b38ae6 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 15 Mar 2022 16:22:29 -0500 Subject: [PATCH 1/3] Sync versions: filter by type on update If two versions share the same verbose name (a branch named 2.0 and a tag named 2.0), then we will be updating both, instead of just one and changing its type from branch to tag. Later when running this same code, it will see the branch as new, since isn't included in our queryset that filters by type=branch. This wasn't happening before because this check was true https://github.com/readthedocs/readthedocs.org/blob/2e3f208c4906de649a9b5b9e33afee6cc20b86bc/readthedocs/api/v2/utils.py#L74-L74 Since a branch would never change its identifier (origin/name), but once lsremote was enabled, the identifier didn't included the 'origin/' part. So, when doing a full build the branches would be changed to (origin/name) and when doing a syn they would be changed to (name), and so on. Ref https://github.com/readthedocs/readthedocs.org/issues/8992#issuecomment-1062466017 --- readthedocs/api/v2/utils.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/api/v2/utils.py b/readthedocs/api/v2/utils.py index 055e131954d..26de2c8af9e 100644 --- a/readthedocs/api/v2/utils.py +++ b/readthedocs/api/v2/utils.py @@ -79,9 +79,9 @@ def sync_versions_to_db(project, versions, type): # pylint: disable=redefined-b Version.objects.filter( project=project, verbose_name=version_name, + type=type, ).update( identifier=version_id, - type=type, machine=False, ) From 12b5889d040849cd7453ff64ed8da6bc5a37a0e8 Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Wed, 16 Mar 2022 13:29:57 -0500 Subject: [PATCH 2/3] Add test --- readthedocs/api/v2/utils.py | 4 +- .../rtd_tests/tests/test_sync_versions.py | 53 ++++++++++++++++++- 2 files changed, 54 insertions(+), 3 deletions(-) diff --git a/readthedocs/api/v2/utils.py b/readthedocs/api/v2/utils.py index 26de2c8af9e..d7b3cd209c3 100644 --- a/readthedocs/api/v2/utils.py +++ b/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 ( @@ -79,6 +79,8 @@ def sync_versions_to_db(project, versions, type): # pylint: disable=redefined-b Version.objects.filter( project=project, verbose_name=version_name, + # Always filter by type, a tag and a branch + # can share the same verbose_name. type=type, ).update( identifier=version_id, diff --git a/readthedocs/rtd_tests/tests/test_sync_versions.py b/readthedocs/rtd_tests/tests/test_sync_versions.py index 5c56935953d..226d18efea8 100644 --- a/readthedocs/rtd_tests/tests/test_sync_versions.py +++ b/readthedocs/rtd_tests/tests/test_sync_versions.py @@ -1,4 +1,3 @@ -import json from unittest import mock from django.conf import settings @@ -14,7 +13,6 @@ ) from readthedocs.builds.tasks import sync_versions_task from readthedocs.organizations.models import Organization, OrganizationOwner -from readthedocs.projects.constants import PUBLIC from readthedocs.projects.models import Project @@ -726,6 +724,57 @@ def test_deletes_version_with_same_identifier(self): 1, ) + def test_versions_with_same_verbose_name(self): + get( + Version, + project=self.pip, + identifier="v2", + verbose_name="v2", + active=True, + type=BRANCH, + ) + get( + Version, + project=self.pip, + identifier="1234abc", + verbose_name="v2", + active=True, + type=TAG, + ) + branches_data = [ + { + "identifier": "v2", + "verbose_name": "v2", + }, + ] + tags_data = [ + { + # THe identifier has changed! + "identifier": "12345abc", + "verbose_name": "v2", + }, + ] + + sync_versions_task( + self.pip.pk, + branches_data=branches_data, + tags_data=tags_data, + ) + + self.assertEqual( + self.pip.versions.filter( + verbose_name="v2", identifier="v2", type=BRANCH + ).count(), + 1, + ) + self.assertEqual( + self.pip.versions.filter( + verbose_name="v2", identifier="12345abc", type=TAG + ).count(), + 1, + ) + + @mock.patch('readthedocs.builds.tasks.run_automation_rules') def test_automation_rules_are_triggered_for_new_versions(self, run_automation_rules): Version.objects.create( From 7205620398d461a8257c149048d0f0f0701c5b2d Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Tue, 29 Mar 2022 17:35:05 -0500 Subject: [PATCH 3/3] Don't use the origin/ part for branches It's redundant as we always have only one remote (origin), and we need to match the same behavior of lsremote. --- readthedocs/rtd_tests/tests/test_backend.py | 8 ++++---- readthedocs/vcs_support/backends/git.py | 14 ++++++++++---- 2 files changed, 14 insertions(+), 8 deletions(-) diff --git a/readthedocs/rtd_tests/tests/test_backend.py b/readthedocs/rtd_tests/tests/test_backend.py index 30b144e45d3..af931c5c345 100644 --- a/readthedocs/rtd_tests/tests/test_backend.py +++ b/readthedocs/rtd_tests/tests/test_backend.py @@ -75,8 +75,8 @@ def test_git_lsremote(self): repo_branches, repo_tags = repo.lsremote self.assertEqual( - set(branches + default_branches), - {branch.verbose_name for branch in repo_branches}, + {branch: branch for branch in default_branches + branches}, + {branch.verbose_name: branch.identifier for branch in repo_branches}, ) self.assertEqual( @@ -111,8 +111,8 @@ def test_git_branches(self, checkout_path): repo.clone() self.assertEqual( - set(branches + default_branches), - {branch.verbose_name for branch in repo.branches}, + {branch: branch for branch in default_branches + branches}, + {branch.verbose_name: branch.identifier for branch in repo.branches}, ) @patch('readthedocs.projects.models.Project.checkout_path') diff --git a/readthedocs/vcs_support/backends/git.py b/readthedocs/vcs_support/backends/git.py index eb785729865..a5d27928e66 100644 --- a/readthedocs/vcs_support/backends/git.py +++ b/readthedocs/vcs_support/backends/git.py @@ -287,11 +287,17 @@ def branches(self): for branch in branches: verbose_name = branch.name - if verbose_name.startswith('origin/'): - verbose_name = verbose_name.replace('origin/', '') - if verbose_name == 'HEAD': + if verbose_name.startswith("origin/"): + verbose_name = verbose_name.replace("origin/", "", 1) + if verbose_name == "HEAD": continue - versions.append(VCSVersion(self, str(branch), verbose_name)) + versions.append( + VCSVersion( + repository=self, + identifier=verbose_name, + verbose_name=verbose_name, + ) + ) return versions @property