diff --git a/readthedocs/builds/models.py b/readthedocs/builds/models.py index f04d5be2e68..a6309e9985e 100644 --- a/readthedocs/builds/models.py +++ b/readthedocs/builds/models.py @@ -306,11 +306,11 @@ def config(self): success=True, ) .order_by("-date") - .only("_config") + .only("readthedocs_yaml_config") .first() ) - if last_build: - return last_build.config + if last_build and last_build.readthedocs_yaml_config: + return last_build.readthedocs_yaml_config.data return None @property @@ -716,7 +716,8 @@ class Build(models.Model): blank=True, ) - # TODO: remove `_config` field after migrating all builds to use `readthedocs_yaml_config` + # TODO: remove _config field once we have migrated all builds. + # _config field is deprecated in favor of readthedocs_yaml_config _config = models.JSONField( _("Configuration used in the build"), null=True, @@ -782,8 +783,6 @@ class Build(models.Model): # Only include EXTERNAL type Version builds. external = ExternalBuildManager.from_queryset(BuildQuerySet)() - CONFIG_KEY = "__config" - class Meta: ordering = ["-date"] get_latest_by = "date" @@ -798,80 +797,49 @@ class Meta: def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self._config_changed = False - - @property - def previous(self): - """ - Returns the previous build to the current one. - - Matching the project and version. - """ - date = self.date or timezone.now() - if self.project is not None and self.version is not None: - return ( - Build.objects.filter( - project=self.project, - version=self.version, - date__lt=date, - ) - .order_by("-date") - .first() - ) - return None + self._readthedocs_yaml_config = None + self._readthedocs_yaml_config_changed = False @property def config(self): """ - Get the config used for this build. + Helper to get the build config data as a dict. - Since we are saving the config into the JSON field only when it differs - from the previous one, this helper returns the correct JSON used in this - Build object (it could be stored in this object or one of the previous - ones). + Keeping it for backwards compatibility for now. + We could use `readthedocs_yaml_config` directly instead if we want. """ - # TODO: now that we are using a proper JSONField here, we could - # probably change this field to be a ForeignKey to avoid repeating the - # config file over and over again and reuse them to save db data as - # well - if self._config and self.CONFIG_KEY in self._config: - return Build.objects.only("_config").get(pk=self._config[self.CONFIG_KEY])._config - return self._config + return self.readthedocs_yaml_config.data if self.readthedocs_yaml_config else {} @config.setter def config(self, value): """ - Set `_config` to value. + Helper to create a `BuildConfig` from a dict. - `_config` should never be set directly from outside the class. - """ - self._config = value - self._config_changed = True - - def save(self, *args, **kwargs): # noqa - """ - Save object. - - To save space on the db we only save the config if it's different - from the previous one. + Keeping it for backwards compatibility for now. We are using it as: - If the config is the same, we save the pk of the object - that has the **real** config under the `CONFIG_KEY` key. + build.config = { + "search": { + "ranking": { + "*index.html": 5, + } + } + } - Additionally, we create or get a BuildConfig object for the new - readthedocs_yaml_config field to facilitate the migration to the new model. + We could remove this and create `BuildConfig` objects directly instead if we want. """ - if self.pk is None or self._config_changed: - if self._config is None: - self.readthedocs_yaml_config = None - else: - build_config, created = BuildConfig.objects.get_or_create(data=self._config) - self.readthedocs_yaml_config = build_config - - previous = self.previous - if previous is not None and self._config and self._config == previous.config: - previous_pk = previous._config.get(self.CONFIG_KEY, previous.pk) - self._config = {self.CONFIG_KEY: previous_pk} + self._readthedocs_yaml_config = value + self._readthedocs_yaml_config_changed = True + + def save(self, *args, **kwargs): # noqa + if self._readthedocs_yaml_config_changed: + build_config, _ = BuildConfig.objects.get_or_create(data=self._readthedocs_yaml_config) + self.readthedocs_yaml_config = build_config + log.warning( + "Creating BuildConfig using `build.config` setter.", + build_id=self.id, + build_config_data=self._readthedocs_yaml_config, + build_config_id=build_config.id, + ) if self.version: self.version_name = self.version.verbose_name @@ -879,7 +847,8 @@ def save(self, *args, **kwargs): # noqa self.version_type = self.version.type super().save(*args, **kwargs) - self._config_changed = False + self._readthedocs_yaml_config = None + self._readthedocs_yaml_config_changed = False def get_absolute_url(self): return reverse("builds_detail", args=[self.project.slug, self.pk]) diff --git a/readthedocs/builds/tasks.py b/readthedocs/builds/tasks.py index a232b6fb922..7ae90a037aa 100644 --- a/readthedocs/builds/tasks.py +++ b/readthedocs/builds/tasks.py @@ -4,6 +4,7 @@ import requests import structlog from django.conf import settings +from django.db.models import Q from django.urls import reverse from django.utils import timezone from django.utils.translation import gettext_lazy as _ @@ -116,33 +117,20 @@ def route_for_task(self, task, args, kwargs, **__): ) return routing_queue - last_builds = version.builds.order_by("-date")[: self.N_LAST_BUILDS] # Version has used conda in previous builds - for build in last_builds.iterator(): - build_tools_python = "" - conda = None - if build.config: - build_tools_python = ( - build.config.get("build", {}) - .get("tools", {}) - .get("python", {}) - .get("version", "") - ) - conda = build.config.get("conda", None) - - uses_conda = any( - [ - conda, - build_tools_python.startswith("miniconda"), - ] + query = ( + Q(builds__in=version.builds.order_by("-date")[: self.N_LAST_BUILDS]) + & Q(data__build__tools__python__version__startswith="miniconda") + & ~Q(data__contains={"conda": None}) + ) + uses_conda = BuildConfig.objects.filter(query).exists() + if uses_conda: + log.info( + "Routing task because project uses conda.", + project_slug=project.slug, + queue=self.BUILD_LARGE_QUEUE, ) - if uses_conda: - log.info( - "Routing task because project uses conda.", - project_slug=project.slug, - queue=self.BUILD_LARGE_QUEUE, - ) - return self.BUILD_LARGE_QUEUE + return self.BUILD_LARGE_QUEUE successful_builds_count = version.builds.filter(success=True).order_by("-date").count() # We do not have enough builds for this version yet diff --git a/readthedocs/builds/tests/test_buildconfig.py b/readthedocs/builds/tests/test_buildconfig.py index 577ff9630ab..3ad43564e5d 100644 --- a/readthedocs/builds/tests/test_buildconfig.py +++ b/readthedocs/builds/tests/test_buildconfig.py @@ -22,8 +22,10 @@ def test_build_saves_with_config_creates_buildconfig(self): build.config = config_data build.save() - # Check that both old and new fields are populated - assert build._config == config_data + assert BuildConfig.objects.count() == 1 + assert BuildConfig.objects.filter(data=config_data).count() == 1 + + build.refresh_from_db() assert build.readthedocs_yaml_config.data == config_data def test_build_with_same_config_reuses_buildconfig(self): @@ -73,7 +75,6 @@ def test_build_without_config_does_not_create_buildconfig(self): # Build has no config set build.save() - assert build._config is None assert build.readthedocs_yaml_config is None assert BuildConfig.objects.count() == 0 @@ -94,8 +95,6 @@ def test_build_with_config_reference_uses_same_buildconfig(self): build2.config = config_data build2.save() - # build2 should have a reference in _config, not actual data - assert Build.CONFIG_KEY in build2._config # Both builds should have the same BuildConfig assert build1.readthedocs_yaml_config is not None assert build2.readthedocs_yaml_config is not None diff --git a/readthedocs/builds/tests/test_celery_task_router.py b/readthedocs/builds/tests/test_celery_task_router.py index e79d202979c..17ecf1a9c16 100644 --- a/readthedocs/builds/tests/test_celery_task_router.py +++ b/readthedocs/builds/tests/test_celery_task_router.py @@ -42,7 +42,7 @@ def test_project_custom_queue(self): ) def test_used_conda_in_last_builds(self): - self.build._config = {"conda": {"file": "docs/environment.yml"}} + self.build.config = {"conda": {"file": "docs/environment.yml"}} self.build.save() self.assertEqual( @@ -51,7 +51,7 @@ def test_used_conda_in_last_builds(self): ) def test_used_conda_in_last_failed_build(self): - self.build._config = {"conda": {"file": "docs/environment.yml"}} + self.build.config = {"conda": {"file": "docs/environment.yml"}} self.build.success = False self.build.save() diff --git a/readthedocs/rtd_tests/tests/test_api.py b/readthedocs/rtd_tests/tests/test_api.py index 3795d13b471..638908bc28b 100644 --- a/readthedocs/rtd_tests/tests/test_api.py +++ b/readthedocs/rtd_tests/tests/test_api.py @@ -269,6 +269,7 @@ def test_api_does_not_have_private_config_key_superuser(self): resp = client.get("/api/v2/build/{}/".format(build.pk)) self.assertEqual(resp.status_code, status.HTTP_200_OK) + # TODO: update these tests to check for `readthedocs_yaml_config` instead of `_config` self.assertIn("config", resp.data) self.assertNotIn("_config", resp.data) @@ -315,12 +316,12 @@ def test_save_same_config_using_patch(self): # Checking the values from the db, just to be sure the # api isn't lying. self.assertEqual( - Build.objects.get(pk=build_one.pk)._config, + Build.objects.get(pk=build_one.pk).readthedocs_yaml_config.data, {"one": "two"}, ) self.assertEqual( - Build.objects.get(pk=build_two.pk)._config, - {Build.CONFIG_KEY: build_one.pk}, + Build.objects.get(pk=build_one.pk).readthedocs_yaml_config.pk, + Build.objects.get(pk=build_two.pk).readthedocs_yaml_config.pk, ) def test_response_building(self): diff --git a/readthedocs/rtd_tests/tests/test_builds.py b/readthedocs/rtd_tests/tests/test_builds.py index a0c2fa171f5..dbaab7de81f 100644 --- a/readthedocs/rtd_tests/tests/test_builds.py +++ b/readthedocs/rtd_tests/tests/test_builds.py @@ -12,7 +12,7 @@ GITHUB_EXTERNAL_VERSION_NAME, GITLAB_EXTERNAL_VERSION_NAME, ) -from readthedocs.builds.models import Build, Version +from readthedocs.builds.models import Build, Version, BuildConfig from readthedocs.projects.models import Project @@ -55,32 +55,6 @@ def setUp(self): type=BRANCH, ) - def test_get_previous_build(self): - build_one = get( - Build, - project=self.project, - version=self.version, - _config={"version": 1}, - ) - build_two = get( - Build, - project=self.project, - version=self.version, - _config={"version": 2}, - ) - build_three = get( - Build, - project=self.project, - version=self.version, - _config={"version": 3}, - success=False, - ) - - self.assertIsNone(build_one.previous) - self.assertEqual(build_two.previous, build_one) - self.assertEqual(build_three.previous, build_two) - self.assertEqual(build_three.previous.previous, build_one) - def test_normal_save_config(self): build = get( Build, @@ -88,12 +62,16 @@ def test_normal_save_config(self): version=self.version, ) build.config = {"version": 1} + self.assertEqual(BuildConfig.objects.count(), 0) + build.save() - self.assertEqual(build.config, {"version": 1}) + self.assertEqual(BuildConfig.objects.count(), 1) + self.assertEqual(build.readthedocs_yaml_config.data, {"version": 1}) build.config = {"version": 2} build.save() - self.assertEqual(build.config, {"version": 2}) + self.assertEqual(BuildConfig.objects.count(), 2) + self.assertEqual(build.readthedocs_yaml_config.data, {"version": 2}) def test_save_same_config(self): build_one = get( @@ -114,111 +92,6 @@ def test_save_same_config(self): self.assertEqual(build_two.config, {"version": 2}) - def test_save_same_config_previous_empty(self): - build_one = get( - Build, - project=self.project, - version=self.version, - ) - build_one.config = {} - build_one.save() - - build_two = get( - Build, - project=self.project, - version=self.version, - ) - build_two.config = {} - build_two.save() - - self.assertEqual(build_two.config, {}) - build_two.config = {"version": 2} - build_two.save() - self.assertEqual(build_two.config, {"version": 2}) - - def test_do_not_save_same_config(self): - build_one = get( - Build, - project=self.project, - version=self.version, - ) - build_one.config = {"version": 1} - build_one.save() - - build_two = get( - Build, - project=self.project, - version=self.version, - ) - build_two.config = {"version": 1} - build_two.save() - self.assertEqual(build_two._config, {Build.CONFIG_KEY: build_one.pk}) - self.assertEqual(build_two.config, {"version": 1}) - - def test_do_not_save_same_config_nested(self): - build_one = get( - Build, - project=self.project, - version=self.version, - ) - build_one.config = {"version": 1} - build_one.save() - - build_two = get( - Build, - project=self.project, - version=self.version, - ) - build_two.config = {"version": 1} - build_two.save() - - build_three = get( - Build, - project=self.project, - version=self.version, - ) - build_three.config = {"version": 1} - build_three.save() - - build_four = get( - Build, - project=self.project, - version=self.version, - ) - build_four.config = {"version": 2} - build_four.save() - - self.assertEqual(build_one.config, {"version": 1}) - self.assertEqual(build_one._config, {"version": 1}) - - self.assertEqual(build_two._config, {Build.CONFIG_KEY: build_one.pk}) - self.assertEqual(build_three._config, {Build.CONFIG_KEY: build_one.pk}) - - self.assertEqual(build_two.config, {"version": 1}) - self.assertEqual(build_three.config, {"version": 1}) - - self.assertEqual(build_four.config, {"version": 2}) - self.assertEqual(build_four._config, {"version": 2}) - - def test_do_not_reference_empty_configs(self): - build_one = get( - Build, - project=self.project, - version=self.version, - ) - build_one.config = {} - build_one.save() - - build_two = get( - Build, - project=self.project, - version=self.version, - ) - build_two.config = {} - build_two.save() - self.assertEqual(build_two._config, {}) - self.assertEqual(build_two.config, {}) - def test_build_is_stale(self): now = timezone.now() @@ -257,7 +130,6 @@ def test_build_is_external(self): Build, project=self.project, version=self.version, - _config={"version": 1}, ) self.assertTrue(external_build.is_external) @@ -267,7 +139,6 @@ def test_build_is_not_external(self): Build, project=self.project, version=self.version, - _config={"version": 1}, ) self.assertFalse(build.is_external) @@ -277,7 +148,6 @@ def test_no_external_version_name(self): Build, project=self.project, version=self.version, - _config={"version": 1}, ) self.assertEqual(build.external_version_name, None) @@ -313,7 +183,6 @@ def test_external_version_name_generic(self): Build, project=self.project, version=self.version, - _config={"version": 1}, ) self.assertEqual( @@ -328,7 +197,6 @@ def test_get_commit_url_external_version_github(self): Build, project=self.pip, version=self.external_version, - _config={"version": 1}, ) expected_url = "https://github.com/pypa/pip/pull/{number}/commits/{sha}".format( number=self.external_version.verbose_name, sha=external_build.commit @@ -343,7 +211,6 @@ def test_get_commit_url_external_version_gitlab(self): Build, project=self.pip, version=self.external_version, - _config={"version": 1}, ) expected_url = ( "https://gitlab.com/pypa/pip/commit/" "{commit}?merge_request_iid={number}" @@ -357,7 +224,6 @@ def test_get_commit_url_internal_version(self): Build, project=self.pip, version=self.pip_version, - _config={"version": 1}, ) expected_url = "https://github.com/pypa/pip/commit/{sha}".format( sha=build.commit @@ -393,7 +259,6 @@ def test_can_rebuild_with_regular_version(self): Build, project=self.project, version=self.version, - _config={"version": 1}, ) self.assertFalse(build.can_rebuild) @@ -408,7 +273,6 @@ def test_can_rebuild_with_external_active_version(self): Build, project=self.project, version=self.version, - _config={"version": 1}, ) self.assertTrue(external_build.can_rebuild) @@ -423,7 +287,6 @@ def test_can_rebuild_with_external_inactive_version(self): Build, project=self.project, version=self.version, - _config={"version": 1}, ) self.assertFalse(external_build.can_rebuild) @@ -438,14 +301,12 @@ def test_can_rebuild_with_old_build(self): Build, project=self.project, version=self.version, - _config={"version": 1}, ) latest_external_build = get( Build, project=self.project, version=self.version, - _config={"version": 1}, ) self.assertFalse(old_external_build.can_rebuild) diff --git a/readthedocs/rtd_tests/tests/test_version_config.py b/readthedocs/rtd_tests/tests/test_version_config.py index cd8220e98ec..90436be3ef5 100644 --- a/readthedocs/rtd_tests/tests/test_version_config.py +++ b/readthedocs/rtd_tests/tests/test_version_config.py @@ -2,7 +2,7 @@ from django_dynamic_fixture import get from readthedocs.builds.constants import BUILD_STATE_BUILDING, BUILD_STATE_FINISHED -from readthedocs.builds.models import Build, Version +from readthedocs.builds.models import Build, Version, BuildConfig from readthedocs.projects.models import Project @@ -12,29 +12,34 @@ def setUp(self): self.version = get(Version, project=self.project) def test_get_correct_config(self): + bc1 = BuildConfig.objects.create(data={"version": 1}) + bc2 = BuildConfig.objects.create(data={"version": 2}) + bc3 = BuildConfig.objects.create(data={"version": 3}) + bc4 = BuildConfig.objects.create(data={"version": 4}) + build_old = Build.objects.create( project=self.project, version=self.version, - _config={"version": 1}, + readthedocs_yaml_config=bc1, state=BUILD_STATE_FINISHED, ) build_new = Build.objects.create( project=self.project, version=self.version, - _config={"version": 2}, + readthedocs_yaml_config=bc2, state=BUILD_STATE_FINISHED, ) build_new_error = Build.objects.create( project=self.project, version=self.version, - _config={"version": 3}, success=False, + readthedocs_yaml_config=bc3, state=BUILD_STATE_FINISHED, ) build_new_unfinish = Build.objects.create( project=self.project, version=self.version, - _config={"version": 4}, + readthedocs_yaml_config=bc4, state=BUILD_STATE_BUILDING, ) self.assertEqual(self.version.config, {"version": 2}) @@ -44,7 +49,6 @@ def test_get_correct_config_when_same_config(self): Build, project=self.project, version=self.version, - _config={}, state=BUILD_STATE_FINISHED, ) build_old.config = {"version": 1} @@ -54,7 +58,6 @@ def test_get_correct_config_when_same_config(self): Build, project=self.project, version=self.version, - _config={}, state=BUILD_STATE_FINISHED, ) build_new.config = {"version": 1} @@ -64,7 +67,6 @@ def test_get_correct_config_when_same_config(self): Build, project=self.project, version=self.version, - _config={}, success=False, state=BUILD_STATE_FINISHED, ) @@ -75,7 +77,6 @@ def test_get_correct_config_when_same_config(self): Build, project=self.project, version=self.version, - _config={}, state=BUILD_STATE_BUILDING, ) build_new_unfinish.config = {"version": 1}