diff --git a/readthedocs/api/mixins.py b/readthedocs/api/mixins.py index a6eb1f76695..d4fbdcc5acd 100644 --- a/readthedocs/api/mixins.py +++ b/readthedocs/api/mixins.py @@ -17,11 +17,8 @@ class CDNCacheTagsMixin: """ Add cache tags for project and version to the response of this view. - The view inheriting this mixin should implement the - `self._get_project` and `self._get_version` methods. - - If `self._get_version` returns `None`, - only the project level tags are added. + The view inheriting this mixin can either call :py:method:`set_cache_tags` or + implement the ``self._get_project`` and ``self._get_version`` methods. You can add an extra per-project tag by overriding the `project_cache_tag` attribute. """ @@ -30,29 +27,40 @@ class CDNCacheTagsMixin: def dispatch(self, request, *args, **kwargs): response = super().dispatch(request, *args, **kwargs) - cache_tags = self._get_cache_tags() + cache_tags = getattr(self, "_cache_tags", self._get_cache_tags()) if cache_tags: add_cache_tags(response, cache_tags) return response - def _get_cache_tags(self): + def _get_cache_tags(self, project=None, version=None): """ Get cache tags for this view. + This returns an array of tag identifiers used to tag the response at CDN. + + If project and version are not passed in, these values will come from the + methods ``_get_project()`` and ``_get_version()``. + If ``_get_version()`` returns ``None``, only the project level tags are added. + + It's easier to use :py:method:`set_cache_tags` if project/version aren't + set at the instance level, or if they are passed in through a method + like ``get()``. + .. warning:: This method is run at the end of the request, so any exceptions like 404 should be caught. """ - try: - project = self._get_project() - version = self._get_version() - except Exception: - log.warning( - "Error while retrieving project or version for this view.", - exc_info=True, - ) - return [] + if project is None and version is None: + try: + project = self._get_project() + version = self._get_version() + except Exception: + log.warning( + "Error while retrieving project or version for this view.", + exc_info=True, + ) + return [] tags = [] if project: @@ -63,6 +71,19 @@ def _get_cache_tags(self): tags.append(get_cache_tag(project.slug, self.project_cache_tag)) return tags + def set_cache_tags(self, project=None, version=None): + """ + Store cache tags to be added to response. + + This method can be used if project/version do not exist on the view + instance or if they are passed into the view through a method like + ``get()``. + + The attribute methods ``_get_project()``/``_get_version()`` aren`t used + in this pattern. + """ + self._cache_tags = self._get_cache_tags(project, version) + class EmbedAPIMixin: """ diff --git a/readthedocs/projects/views/public.py b/readthedocs/projects/views/public.py index 80567f033be..0a0bbedafc8 100644 --- a/readthedocs/projects/views/public.py +++ b/readthedocs/projects/views/public.py @@ -382,12 +382,7 @@ def get( slug=version_slug, ) - # TODO don't do this, it's a leftover of trying to use CDNCacheTagsMixin - # without class level variables. See proxito.views.serve for - # other instances of this pattern to update. - # See: https://github.com/readthedocs/readthedocs.org/pull/12495 - self.project = version.project - self.version = version + self.set_cache_tags(project=version.project, version=version) return self._serve_dowload( request=request, diff --git a/readthedocs/proxito/tests/test_full.py b/readthedocs/proxito/tests/test_full.py index 02486647143..b614549530a 100644 --- a/readthedocs/proxito/tests/test_full.py +++ b/readthedocs/proxito/tests/test_full.py @@ -857,6 +857,7 @@ def test_default_robots_txt(self, storage_exists): reverse("robots_txt"), headers={"host": "project.readthedocs.io"} ) self.assertEqual(response.status_code, 200) + self.assertEqual(response.headers["Cache-Tag"], "project,project:robots.txt") expected = dedent( """ User-agent: * @@ -909,6 +910,7 @@ def test_default_robots_txt_disallow_hidden_versions(self, storage_exists): reverse("robots_txt"), headers={"host": "project.readthedocs.io"} ) self.assertEqual(response.status_code, 200) + self.assertEqual(response.headers["Cache-Tag"], "project,project:robots.txt") expected = dedent( """ User-agent: * @@ -932,6 +934,7 @@ def test_default_robots_txt_private_version(self, storage_exists): reverse("robots_txt"), headers={"host": "project.readthedocs.io"} ) self.assertEqual(response.status_code, 404) + self.assertNotIn("Cache-Tag", response.headers) def test_custom_robots_txt(self): self.project.versions.update(active=True, built=True) @@ -942,6 +945,7 @@ def test_custom_robots_txt(self): response["x-accel-redirect"], "/proxito/media/html/project/latest/robots.txt", ) + self.assertEqual(response.headers["Cache-Tag"], "project,project:latest,project:robots.txt") def test_custom_robots_txt_private_version(self): self.project.versions.update( @@ -951,6 +955,7 @@ def test_custom_robots_txt_private_version(self): reverse("robots_txt"), headers={"host": "project.readthedocs.io"} ) self.assertEqual(response.status_code, 404) + self.assertNotIn("Cache-Tag", response.headers) def test_directory_indexes(self): self.project.versions.update(active=True, built=True) diff --git a/readthedocs/proxito/views/serve.py b/readthedocs/proxito/views/serve.py index 085b714d082..b2e1faca025 100644 --- a/readthedocs/proxito/views/serve.py +++ b/readthedocs/proxito/views/serve.py @@ -640,6 +640,10 @@ def get(self, request): """ project = request.unresolved_domain.project + # Set cache tag only for project for now, for responses that don't use a + # specific version instance. + self.set_cache_tags(project=project) + if project.delisted: return render( request, @@ -690,6 +694,9 @@ def get(self, request): filename="robots.txt", check_if_exists=True, ) + # Set project and version cache tags, override project only tag as + # we want to purge this file each time the version changes too. + self.set_cache_tags(project=project, version=version) log.info("Serving custom robots.txt file.") return response except StorageFileNotFound: @@ -704,6 +711,7 @@ def get(self, request): "sitemap_url": sitemap_url, "hidden_paths": self._get_hidden_paths(project), } + return render( request, "robots.txt", @@ -720,17 +728,6 @@ def _get_hidden_paths(self, project): ] return hidden_paths - def _get_project(self): - # Method used by the CDNCacheTagsMixin class. - return self.request.unresolved_domain.project - - def _get_version(self): - # Method used by the CDNCacheTagsMixin class. - # This view isn't explicitly mapped to a version, - # but it can be when we serve a custom robots.txt file. - # TODO: refactor how we set cache tags to avoid this. - return None - class ServeRobotsTXT(SettingsOverrideObject): _default_class = ServeRobotsTXTBase @@ -877,6 +874,9 @@ def changefreqs_generator(): versions.append(element) + # Cache tag only for project, don't include version cache tag + self.set_cache_tags(project=project) + context = { "versions": versions, } @@ -887,16 +887,6 @@ def changefreqs_generator(): content_type="application/xml", ) - def _get_project(self): - # Method used by the CDNCacheTagsMixin class. - return self.request.unresolved_domain.project - - def _get_version(self): - # Method used by the CDNCacheTagsMixin class. - # This view isn't explicitly mapped to a version, - # TODO: refactor how we set cache tags to avoid this. - return None - class ServeSitemapXML(SettingsOverrideObject): _default_class = ServeSitemapXMLBase @@ -917,25 +907,20 @@ class ServeStaticFiles(CDNCacheControlMixin, CDNCacheTagsMixin, ServeDocsMixin, def get(self, request, filename): try: - return self._serve_static_file(request=request, filename=filename) + resp = self._serve_static_file(request=request, filename=filename) + # Cache tag only for project, don't include version cache tag + self.set_cache_tags(project=request.unresolved_domain.project) + return resp except InvalidPathError: raise Http404 - def _get_cache_tags(self): + def _get_cache_tags(self, project=None, version=None): """ Add an additional *global* tag. This is so we can purge all files from all projects with one single call. """ - tags = super()._get_cache_tags() + tags = super()._get_cache_tags(project=project, version=version) tags.append(self.project_cache_tag) return tags - - def _get_project(self): - # Method used by the CDNCacheTagsMixin class. - return self.request.unresolved_domain.project - - def _get_version(self): - # This view isn't attached to a version. - return None