From ada1ef5e93f1f13c19c069330ded86b594d712b4 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 1 Dec 2025 11:21:06 +0000 Subject: [PATCH 1/6] Initial plan From 1fcb6685a30c7ddc7090c088522f51f085020eef Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 1 Dec 2025 11:48:07 +0000 Subject: [PATCH 2/6] Upgrade STORAGES setting to include custom storage backends Co-authored-by: humitos <244656+humitos@users.noreply.github.com> --- readthedocs/projects/tasks/storage.py | 17 ++++++++++++++--- readthedocs/settings/base.py | 17 +++++++++++++++-- readthedocs/settings/docker_compose.py | 14 +++++++++++++- readthedocs/settings/test.py | 9 +++++++++ readthedocs/storage/__init__.py | 9 +++++---- 5 files changed, 56 insertions(+), 10 deletions(-) diff --git a/readthedocs/projects/tasks/storage.py b/readthedocs/projects/tasks/storage.py index 4613cb71aba..632721b3091 100644 --- a/readthedocs/projects/tasks/storage.py +++ b/readthedocs/projects/tasks/storage.py @@ -3,7 +3,7 @@ import structlog from django.conf import settings -from django.utils.module_loading import import_string +from django.core.files.storage import storages from readthedocs.doc_builder.exceptions import BuildAppError from readthedocs.projects.models import Feature @@ -53,13 +53,24 @@ def _get_storage_class(storage_type: StorageType): raise ValueError("Invalid storage type") +def _get_storage_backend(alias: str): + """ + Get the storage backend class for a given alias from STORAGES setting. + + This returns the class, not an instance, so it can be instantiated + with custom kwargs (e.g., per-build credentials). + """ + storage = storages[alias] + return type(storage) + + def _get_build_media_storage_class(): """ Get a storage class to use for syncing build artifacts. This is done in a separate method to make it easier to mock in tests. """ - return import_string(settings.RTD_BUILD_MEDIA_STORAGE) + return _get_storage_backend("build-media") def _get_build_tools_storage_class(): @@ -68,7 +79,7 @@ def _get_build_tools_storage_class(): This is done in a separate method to make it easier to mock in tests. """ - return import_string(settings.RTD_BUILD_TOOLS_STORAGE) + return _get_storage_backend("build-tools") def _get_s3_scoped_credentials(*, project, build_id, api_client, storage_type: StorageType): diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 2c8eba8f030..729e57d6b12 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -1192,16 +1192,29 @@ def GITHUB_APP_CLIENT_ID(self): @property def STORAGES(self): # https://django-storages.readthedocs.io/en/latest/backends/amazon-S3.html + # https://docs.djangoproject.com/en/5.0/ref/settings/#std-setting-STORAGES return { + "default": { + "BACKEND": "django.core.files.storage.FileSystemStorage", + }, "staticfiles": { - "BACKEND": "readthedocs.storage.s3_storage.S3StaticStorage" + "BACKEND": "readthedocs.storage.s3_storage.S3StaticStorage", + }, + "build-media": { + "BACKEND": self.RTD_BUILD_MEDIA_STORAGE, + }, + "build-commands": { + "BACKEND": self.RTD_BUILD_COMMANDS_STORAGE, + }, + "build-tools": { + "BACKEND": self.RTD_BUILD_TOOLS_STORAGE, }, "usercontent": { "BACKEND": "django.core.files.storage.FileSystemStorage", "OPTIONS": { "location": Path(self.MEDIA_ROOT) / "usercontent", "allow_overwrite": True, - } + }, }, } diff --git a/readthedocs/settings/docker_compose.py b/readthedocs/settings/docker_compose.py index 105ed8decef..269d635c7dd 100644 --- a/readthedocs/settings/docker_compose.py +++ b/readthedocs/settings/docker_compose.py @@ -243,8 +243,20 @@ def SOCIALACCOUNT_PROVIDERS(self): @property def STORAGES(self): return { + "default": { + "BACKEND": "django.core.files.storage.FileSystemStorage", + }, "staticfiles": { - "BACKEND": "readthedocs.storage.s3_storage.S3StaticStorage" + "BACKEND": "readthedocs.storage.s3_storage.S3StaticStorage", + }, + "build-media": { + "BACKEND": self.RTD_BUILD_MEDIA_STORAGE, + }, + "build-commands": { + "BACKEND": self.RTD_BUILD_COMMANDS_STORAGE, + }, + "build-tools": { + "BACKEND": self.RTD_BUILD_TOOLS_STORAGE, }, "usercontent": { "BACKEND": "storages.backends.s3.S3Storage", diff --git a/readthedocs/settings/test.py b/readthedocs/settings/test.py index 7713667db33..501ce697367 100644 --- a/readthedocs/settings/test.py +++ b/readthedocs/settings/test.py @@ -142,6 +142,15 @@ def STORAGES(self): "staticfiles": { "BACKEND": "django.contrib.staticfiles.storage.StaticFilesStorage", }, + "build-media": { + "BACKEND": self.RTD_BUILD_MEDIA_STORAGE, + }, + "build-commands": { + "BACKEND": self.RTD_BUILD_COMMANDS_STORAGE, + }, + "build-tools": { + "BACKEND": self.RTD_BUILD_TOOLS_STORAGE, + }, "usercontent": { "BACKEND": "django.core.files.storage.FileSystemStorage", "OPTIONS": { diff --git a/readthedocs/storage/__init__.py b/readthedocs/storage/__init__.py index 1f8981c7d51..35ff59ae736 100644 --- a/readthedocs/storage/__init__.py +++ b/readthedocs/storage/__init__.py @@ -7,6 +7,7 @@ """ from django.conf import settings +from django.core.files.storage import storages from django.utils.functional import LazyObject from django.utils.module_loading import import_string @@ -22,22 +23,22 @@ def get_storage_class(import_path=None): class ConfiguredBuildMediaStorage(LazyObject): def _setup(self): - self._wrapped = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)() + self._wrapped = storages["build-media"] class ConfiguredBuildCommandsStorage(LazyObject): def _setup(self): - self._wrapped = get_storage_class(settings.RTD_BUILD_COMMANDS_STORAGE)() + self._wrapped = storages["build-commands"] class ConfiguredBuildToolsStorage(LazyObject): def _setup(self): - self._wrapped = get_storage_class(settings.RTD_BUILD_TOOLS_STORAGE)() + self._wrapped = storages["build-tools"] class ConfiguredStaticStorage(LazyObject): def _setup(self): - self._wrapped = get_storage_class(settings.RTD_STATICFILES_STORAGE)() + self._wrapped = storages["staticfiles"] build_media_storage = ConfiguredBuildMediaStorage() From 90de92a6479850553e073ac7404fae40b51f14e0 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Mon, 1 Dec 2025 11:55:19 +0000 Subject: [PATCH 3/6] Address code review feedback: update test files to use storages API Co-authored-by: humitos <244656+humitos@users.noreply.github.com> --- readthedocs/proxito/tests/base.py | 7 ++--- .../rtd_tests/tests/test_imported_file.py | 7 ++--- readthedocs/storage/__init__.py | 26 ++++++++++++++----- 3 files changed, 25 insertions(+), 15 deletions(-) diff --git a/readthedocs/proxito/tests/base.py b/readthedocs/proxito/tests/base.py index 869a54fe994..e4dc231a118 100644 --- a/readthedocs/proxito/tests/base.py +++ b/readthedocs/proxito/tests/base.py @@ -2,9 +2,8 @@ import django_dynamic_fixture as fixture import pytest -from django.conf import settings from django.contrib.auth.models import User -from readthedocs.storage import get_storage_class +from django.core.files.storage import storages from django.test import TestCase from readthedocs.builds.constants import LATEST @@ -19,9 +18,7 @@ def setUp(self): # Re-initialize storage # Various tests override either this setting or various aspects of the storage engine # By resetting it every test case, we avoid this caching (which is a huge benefit in prod) - serve.build_media_storage = get_storage_class( - settings.RTD_BUILD_MEDIA_STORAGE - )() + serve.build_media_storage = storages["build-media"] self.eric = fixture.get(User, username="eric") self.eric.set_password("eric") diff --git a/readthedocs/rtd_tests/tests/test_imported_file.py b/readthedocs/rtd_tests/tests/test_imported_file.py index d5d901225e6..4f0f0a85701 100644 --- a/readthedocs/rtd_tests/tests/test_imported_file.py +++ b/readthedocs/rtd_tests/tests/test_imported_file.py @@ -2,8 +2,7 @@ from unittest import mock import pytest -from django.conf import settings -from readthedocs.storage import get_storage_class +from django.core.files.storage import storages from django.test import TestCase from django.test.utils import override_settings from django_dynamic_fixture import get @@ -21,7 +20,9 @@ @pytest.mark.search @override_settings(ELASTICSEARCH_DSL_AUTOSYNC=True) class ImportedFileTests(TestCase): - storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)() + @property + def storage(self): + return storages["build-media"] def setUp(self): self.project = get(Project) diff --git a/readthedocs/storage/__init__.py b/readthedocs/storage/__init__.py index 35ff59ae736..0cdb9c47dc2 100644 --- a/readthedocs/storage/__init__.py +++ b/readthedocs/storage/__init__.py @@ -6,19 +6,31 @@ so doing those upfront improves performance. """ -from django.conf import settings +import warnings + from django.core.files.storage import storages from django.utils.functional import LazyObject from django.utils.module_loading import import_string -# Borrowed from Django 4.2 since it was deprecrated and removed in 5.2 -# NOTE: we can use settings.STORAGES for our own storages as well if we want to use the standards. -# -# https://docs.djangoproject.com/en/5.0/ref/settings/#std-setting-STORAGES) -# https://github.com/django/django/blob/4.2/django/core/files/storage/__init__.py#L31 def get_storage_class(import_path=None): - return import_string(import_path or settings.DEFAULT_FILE_STORAGE) + """ + Get a storage class from an import path. + + .. deprecated:: + This function is deprecated. Use Django's ``storages`` API instead: + ``from django.core.files.storage import storages`` + and access storage by alias: ``storages["build-media"]`` + """ + warnings.warn( + "get_storage_class is deprecated. Use Django's storages API instead: " + "from django.core.files.storage import storages; storages['alias']", + DeprecationWarning, + stacklevel=2, + ) + from django.conf import settings as django_settings + + return import_string(import_path or django_settings.DEFAULT_FILE_STORAGE) class ConfiguredBuildMediaStorage(LazyObject): From a848f3f8878c40ba1ad5a6be86863754a4460080 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 3 Dec 2025 12:39:28 +0100 Subject: [PATCH 4/6] Apply suggestions from code review Co-authored-by: Santos Gallegos --- readthedocs/projects/tasks/storage.py | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/readthedocs/projects/tasks/storage.py b/readthedocs/projects/tasks/storage.py index 632721b3091..9a6bc386676 100644 --- a/readthedocs/projects/tasks/storage.py +++ b/readthedocs/projects/tasks/storage.py @@ -53,7 +53,7 @@ def _get_storage_class(storage_type: StorageType): raise ValueError("Invalid storage type") -def _get_storage_backend(alias: str): +def _get_storage_backend_class(alias: str): """ Get the storage backend class for a given alias from STORAGES setting. @@ -70,7 +70,7 @@ def _get_build_media_storage_class(): This is done in a separate method to make it easier to mock in tests. """ - return _get_storage_backend("build-media") + return _get_storage_backend_class("build-media") def _get_build_tools_storage_class(): @@ -79,7 +79,7 @@ def _get_build_tools_storage_class(): This is done in a separate method to make it easier to mock in tests. """ - return _get_storage_backend("build-tools") + return _get_storage_backend_class("build-tools") def _get_s3_scoped_credentials(*, project, build_id, api_client, storage_type: StorageType): From bceb15d87f518ce9e7e861bf57e64c1ffb7f9482 Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 3 Dec 2025 12:43:31 +0100 Subject: [PATCH 5/6] Remove old `get_storage_class` --- readthedocs/storage/__init__.py | 23 ----------------------- 1 file changed, 23 deletions(-) diff --git a/readthedocs/storage/__init__.py b/readthedocs/storage/__init__.py index 0cdb9c47dc2..917b9afe316 100644 --- a/readthedocs/storage/__init__.py +++ b/readthedocs/storage/__init__.py @@ -6,31 +6,8 @@ so doing those upfront improves performance. """ -import warnings - from django.core.files.storage import storages from django.utils.functional import LazyObject -from django.utils.module_loading import import_string - - -def get_storage_class(import_path=None): - """ - Get a storage class from an import path. - - .. deprecated:: - This function is deprecated. Use Django's ``storages`` API instead: - ``from django.core.files.storage import storages`` - and access storage by alias: ``storages["build-media"]`` - """ - warnings.warn( - "get_storage_class is deprecated. Use Django's storages API instead: " - "from django.core.files.storage import storages; storages['alias']", - DeprecationWarning, - stacklevel=2, - ) - from django.conf import settings as django_settings - - return import_string(import_path or django_settings.DEFAULT_FILE_STORAGE) class ConfiguredBuildMediaStorage(LazyObject): From 247f63749ac27172a88bd1ea5c7f13959eede34b Mon Sep 17 00:00:00 2001 From: Manuel Kaufmann Date: Wed, 3 Dec 2025 12:56:54 +0100 Subject: [PATCH 6/6] Apply suggestion from @humitos --- readthedocs/settings/base.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/readthedocs/settings/base.py b/readthedocs/settings/base.py index 729e57d6b12..26f0bc24925 100644 --- a/readthedocs/settings/base.py +++ b/readthedocs/settings/base.py @@ -1192,7 +1192,7 @@ def GITHUB_APP_CLIENT_ID(self): @property def STORAGES(self): # https://django-storages.readthedocs.io/en/latest/backends/amazon-S3.html - # https://docs.djangoproject.com/en/5.0/ref/settings/#std-setting-STORAGES + # https://docs.djangoproject.com/en/5.2/ref/settings/#std-setting-STORAGES return { "default": { "BACKEND": "django.core.files.storage.FileSystemStorage",