Build: rename PDF/ePUB filename to valid one automatically (#11198)

* Build: rename PDF/ePUB filename to valid one automatically

Currently, we _require_ the user to save the PDF or ePUB file with a pretty
specific filename: `{project.slug}-{version.slug}.{extension}`. This has caused
a lot of confusions to users.

Since we only support 1 file for PDF/ePUB for now, we can rename this file to
the valid filename automatically.

Closes #10873

* Rename the offline format file properly

* Initial attempt to write a test case

* Revert "Initial attempt to write a test case"

This reverts commit 76ab170.

* Test case and filename fix

* Remove the conditional that's not needed

* Assert the path is inside OUTPUT dir before calling `shutil.move`

* Revert "Assert the path is inside OUTPUT dir before calling `shutil.move`"

This reverts commit e9a0f08.

* Assert the path is inside the DOCROOT before calling `shutil.move`

* Update readthedocs/projects/tasks/builds.py

Co-authored-by: Santos Gallegos <[email protected]>

---------

Co-authored-by: Santos Gallegos <[email protected]>

Author: humitos

readthedocs/core/utils/filesystem.py

@@ -42,6 +42,7 @@ def assert_path_is_inside_docroot(path):
         log.error(
             "Suspicious operation outside the docroot directory.",
             path_resolved=str(resolved_path),
+            docroot=settings.DOCROOT,
         )
         raise SuspiciousFileOperation(path)
 

readthedocs/projects/tasks/builds.py

@@ -5,10 +5,12 @@
 rebuilding documentation.
 """
 import os
+import shutil
 import signal
 import socket
 import subprocess
 from dataclasses import dataclass, field
+from pathlib import Path
 
 import structlog
 from celery import Task
@@ -40,6 +42,7 @@
 from readthedocs.builds.utils import memcache_lock
 from readthedocs.config.config import BuildConfigV2
 from readthedocs.config.exceptions import ConfigError
+from readthedocs.core.utils.filesystem import assert_path_is_inside_docroot
 from readthedocs.doc_builder.director import BuildDirector
 from readthedocs.doc_builder.environments import (
     DockerBuildEnvironment,
@@ -605,7 +608,8 @@ def get_valid_artifact_types(self):
             # These output format does not support multiple files yet.
             # In case multiple files are found, the upload for this format is not performed.
             if artifact_type in ARTIFACT_TYPES_WITHOUT_MULTIPLE_FILES_SUPPORT:
-                artifact_format_files = len(os.listdir(artifact_directory))
+                list_dir = os.listdir(artifact_directory)
+                artifact_format_files = len(list_dir)
                 if artifact_format_files > 1:
                     log.error(
                         "Multiple files are not supported for this format. "
@@ -626,6 +630,18 @@ def get_valid_artifact_types(self):
                         },
                     )
 
+                # Rename file as "<project_slug>-<version_slug>.<artifact_type>",
+                # which is the filename that Proxito serves for offline formats.
+                filename = list_dir[0]
+                _, extension = filename.rsplit(".")
+                path = Path(artifact_directory) / filename
+                destination = (
+                    Path(artifact_directory) / f"{self.data.project.slug}.{extension}"
+                )
+                assert_path_is_inside_docroot(path)
+                assert_path_is_inside_docroot(destination)
+                shutil.move(path, destination)
+
             # If all the conditions were met, the artifact is valid
             valid_artifacts.append(artifact_type)
 

readthedocs/projects/tests/test_build_tasks.py

@@ -1,11 +1,14 @@
 import os
 import pathlib
 import textwrap
+import uuid
+from pathlib import Path
 from unittest import mock
 
 import django_dynamic_fixture as fixture
 import pytest
 from django.conf import settings
+from django.test.utils import override_settings
 
 from readthedocs.builds.constants import (
     BUILD_STATUS_FAILURE,
@@ -260,6 +263,7 @@ def test_build_respects_formats_mkdocs(self, build_docs_class, load_yaml_config)
 
         build_docs_class.assert_called_once_with("mkdocs")  # HTML builder
 
+    @override_settings(DOCROOT="/tmp/readthedocs-tests/git-repository/")
     @mock.patch("readthedocs.doc_builder.director.load_yaml_config")
     def test_build_updates_documentation_type(self, load_yaml_config):
         assert self.version.documentation_type == "sphinx"
@@ -403,6 +407,8 @@ def test_get_env_vars(self, load_yaml_config, build_environment, config, externa
             expected_build_env_vars["PRIVATE_TOKEN"] = "a1b2c3"
         assert build_env_vars == expected_build_env_vars
 
+    @override_settings(DOCROOT="/tmp/readthedocs-tests/git-repository/")
+    @mock.patch("readthedocs.projects.tasks.builds.shutil")
     @mock.patch("readthedocs.projects.tasks.builds.index_build")
     @mock.patch("readthedocs.projects.tasks.builds.build_complete")
     @mock.patch("readthedocs.projects.tasks.builds.send_external_build_status")
@@ -417,6 +423,7 @@ def test_successful_build(
         send_external_build_status,
         build_complete,
         index_build,
+        shutilmock,
     ):
         load_yaml_config.return_value = get_build_config(
             {
@@ -433,12 +440,16 @@ def test_successful_build(
         # Create the artifact paths, so it's detected by the builder
         os.makedirs(self.project.artifact_path(version=self.version.slug, type_="html"))
         os.makedirs(self.project.artifact_path(version=self.version.slug, type_="json"))
+        filename = str(uuid.uuid4())
         for f in ("htmlzip", "epub", "pdf"):
+            extension = "zip" if f == "htmlzip" else f
             os.makedirs(self.project.artifact_path(version=self.version.slug, type_=f))
             pathlib.Path(
                 os.path.join(
                     self.project.artifact_path(version=self.version.slug, type_=f),
-                    f"{self.project.slug}.{f}",
+                    # Use a random name for the offline format.
+                    # We will automatically rename this file to filename El Proxito expects.
+                    f"{filename}.{extension}",
                 )
             ).touch()
 
@@ -452,6 +463,36 @@ def test_successful_build(
 
         self._trigger_update_docs_task()
 
+        # Offline formats were renamed to the correct filename.
+        shutilmock.move.assert_has_calls(
+            [
+                mock.call(
+                    Path(
+                        f"/tmp/readthedocs-tests/git-repository/_readthedocs/htmlzip/{filename}.zip"
+                    ),
+                    Path(
+                        f"/tmp/readthedocs-tests/git-repository/_readthedocs/htmlzip/{self.project.slug}.zip"
+                    ),
+                ),
+                mock.call(
+                    Path(
+                        f"/tmp/readthedocs-tests/git-repository/_readthedocs/pdf/{filename}.pdf"
+                    ),
+                    Path(
+                        f"/tmp/readthedocs-tests/git-repository/_readthedocs/pdf/{self.project.slug}.pdf"
+                    ),
+                ),
+                mock.call(
+                    Path(
+                        f"/tmp/readthedocs-tests/git-repository/_readthedocs/epub/{filename}.epub"
+                    ),
+                    Path(
+                        f"/tmp/readthedocs-tests/git-repository/_readthedocs/epub/{self.project.slug}.epub"
+                    ),
+                ),
+            ]
+        )
+
         # It has to be called twice, ``before_start`` and ``after_return``
         clean_build.assert_has_calls(
             [mock.call(mock.ANY), mock.call(mock.ANY)]  # the argument is an APIVersion