Merge pull request #7908 from readthedocs/davidfischer/storage-classes-module-vars

Make storage classes into module level vars

Author: ericholscher

readthedocs/api/v2/views/model_views.py

@@ -5,7 +5,6 @@
 
 from allauth.socialaccount.models import SocialAccount
 from django.conf import settings
-from django.core.files.storage import get_storage_class
 from django.shortcuts import get_object_or_404
 from django.template.loader import render_to_string
 from rest_framework import decorators, permissions, status, viewsets
@@ -19,6 +18,7 @@
 from readthedocs.oauth.models import RemoteOrganization, RemoteRepository
 from readthedocs.oauth.services import GitHubService, registry
 from readthedocs.projects.models import Domain, Project
+from readthedocs.storage import build_commands_storage
 
 from ..permissions import APIPermission, APIRestrictedPermission, IsOwner
 from ..serializers import (
@@ -257,14 +257,13 @@ def retrieve(self, *args, **kwargs):
         serializer = self.get_serializer(instance)
         data = serializer.data
         if instance.cold_storage:
-            storage = get_storage_class(settings.RTD_BUILD_COMMANDS_STORAGE)()
             storage_path = '{date}/{id}.json'.format(
                 date=str(instance.date.date()),
                 id=instance.id,
             )
-            if storage.exists(storage_path):
+            if build_commands_storage.exists(storage_path):
                 try:
-                    json_resp = storage.open(storage_path).read()
+                    json_resp = build_commands_storage.open(storage_path).read()
                     data['commands'] = json.loads(json_resp)
                 except Exception:
                     log.exception(

readthedocs/builds/models.py

@@ -8,7 +8,6 @@
 
 import regex
 from django.conf import settings
-from django.core.files.storage import get_storage_class
 from django.db import models
 from django.db.models import F
 from django.urls import reverse
@@ -88,6 +87,7 @@
 )
 from readthedocs.projects.models import APIProject, Project
 from readthedocs.projects.version_handling import determine_stable_version
+from readthedocs.storage import build_environment_storage
 
 log = logging.getLogger(__name__)
 
@@ -411,8 +411,7 @@ def get_storage_paths(self):
 
     def get_storage_environment_cache_path(self):
         """Return the path of the cached environment tar file."""
-        storage = get_storage_class(settings.RTD_BUILD_ENVIRONMENT_STORAGE)()
-        return storage.join(self.project.slug, f'{self.slug}.tar')
+        return build_environment_storage.join(self.project.slug, f'{self.slug}.tar')
 
     def clean_build_path(self):
         """

readthedocs/builds/tasks.py

@@ -5,7 +5,6 @@
 
 from celery import Task
 from django.conf import settings
-from django.core.files.storage import get_storage_class
 
 from readthedocs.api.v2.serializers import BuildSerializer
 from readthedocs.api.v2.utils import (
@@ -27,6 +26,7 @@
 from readthedocs.core.utils import trigger_build
 from readthedocs.projects.models import Project
 from readthedocs.projects.tasks import send_build_status
+from readthedocs.storage import build_commands_storage
 from readthedocs.worker import app
 
 log = logging.getLogger(__name__)
@@ -158,7 +158,6 @@ def archive_builds_task(days=14, limit=200, include_cold=False, delete=False):
         queryset = queryset.exclude(cold_storage=True)
     queryset = queryset.filter(date__lt=max_date)[:limit]
 
-    storage = get_storage_class(settings.RTD_BUILD_COMMANDS_STORAGE)()
     for build in queryset:
         data = BuildSerializer(build).data['commands']
         if data:
@@ -172,7 +171,7 @@ def archive_builds_task(days=14, limit=200, include_cold=False, delete=False):
             output.seek(0)
             filename = '{date}/{id}.json'.format(date=str(build.date.date()), id=build.id)
             try:
-                storage.save(name=filename, content=output)
+                build_commands_storage.save(name=filename, content=output)
                 build.cold_storage = True
                 build.save()
                 if delete:

readthedocs/core/utils/general.py

@@ -2,13 +2,12 @@
 
 import os
 
-from django.conf import settings
-from django.core.files.storage import get_storage_class
 from django.shortcuts import get_object_or_404
 
 from readthedocs.core.utils import broadcast
 from readthedocs.projects.tasks import remove_dirs
 from readthedocs.builds.models import Version
+from readthedocs.storage import build_environment_storage
 
 
 def wipe_version_via_slugs(version_slug, project_slug):
@@ -28,5 +27,4 @@ def wipe_version_via_slugs(version_slug, project_slug):
         broadcast(type='build', task=remove_dirs, args=[(del_dir,)])
 
     # Delete the cache environment from storage
-    storage = get_storage_class(settings.RTD_BUILD_ENVIRONMENT_STORAGE)()
-    storage.delete(version.get_storage_environment_cache_path())
+    build_environment_storage.delete(version.get_storage_environment_cache_path())

readthedocs/projects/models.py

@@ -11,7 +11,6 @@
 from django.conf import settings
 from django.conf.urls import include
 from django.contrib.auth.models import User
-from django.core.files.storage import get_storage_class
 from django.core.validators import MaxValueValidator, MinValueValidator
 from django.db import models
 from django.db.models import Prefetch
@@ -46,6 +45,7 @@
 )
 from readthedocs.projects.version_handling import determine_stable_version
 from readthedocs.search.parsers import MkDocsParser, SphinxParser
+from readthedocs.storage import build_media_storage
 from readthedocs.vcs_support.backends import backend_cls
 from readthedocs.vcs_support.utils import Lock, NonBlockingLock
 
@@ -844,12 +844,11 @@ def has_good_build(self):
         return self.builds(manager=INTERNAL).filter(success=True).exists()
 
     def has_media(self, type_, version_slug=LATEST, version_type=None):
-        storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)()
         storage_path = self.get_storage_path(
             type_=type_, version_slug=version_slug,
             version_type=version_type
         )
-        return storage.exists(storage_path)
+        return build_media_storage.exists(storage_path)
 
     def has_pdf(self, version_slug=LATEST, version_type=None):
         return self.has_media(

readthedocs/projects/tasks.py

@@ -21,7 +21,6 @@
 import requests
 from celery.exceptions import SoftTimeLimitExceeded
 from django.conf import settings
-from django.core.files.storage import get_storage_class
 from django.db.models import Q
 from django.urls import reverse
 from django.utils import timezone
@@ -73,6 +72,7 @@
 from readthedocs.projects.models import APIProject, Feature
 from readthedocs.search.utils import index_new_files, remove_indexed_files
 from readthedocs.sphinx_domains.models import SphinxDomain
+from readthedocs.storage import build_media_storage, build_environment_storage
 from readthedocs.vcs_support import utils as vcs_support_utils
 from readthedocs.worker import app
 
@@ -98,7 +98,6 @@ def pull_cached_environment(self):
         if not self.project.has_feature(feature_id=Feature.CACHED_ENVIRONMENT):
             return
 
-        storage = get_storage_class(settings.RTD_BUILD_ENVIRONMENT_STORAGE)()
         filename = self.version.get_storage_environment_cache_path()
 
         msg = 'Checking for cached environment'
@@ -110,7 +109,7 @@ def pull_cached_environment(self):
                 'msg': msg,
             }
         )
-        if storage.exists(filename):
+        if build_environment_storage.exists(filename):
             msg = 'Pulling down cached environment from storage'
             log.info(
                 LOG_TEMPLATE,
@@ -120,7 +119,7 @@ def pull_cached_environment(self):
                     'msg': msg,
                 }
             )
-            remote_fd = storage.open(filename, mode='rb')
+            remote_fd = build_environment_storage.open(filename, mode='rb')
             with tarfile.open(fileobj=remote_fd) as tar:
                 tar.extractall(self.project.doc_path)
 
@@ -153,7 +152,6 @@ def push_cached_environment(self):
             if os.path.exists(path):
                 tar.add(path, arcname='.cache')
 
-        storage = get_storage_class(settings.RTD_BUILD_ENVIRONMENT_STORAGE)()
         with open(tmp_filename, 'rb') as fd:
             msg = 'Pushing up cached environment to storage'
             log.info(
@@ -164,7 +162,7 @@ def push_cached_environment(self):
                     'msg': msg,
                 }
             )
-            storage.save(
+            build_environment_storage.save(
                 self.version.get_storage_environment_cache_path(),
                 fd,
             )
@@ -1030,7 +1028,6 @@ def store_build_artifacts(
         :param pdf: whether to save PDF output
         :param epub: whether to save ePub output
         """
-        storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)()
         log.info(
             LOG_TEMPLATE,
             {
@@ -1086,7 +1083,7 @@ def store_build_artifacts(
                 },
             )
             try:
-                storage.sync_directory(from_path, to_path)
+                build_media_storage.sync_directory(from_path, to_path)
             except Exception:
                 # Ideally this should just be an IOError
                 # but some storage backends unfortunately throw other errors
@@ -1115,7 +1112,7 @@ def store_build_artifacts(
                 },
             )
             try:
