Use BuildConfig model in our code

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,88 +797,58 @@ 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
             self.version_slug = self.version.slug
             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])

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

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

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()
 

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):