From fc13b1ad0b59d3f97c7ec6fb2b8a7935aa52a1a2 Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Sat, 24 May 2025 23:57:17 -0400 Subject: [PATCH 1/2] Create a dataclass for version info Convert from fixed-shape dicts to a dataclass. Preserve almost all code semantics identically -- e.g., variable assignments in `readthedocs.api.v2.utils` may be micro-optimizations which could be important. The only intentional semantic change included here (other than the change in types) is in `readthedocs.projects.tasks.mixins`, where a list comprehension is replaced with a generator expression. --- readthedocs/api/v2/utils.py | 8 +- readthedocs/builds/tasks.py | 30 +- readthedocs/projects/datatypes.py | 23 + readthedocs/projects/tasks/mixins.py | 26 +- .../rtd_tests/tests/test_sync_versions.py | 618 ++++++------------ 5 files changed, 275 insertions(+), 430 deletions(-) create mode 100644 readthedocs/projects/datatypes.py diff --git a/readthedocs/api/v2/utils.py b/readthedocs/api/v2/utils.py index c5a0bc8bfee..2f31f217c5a 100644 --- a/readthedocs/api/v2/utils.py +++ b/readthedocs/api/v2/utils.py @@ -48,8 +48,8 @@ def sync_versions_to_db(project, versions, type): has_user_stable = False has_user_latest = False for version in versions: - version_id = version["identifier"] - version_name = version["verbose_name"] + version_id = version.identifier + version_name = version.verbose_name if version_name == STABLE_VERBOSE_NAME: has_user_stable = True created_version, created = _set_or_create_version( @@ -170,8 +170,8 @@ def _set_or_create_version(project, slug, version_id, verbose_name, type_): def _get_deleted_versions_qs(project, tags_data, branches_data): # We use verbose_name for tags # because several tags can point to the same identifier. - versions_tags = [version["verbose_name"] for version in tags_data] - versions_branches = [version["identifier"] for version in branches_data] + versions_tags = [version.verbose_name for version in tags_data] + versions_branches = [version.identifier for version in branches_data] to_delete_qs = ( project.versions(manager=INTERNAL) diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index f23c8f2aa58..257376d7dd9 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -34,6 +34,7 @@ from readthedocs.integrations.models import HttpExchange from readthedocs.notifications.models import Notification from readthedocs.oauth.notifications import MESSAGE_OAUTH_BUILD_STATUS_FAILURE +from readthedocs.projects.datatypes import ProjectVersionInfo from readthedocs.projects.models import Project from readthedocs.projects.models import WebHookEvent from readthedocs.storage import build_commands_storage @@ -275,22 +276,33 @@ def delete_closed_external_versions(limit=200, days=30 * 3): @app.task(max_retries=1, default_retry_delay=60, queue="web") -def sync_versions_task(project_pk, tags_data, branches_data, **kwargs): +def sync_versions_task( + project_pk: int, + tags_data: list[ProjectVersionInfo], + branches_data: list[ProjectVersionInfo], + **kwargs: object, +): """ Sync the version data in the repo (from build server) into our database. - Creates new Version objects for tags/branches that aren't tracked in the database, - and deletes Version objects for tags/branches that don't exists in the repository. + Creates new Version objects for tags/branches that aren't tracked in the + database, and deletes Version objects for tags/branches that don't exists + in the repository. - :param tags_data: List of dictionaries with ``verbose_name`` and ``identifier`` + :param tags_data: List of version descriptions Example: [ - {"verbose_name": "v1.0.0", - "identifier": "67a9035990f44cb33091026d7453d51606350519"}, + ProjectVersionInfo( + verbose_name="v1.0.0", + identifier="67a9035990f44cb33091026d7453d51606350519", + ) ]. - :param branches_data: Same as ``tags_data`` but for branches (branch name, branch identifier). + :param branches_data: Same as ``tags_data`` but for branches (branch name, + branch identifier). Example: [ - {"verbose_name": "latest", - "identifier": "main"}, + ProjectVersionInfo( + verbose_name="latest", + identifier="main", + ) ]. :returns: `True` or `False` if the task succeeded. """ diff --git a/readthedocs/projects/datatypes.py b/readthedocs/projects/datatypes.py new file mode 100644 index 00000000000..a7d450e14d2 --- /dev/null +++ b/readthedocs/projects/datatypes.py @@ -0,0 +1,23 @@ +"""Simple datatypes defined using dataclasses, for describing project data. + +Contrast this with the similar 'models' module, which defines Pydantic model +classes. +""" + +import dataclasses + + +@dataclasses.dataclass +class ProjectVersionInfo: + """ + Version information for a project associated with a branch or tag. + + The name fields is the end-user facing description, e.g., as seen in the + version selector menu. + + The identifier identifies the source for the build, e.g., a branch or + commit hash. + """ + + verbose_name: str + identifier: str diff --git a/readthedocs/projects/tasks/mixins.py b/readthedocs/projects/tasks/mixins.py index 909a9919584..7cb806c58f3 100644 --- a/readthedocs/projects/tasks/mixins.py +++ b/readthedocs/projects/tasks/mixins.py @@ -7,6 +7,7 @@ from readthedocs.builds.constants import STABLE_VERBOSE_NAME from readthedocs.builds.models import APIVersion +from ..datatypes import ProjectVersionInfo from ..exceptions import RepositoryError from ..models import Feature @@ -57,18 +58,18 @@ def sync_versions(self, vcs_repository): return tags_data = [ - { - "identifier": v.identifier, - "verbose_name": v.verbose_name, - } + ProjectVersionInfo( + verbose_name=v.verbose_name, + identifier=v.identifier, + ) for v in tags ] branches_data = [ - { - "identifier": v.identifier, - "verbose_name": v.verbose_name, - } + ProjectVersionInfo( + verbose_name=v.verbose_name, + identifier=v.identifier, + ) for v in branches ] @@ -85,7 +86,11 @@ def sync_versions(self, vcs_repository): branches_data=branches_data, ) - def validate_duplicate_reserved_versions(self, tags_data, branches_data): + def validate_duplicate_reserved_versions( + self, + tags_data: list[ProjectVersionInfo], + branches_data: list[ProjectVersionInfo], + ) -> None: """ Check if there are duplicated names of reserved versions. @@ -95,8 +100,7 @@ def validate_duplicate_reserved_versions(self, tags_data, branches_data): :param data: Dict containing the versions from tags and branches """ - version_names = [version["verbose_name"] for version in tags_data + branches_data] - counter = Counter(version_names) + counter = Counter(v.verbose_name for v in tags_data + branches_data) for reserved_name in [STABLE_VERBOSE_NAME, LATEST_VERBOSE_NAME]: if counter[reserved_name] > 1: raise RepositoryError( diff --git a/readthedocs/rtd_tests/tests/test_sync_versions.py b/readthedocs/rtd_tests/tests/test_sync_versions.py index d7339f28cfa..93c45668bfc 100644 --- a/readthedocs/rtd_tests/tests/test_sync_versions.py +++ b/readthedocs/rtd_tests/tests/test_sync_versions.py @@ -14,6 +14,7 @@ from readthedocs.builds.tasks import sync_versions_task from readthedocs.organizations.models import Organization, OrganizationOwner from readthedocs.projects.models import Project +from readthedocs.projects.datatypes import ProjectVersionInfo @mock.patch("readthedocs.core.utils.trigger_build", mock.MagicMock()) @@ -58,14 +59,12 @@ def setUp(self): def test_proper_url_no_slash(self): branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, - { - "identifier": "origin/to_add", - "verbose_name": "to_add", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), + ProjectVersionInfo( + verbose_name="to_add", identifier="origin/to_add" + ), ] self.assertEqual( @@ -92,24 +91,16 @@ def test_new_tag_update_active(self): self.pip.update_stable_version() branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, - { - "identifier": "origin/to_add", - "verbose_name": "to_add", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), + ProjectVersionInfo( + verbose_name="to_add", identifier="origin/to_add" + ), ] tags_data = [ - { - "identifier": "0.9", - "verbose_name": "0.9", - }, - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="0.9", identifier="0.9"), + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] sync_versions_task( @@ -137,24 +128,16 @@ def test_new_tag_dont_update_inactive(self): self.pip.update_stable_version() branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, - { - "identifier": "origin/to_add", - "verbose_name": "to_add", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), + ProjectVersionInfo( + verbose_name="to_add", identifier="origin/to_add" + ), ] tags_data = [ - { - "identifier": "0.9", - "verbose_name": "0.9", - }, - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="0.9", identifier="0.9"), + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] sync_versions_task( @@ -193,10 +176,9 @@ def test_delete_version(self): self.pip.update_stable_version() branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] self.assertTrue( @@ -225,26 +207,16 @@ def test_update_stable_version_type(self): self.assertEqual(stable_version.type, TAG) branches_data = [ - { - "identifier": "master", - "verbose_name": "master", - }, - { - "identifier": "1.0", - "verbose_name": "1.0", - }, - { - "identifier": "1.1", - "verbose_name": "1.1", - }, - { - "identifier": "2.0", - "verbose_name": "2.0", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), + ProjectVersionInfo(verbose_name="1.0", identifier="1.0"), + ProjectVersionInfo(verbose_name="1.1", identifier="1.1"), + ProjectVersionInfo(verbose_name="2.0", identifier="2.0"), ] - # Deactivate all other versions, so we only have branches for consideration - # for the new stable version. + # Deactivate all other versions, so we only have branches for + # consideration for the new stable version. self.pip.versions.exclude(slug__in=[LATEST, STABLE]).update(active=False) sync_versions_task( self.pip.pk, @@ -269,16 +241,10 @@ def test_update_latest_version_type(self): self.assertEqual(latest_version.type, BRANCH) branches_data = [ - { - "identifier": "master", - "verbose_name": "master", - }, + ProjectVersionInfo(verbose_name="master", identifier="master"), ] tags_data = [ - { - "identifier": "abc123", - "verbose_name": "latest", - } + ProjectVersionInfo(verbose_name="latest", identifier="abc123"), ] # Latest is created as machine=False, and as a tag. @@ -315,14 +281,8 @@ def test_update_latest_version_type(self): sync_versions_task( self.pip.pk, branches_data=[ - { - "identifier": "master", - "verbose_name": "master", - }, - { - "identifier": "2.6", - "verbose_name": "2.6", - }, + ProjectVersionInfo(verbose_name="master", identifier="master"), + ProjectVersionInfo(verbose_name="2.6", identifier="2.6"), ], tags_data=[], ) @@ -336,16 +296,10 @@ def test_update_latest_version_type(self): sync_versions_task( self.pip.pk, branches_data=[ - { - "identifier": "master", - "verbose_name": "master", - }, + ProjectVersionInfo(verbose_name="master", identifier="master"), ], tags_data=[ - { - "identifier": "abc123", - "verbose_name": "2.6", - } + ProjectVersionInfo(verbose_name="2.6", identifier="abc123"), ], ) @@ -382,21 +336,14 @@ def test_machine_attr_when_user_define_stable_tag_and_delete_it(self): self.assertTrue(current_stable.machine) branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] tags_data = [ # User new stable - { - "identifier": "1abc2def3", - "verbose_name": "stable", - }, - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="stable", identifier="1abc2def3"), + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] sync_versions_task( @@ -413,16 +360,12 @@ def test_machine_attr_when_user_define_stable_tag_and_delete_it(self): # Deleting the tag should return the RTD's stable branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] tags_data = [ - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] sync_versions_task( @@ -454,21 +397,14 @@ def test_machine_attr_when_user_define_stable_tag_and_delete_it_new_project(self self.assertIsNone(current_stable) branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] tags_data = [ # User stable - { - "identifier": "1abc2def3", - "verbose_name": "stable", - }, - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="stable", identifier="1abc2def3"), + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] sync_versions_task( @@ -489,16 +425,12 @@ def test_machine_attr_when_user_define_stable_tag_and_delete_it_new_project(self # Deleting the tag should return the RTD's stable branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] tags_data = [ - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] sync_versions_task( @@ -545,19 +477,16 @@ def test_machine_attr_when_user_define_stable_branch_and_delete_it(self): self.assertTrue(current_stable.machine) branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), # User new stable - { - "identifier": "origin/stable", - "verbose_name": "stable", - }, - { - "identifier": "origin/0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo( + verbose_name="stable", identifier="origin/stable" + ), + ProjectVersionInfo( + verbose_name="0.8.3", identifier="origin/0.8.3" + ), ] sync_versions_task( @@ -574,14 +503,12 @@ def test_machine_attr_when_user_define_stable_branch_and_delete_it(self): # Deleting the branch should return the RTD's stable branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, - { - "identifier": "origin/0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), + ProjectVersionInfo( + verbose_name="0.8.3", identifier="origin/0.8.3" + ), ] sync_versions_task( @@ -613,19 +540,16 @@ def test_machine_attr_when_user_define_stable_branch_and_delete_it_new_project( self.assertIsNone(current_stable) branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), # User stable - { - "identifier": "origin/stable", - "verbose_name": "stable", - }, - { - "identifier": "origin/0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo( + verbose_name="stable", identifier="origin/stable" + ), + ProjectVersionInfo( + verbose_name="0.8.3", identifier="origin/0.8.3" + ), ] sync_versions_task( @@ -646,14 +570,12 @@ def test_machine_attr_when_user_define_stable_branch_and_delete_it_new_project( # Deleting the branch should return the RTD's stable branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, - { - "identifier": "origin/0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), + ProjectVersionInfo( + verbose_name="0.8.3", identifier="origin/0.8.3" + ), ] sync_versions_task( @@ -698,14 +620,8 @@ def test_restore_machine_stable_verbose_name(self): assert self.pip.get_stable_version() == custom_stable branches_data = [ - { - "identifier": "master", - "verbose_name": "master", - }, - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="master", identifier="master"), + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3") ] sync_versions_task( @@ -728,17 +644,15 @@ def test_machine_attr_when_user_define_latest_tag_and_delete_it(self): machine=True).""" branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] tags_data = [ # User new stable - { - "identifier": "1abc2def3", - "verbose_name": "latest", - }, + ProjectVersionInfo( + verbose_name="latest", identifier="1abc2def3" + ), ] sync_versions_task( @@ -756,10 +670,9 @@ def test_machine_attr_when_user_define_latest_tag_and_delete_it(self): # Deleting the tag should return the RTD's latest branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] sync_versions_task( @@ -808,15 +721,13 @@ def test_machine_attr_when_user_define_latest_branch_and_delete_it(self): machine=True). """ branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), # User new latest - { - "identifier": "origin/latest", - "verbose_name": "latest", - }, + ProjectVersionInfo( + verbose_name="latest", identifier="origin/latest" + ), ] sync_versions_task( @@ -836,10 +747,9 @@ def test_machine_attr_when_user_define_latest_branch_and_delete_it(self): # Deleting the branch should return the RTD's latest branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] sync_versions_task( @@ -884,16 +794,12 @@ def test_machine_attr_when_user_define_latest_branch_and_delete_it(self): def test_deletes_version_with_same_identifier(self): branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] tags_data = [ - { - "identifier": "1234", - "verbose_name": "one", - }, + ProjectVersionInfo(verbose_name="one", identifier="1234"), ] sync_versions_task( @@ -910,20 +816,13 @@ def test_deletes_version_with_same_identifier(self): # We add a new tag with the same identifier branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] tags_data = [ - { - "identifier": "1234", - "verbose_name": "two", - }, - { - "identifier": "1234", - "verbose_name": "one", - }, + ProjectVersionInfo(verbose_name="two", identifier="1234"), + ProjectVersionInfo(verbose_name="one", identifier="1234"), ] sync_versions_task( @@ -940,16 +839,12 @@ def test_deletes_version_with_same_identifier(self): # We delete one version with identifier `1234` branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] tags_data = [ - { - "identifier": "1234", - "verbose_name": "one", - }, + ProjectVersionInfo(verbose_name="one", identifier="1234"), ] sync_versions_task( @@ -982,17 +877,13 @@ def test_versions_with_same_verbose_name(self): type=TAG, ) branches_data = [ - { - "identifier": "v2", - "verbose_name": "v2", - }, + ProjectVersionInfo(verbose_name="v2", identifier="v2"), ] tags_data = [ - { - # THe identifier has changed! - "identifier": "12345abc", - "verbose_name": "v2", - }, + # The identifier has changed! + ProjectVersionInfo( + verbose_name="v2", identifier="12345abc" + ), ] sync_versions_task( @@ -1027,24 +918,16 @@ def test_automation_rules_are_triggered_for_new_versions( ) branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, - { - "identifier": "origin/new_branch", - "verbose_name": "new_branch", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), + ProjectVersionInfo( + verbose_name="new_branch", identifier="origin/new_branch" + ), ] tags_data = [ - { - "identifier": "new_tag", - "verbose_name": "new_tag", - }, - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="new_tag", identifier="new_tag"), + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] sync_versions_task( self.pip.pk, @@ -1060,14 +943,8 @@ def test_automation_rules_are_triggered_for_new_versions( @mock.patch("readthedocs.builds.automation_actions.trigger_build", mock.MagicMock()) def test_automation_rule_activate_version(self): tags_data = [ - { - "identifier": "new_tag", - "verbose_name": "new_tag", - }, - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="new_tag", identifier="new_tag"), + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] RegexAutomationRule.objects.create( project=self.pip, @@ -1088,14 +965,8 @@ def test_automation_rule_activate_version(self): @mock.patch("readthedocs.builds.automation_actions.trigger_build", mock.MagicMock()) def test_automation_rule_set_default_version(self): tags_data = [ - { - "identifier": "new_tag", - "verbose_name": "new_tag", - }, - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="new_tag", identifier="new_tag"), + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] RegexAutomationRule.objects.create( project=self.pip, @@ -1115,14 +986,8 @@ def test_automation_rule_set_default_version(self): def test_automation_rule_delete_version(self): tags_data = [ - { - "identifier": "new_tag", - "verbose_name": "new_tag", - }, - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="new_tag", identifier="new_tag"), + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] version_slug = "0.8" RegexAutomationRule.objects.create( @@ -1144,14 +1009,8 @@ def test_automation_rule_delete_version(self): def test_automation_rule_dont_delete_default_version(self): tags_data = [ - { - "identifier": "new_tag", - "verbose_name": "new_tag", - }, - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="new_tag", identifier="new_tag"), + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] version_slug = "0.8" RegexAutomationRule.objects.create( @@ -1199,24 +1058,16 @@ def setUp(self): def test_stable_versions(self): branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, - { - "identifier": "origin/to_add", - "verbose_name": "to_add", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), + ProjectVersionInfo( + verbose_name="to_add", identifier="origin/to_add" + ), ] tags_data = [ - { - "identifier": "0.9", - "verbose_name": "0.9", - }, - { - "identifier": "0.8", - "verbose_name": "0.8", - }, + ProjectVersionInfo(verbose_name="0.9", identifier="0.9"), + ProjectVersionInfo(verbose_name="0.8", identifier="0.8"), ] self.assertRaises( @@ -1235,11 +1086,11 @@ def test_stable_versions(self): def test_pre_release_are_not_stable(self): tags_data = [ - {"identifier": "1.0a1", "verbose_name": "1.0a1"}, - {"identifier": "0.9", "verbose_name": "0.9"}, - {"identifier": "0.9b1", "verbose_name": "0.9b1"}, - {"identifier": "0.8", "verbose_name": "0.8"}, - {"identifier": "0.8rc2", "verbose_name": "0.8rc2"}, + ProjectVersionInfo(verbose_name="1.0a1", identifier="1.0a1"), + ProjectVersionInfo(verbose_name="0.9", identifier="0.9"), + ProjectVersionInfo(verbose_name="0.9b1", identifier="0.9b1"), + ProjectVersionInfo(verbose_name="0.8", identifier="0.8"), + ProjectVersionInfo(verbose_name="0.8rc2", identifier="0.8rc2"), ] sync_versions_task( @@ -1253,8 +1104,10 @@ def test_pre_release_are_not_stable(self): def test_post_releases_are_stable(self): tags_data = [ - {"identifier": "1.0", "verbose_name": "1.0"}, - {"identifier": "1.0.post1", "verbose_name": "1.0.post1"}, + ProjectVersionInfo(verbose_name="1.0", identifier="1.0"), + ProjectVersionInfo( + verbose_name="1.0.post1", identifier="1.0.post1" + ), ] sync_versions_task( @@ -1284,14 +1137,10 @@ def test_invalid_version_numbers_are_not_stable(self): self.assertFalse(Version.objects.filter(slug=STABLE).exists()) tags_data = [ - { - "identifier": "1.0", - "verbose_name": "1.0", - }, - { - "identifier": "this.is.invalid", - "verbose_name": "this.is.invalid", - }, + ProjectVersionInfo(verbose_name="1.0", identifier="1.0"), + ProjectVersionInfo( + verbose_name="this.is.invalid", identifier="this.is.invalid" + ), ] sync_versions_task( @@ -1305,20 +1154,13 @@ def test_invalid_version_numbers_are_not_stable(self): def test_update_stable_version(self): branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] tags_data = [ - { - "identifier": "0.9", - "verbose_name": "0.9", - }, - { - "identifier": "0.8", - "verbose_name": "0.8", - }, + ProjectVersionInfo(verbose_name="0.9", identifier="0.9"), + ProjectVersionInfo(verbose_name="0.8", identifier="0.8"), ] self.pip.update_stable_version() @@ -1333,10 +1175,7 @@ def test_update_stable_version(self): self.assertEqual(version_stable.identifier, "0.9") tags_data = [ - { - "identifier": "1.0.0", - "verbose_name": "1.0.0", - }, + ProjectVersionInfo(verbose_name="1.0.0", identifier="1.0.0"), ] sync_versions_task( @@ -1350,10 +1189,7 @@ def test_update_stable_version(self): self.assertEqual(version_stable.identifier, "1.0.0") tags_data = [ - { - "identifier": "0.7", - "verbose_name": "0.7", - }, + ProjectVersionInfo(verbose_name="0.7", identifier="0.7"), ] sync_versions_task( @@ -1368,16 +1204,12 @@ def test_update_stable_version(self): def test_update_inactive_stable_version(self): branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] tags_data = [ - { - "identifier": "0.9", - "verbose_name": "0.9", - }, + ProjectVersionInfo(verbose_name="0.9", identifier="0.9"), ] self.pip.update_stable_version() @@ -1393,10 +1225,7 @@ def test_update_inactive_stable_version(self): version_stable.save() tags_data.append( - { - "identifier": "1.0.0", - "verbose_name": "1.0.0", - } + ProjectVersionInfo(verbose_name="1.0.0", identifier="1.0.0"), ) sync_versions_task( @@ -1414,12 +1243,14 @@ def test_update_inactive_stable_version(self): def test_stable_version_tags_over_branches(self): branches_data = [ # 2.0 development - {"identifier": "origin/2.0", "verbose_name": "2.0"}, - {"identifier": "origin/0.9.1rc1", "verbose_name": "0.9.1rc1"}, + ProjectVersionInfo(verbose_name="2.0", identifier="origin/2.0"), + ProjectVersionInfo( + verbose_name="0.9.1rc1", identifier="origin/0.9.1rc1" + ), ] tags_data = [ - {"identifier": "1.0rc1", "verbose_name": "1.0rc1"}, - {"identifier": "0.9", "verbose_name": "0.9"}, + ProjectVersionInfo(verbose_name="1.0rc1", identifier="1.0rc1"), + ProjectVersionInfo(verbose_name="0.9", identifier="0.9"), ] self.pip.update_stable_version() @@ -1436,10 +1267,7 @@ def test_stable_version_tags_over_branches(self): self.assertEqual(version_stable.identifier, "0.9") tags_data.append( - { - "identifier": "1.0", - "verbose_name": "1.0", - } + ProjectVersionInfo(verbose_name="1.0", identifier="1.0"), ) sync_versions_task( @@ -1455,12 +1283,12 @@ def test_stable_version_tags_over_branches(self): def test_stable_version_same_id_tag_branch(self): branches_data = [ # old 1.0 development branch - {"identifier": "origin/1.0", "verbose_name": "1.0"}, + ProjectVersionInfo(verbose_name="1.0", identifier="origin/1.0"), ] tags_data = [ # tagged 1.0 final version - {"identifier": "1.0", "verbose_name": "1.0"}, - {"identifier": "0.9", "verbose_name": "0.9"}, + ProjectVersionInfo(verbose_name="1.0", identifier="1.0"), + ProjectVersionInfo(verbose_name="0.9", identifier="0.9"), ] self.pip.update_stable_version() @@ -1477,7 +1305,7 @@ def test_stable_version_same_id_tag_branch(self): def test_unicode(self): tags_data = [ - {"identifier": "foo-£", "verbose_name": "foo-£"}, + ProjectVersionInfo(verbose_name="foo-£", identifier="foo-£"), ] sync_versions_task( @@ -1506,25 +1334,15 @@ def test_user_defined_stable_version_tag_with_tags(self): ) branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] tags_data = [ # A new user-defined stable tag - { - "identifier": "1abc2def3", - "verbose_name": "stable", - }, - { - "identifier": "0.9", - "verbose_name": "0.9", - }, - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="stable", identifier="1abc2def3"), + ProjectVersionInfo(verbose_name="0.9", identifier="0.9"), + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] sync_versions_task( @@ -1591,25 +1409,17 @@ def test_user_defined_stable_version_branch_with_tags(self): self.pip.update_stable_version() branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), # A new user-defined stable branch - { - "identifier": "origin/stable", - "verbose_name": "stable", - }, + ProjectVersionInfo( + verbose_name="stable", identifier="origin/stable" + ), ] tags_data = [ - { - "identifier": "0.9", - "verbose_name": "0.9", - }, - { - "identifier": "0.8.3", - "verbose_name": "0.8.3", - }, + ProjectVersionInfo(verbose_name="0.9", identifier="0.9"), + ProjectVersionInfo(verbose_name="0.8.3", identifier="0.8.3"), ] sync_versions_task( @@ -1695,17 +1505,15 @@ def test_user_defined_latest_version_tag(self): # as a BRANCH, then here we will have a # ``latest_a`` version. branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), ] tags_data = [ # A new user-defined latest tag - { - "identifier": "1abc2def3", - "verbose_name": "latest", - }, + ProjectVersionInfo( + verbose_name="latest", identifier="1abc2def3" + ), ] sync_versions_task( @@ -1750,15 +1558,13 @@ def test_user_defined_latest_version_tag(self): def test_user_defined_latest_version_branch(self): branches_data = [ - { - "identifier": "origin/master", - "verbose_name": "master", - }, + ProjectVersionInfo( + verbose_name="master", identifier="origin/master" + ), # A new user-defined latest branch - { - "identifier": "origin/latest", - "verbose_name": "latest", - }, + ProjectVersionInfo( + verbose_name="latest", identifier="origin/latest" + ), ] sync_versions_task( From a795e057d9515bca5019d911f518191551580480 Mon Sep 17 00:00:00 2001 From: Stephen Rosen Date: Fri, 21 Nov 2025 22:42:33 -0600 Subject: [PATCH 2/2] Pass ProjectVersionInfo as JSON-safe data In order to safely pass the data across the celery task `defer` call, which implicitly JSON de/serializes data, the objects need to be converted to and from a JSON-compatible shape. As a simple paradigm for handling this, `serialize_many` and `parse_many` are added to `ProjectVersionInfo` as classmethods for converting to and from a list of dicts. By structuring `ProjectVersionInfo` as a decorator, it's possible to keep the code within the task definition the same as it was before. --- readthedocs/builds/tasks.py | 2 ++ readthedocs/projects/datatypes.py | 45 ++++++++++++++++++++++++++++ readthedocs/projects/tasks/mixins.py | 4 +-- 3 files changed, 49 insertions(+), 2 deletions(-) diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index 257376d7dd9..b29b394ab52 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -276,8 +276,10 @@ def delete_closed_external_versions(limit=200, days=30 * 3): @app.task(max_retries=1, default_retry_delay=60, queue="web") +@ProjectVersionInfo.parse_many("tags_data", "branches_data") def sync_versions_task( project_pk: int, + *, tags_data: list[ProjectVersionInfo], branches_data: list[ProjectVersionInfo], **kwargs: object, diff --git a/readthedocs/projects/datatypes.py b/readthedocs/projects/datatypes.py index a7d450e14d2..548346f173e 100644 --- a/readthedocs/projects/datatypes.py +++ b/readthedocs/projects/datatypes.py @@ -4,7 +4,19 @@ classes. """ +from __future__ import annotations + import dataclasses +import functools +import sys +import typing + + +if typing.TYPE_CHECKING: + if sys.version_info >= (3, 11): + from typing import Self + else: + from typing_extensions import Self @dataclasses.dataclass @@ -21,3 +33,36 @@ class ProjectVersionInfo: verbose_name: str identifier: str + + @classmethod + def serialize_many(cls, data: typing.Sequence[Self]) -> list[dict[str, str]]: + r""" + A preparatory step which converts a sequence of ``ProjectVersionInfo``\s into a + format suitable for JSON serialization. + """ + return [dataclasses.asdict(item) for item in data] + + @classmethod + def parse_many(cls, *argument_names: str) -> typing.Callable[..., typing.Any]: + """ + A decorator which adds a parsing step to deserialize JSON-ified data. + + The keyword argument names which will be targetted for parsing must be supplied + as strings to ``parse_many()``. + """ + + def decorator(func: typing.Callable[..., typing.Any]) -> typing.Callable[..., typing.Any]: + @functools.wraps(func) + def wrapper(*args: typing.Any, **kwargs: typing.Any) -> typing.Any: + for name in argument_names: + if name not in kwargs: + raise TypeError(f"Missing required argument: {name}") + kwargs[name] = [ + item if isinstance(item, ProjectVersionInfo) else cls(**item) + for item in kwargs[name] + ] + return func(*args, **kwargs) + + return wrapper + + return decorator diff --git a/readthedocs/projects/tasks/mixins.py b/readthedocs/projects/tasks/mixins.py index 7cb806c58f3..017edc6fc74 100644 --- a/readthedocs/projects/tasks/mixins.py +++ b/readthedocs/projects/tasks/mixins.py @@ -82,8 +82,8 @@ def sync_versions(self, vcs_repository): build_tasks.sync_versions_task.delay( project_pk=self.data.project.pk, - tags_data=tags_data, - branches_data=branches_data, + tags_data=ProjectVersionInfo.serialize_many(tags_data), + branches_data=ProjectVersionInfo.serialize_many(branches_data), ) def validate_duplicate_reserved_versions(