-                storage.delete_directory(media_path)
+                build_media_storage.delete_directory(media_path)
             except Exception:
                 # Ideally this should just be an IOError
                 # but some storage backends unfortunately throw other errors
@@ -1376,26 +1373,24 @@ def _create_intersphinx_data(version, commit, build):
     if not version.is_sphinx_type:
         return
 
-    storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)()
-
     html_storage_path = version.project.get_storage_path(
         type_='html', version_slug=version.slug, include_file=False
     )
     json_storage_path = version.project.get_storage_path(
         type_='json', version_slug=version.slug, include_file=False
     )
 
-    object_file = storage.join(html_storage_path, 'objects.inv')
-    if not storage.exists(object_file):
+    object_file = build_media_storage.join(html_storage_path, 'objects.inv')
+    if not build_media_storage.exists(object_file):
         log.debug('No objects.inv, skipping intersphinx indexing.')
         return
 
-    type_file = storage.join(json_storage_path, 'readthedocs-sphinx-domain-names.json')
+    type_file = build_media_storage.join(json_storage_path, 'readthedocs-sphinx-domain-names.json')
     types = {}
     titles = {}
-    if storage.exists(type_file):
+    if build_media_storage.exists(type_file):
         try:
-            data = json.load(storage.open(type_file))
+            data = json.load(build_media_storage.open(type_file))
             types = data['types']
             titles = data['titles']
         except Exception:
@@ -1415,7 +1410,7 @@ def warn(self, msg):
             log.warning('Sphinx MockApp: %s', msg)
 
     # Re-create all objects from the new build of the version
-    object_file_url = storage.url(object_file)
+    object_file_url = build_media_storage.url(object_file)
     if object_file_url.startswith('/'):
         # Filesystem backed storage simply prepends MEDIA_URL to the path to get the URL
         # This can cause an issue if MEDIA_URL is not fully qualified
@@ -1537,15 +1532,13 @@ def _create_imported_files(*, version, commit, build, search_ranking, search_ign
     :returns: paths of changed files
     :rtype: set
     """
-    storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)()
-
     changed_files = set()
 
     # Re-create all objects from the new build of the version
     storage_path = version.project.get_storage_path(
         type_='html', version_slug=version.slug, include_file=False
     )
-    for root, __, filenames in storage.walk(storage_path):
+    for root, __, filenames in build_media_storage.walk(storage_path):
         for filename in filenames:
             if filename.endswith('.html'):
                 model_class = HTMLFile
@@ -1556,13 +1549,13 @@ def _create_imported_files(*, version, commit, build, search_ranking, search_ign
                 # For projects not behind a CDN, we don't care about non-HTML
                 continue
 
-            full_path = storage.join(root, filename)
+            full_path = build_media_storage.join(root, filename)
 
             # Generate a relative path for storage similar to os.path.relpath
             relpath = full_path.replace(storage_path, '', 1).lstrip('/')
 
             try:
-                md5 = hashlib.md5(storage.open(full_path, 'rb').read()).hexdigest()
+                md5 = hashlib.md5(build_media_storage.open(full_path, 'rb').read()).hexdigest()
             except Exception:
                 log.exception(
                     'Error while generating md5 for %s:%s:%s. Don\'t stop.',
@@ -1808,10 +1801,9 @@ def remove_build_storage_paths(paths):
 
     :param paths: list of paths in build media storage to delete
     """
-    storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)()
     for storage_path in paths:
         log.info('Removing %s from media storage', storage_path)
-        storage.delete_directory(storage_path)
+        build_media_storage.delete_directory(storage_path)
 
 
 @app.task(queue='web')

readthedocs/projects/views/public.py

@@ -13,7 +13,6 @@
 from django.conf import settings
 from django.contrib import messages
 from django.core.cache import cache
-from django.core.files.storage import get_storage_class
 from django.db.models import prefetch_related_objects
 from django.http import HttpResponse
 from django.shortcuts import get_object_or_404, redirect, render
@@ -37,6 +36,7 @@
 from readthedocs.projects.views.mixins import ProjectRelationListMixin
 from readthedocs.proxito.views.mixins import ServeDocsMixin
 from readthedocs.proxito.views.utils import _get_project_data_from_request
+from readthedocs.storage import build_media_storage
 
 from ..constants import PRIVATE
 from .base import ProjectOnboardMixin
@@ -365,15 +365,14 @@ def get(
             uip=get_client_ip(request),
         )
 
-        storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)()
         storage_path = version.project.get_storage_path(
             type_=type_,
             version_slug=version_slug,
             version_type=version.type,
         )
 
         # URL without scheme and domain to perform an NGINX internal redirect
-        url = storage.url(storage_path)
+        url = build_media_storage.url(storage_path)
         url = urlparse(url)._replace(scheme='', netloc='').geturl()
 
         return self._serve_docs(

readthedocs/proxito/tests/base.py

@@ -3,17 +3,25 @@
 
 import pytest
 import django_dynamic_fixture as fixture
+from django.conf import settings
 from django.contrib.auth.models import User
+from django.core.files.storage import get_storage_class
 from django.test import TestCase
 
 from readthedocs.projects.constants import PUBLIC
 from readthedocs.projects.models import Project, Domain
+from readthedocs.proxito.views import serve
 
 
 @pytest.mark.proxito
 class BaseDocServing(TestCase):
 
     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)()
+
         self.eric = fixture.get(User, username='eric')
         self.eric.set_password('eric')
         self.eric.save()

readthedocs/proxito/views/mixins.py

@@ -5,7 +5,6 @@
 from slugify import slugify as unicode_slugify
 
 from django.conf import settings
-from django.core.files.storage import get_storage_class
 from django.http import (
     HttpResponse,
     HttpResponseRedirect,
@@ -18,6 +17,7 @@
 from readthedocs.builds.constants import EXTERNAL, INTERNAL
 from readthedocs.core.resolver import resolve
 from readthedocs.redirects.exceptions import InfiniteRedirectException
+from readthedocs.storage import build_media_storage
 
 log = logging.getLogger(__name__)  # noqa
 
@@ -72,8 +72,7 @@ def _serve_docs_python(self, request, final_project, path):
         """
         log.debug('[Django serve] path=%s, project=%s', path, final_project.slug)
 
-        storage = get_storage_class(settings.RTD_BUILD_MEDIA_STORAGE)()
-        root_path = storage.path('')
+        root_path = build_media_storage.path('')
         # Serve from Python
         return serve(request, path, root_path)