Refactor get_storage_path* functions

[stsewd]

We were using Project.get_storage_path, which required to manually pass attributes from a version. Most of the time we already have a version object in place, so I "moved" the method to the Version class. The old method was also used to get file of a downloadable media type, I moved this to another method. Version.get_storage_path also accepts a filename, so we don't need to join the paths manually everytime we want to get the storage path from a file.

We also were setting a version_type attribute to force all storage calls to be external/internal, but we should make sure the version we are getting the file from matches the external domain it's served from, this check is already done by the unresolver. We also have tests for this already.

Project.get_storage_paths wasn't returning the paths used by PR previews, we were missing deleting those from storage when the project was deleted.

Ref #12899
Closes #10220

[read-the-docs-community]

Documentation build overview

📚 dev | 🛠️ Build #32081754 | 📁 Comparing 83411de against latest (b21b706)

  🔍 Preview build  

No files changed.

[Copilot]

Pull request overview

This PR refactors storage path generation by moving responsibility from Project.get_storage_path(...) to new Version.get_storage_path(...) / Version.get_download_storage_path(...), and updates multiple call sites accordingly (including removing the proxito version_type override behavior).

Changes:

• Remove Project.get_storage_path(...) and extend Project.get_storage_paths() to include external-version prefixes.
• Add Version.get_storage_path(...) and Version.get_download_storage_path(...), and migrate storage path usage across build/search/embed/proxito code.
• Update unit tests to use the new Version-based storage path APIs.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 4 comments.

Show a summary per file

File	Description
readthedocs/search/parsers.py	Use Version.get_storage_path for HTML page reads during indexing.
readthedocs/rtd_tests/tests/test_project.py	Update storage-path tests to target Version methods and downloadable artifacts API.
readthedocs/proxito/views/serve.py	Remove version_type forcing and switch custom 404 storage path building to Version.get_storage_path.
readthedocs/proxito/views/mixins.py	Replace project-level storage path building with Version-based storage paths.
readthedocs/projects/tasks/search.py	Use Version.get_storage_path as the root for walking HTML files.
readthedocs/projects/tasks/builds.py	Use Version.get_storage_path for artifact sync and cleanup destinations.
readthedocs/projects/models.py	Remove Project.get_storage_path and adjust get_storage_paths to include external prefixes.
readthedocs/filetreediff/init.py	Use Version.get_storage_path for manifest read/write paths.
readthedocs/embed/v3/views.py	Use Version.get_storage_path to read embedded content from storage.
readthedocs/builds/models.py	Introduce Version.get_storage_path / get_download_storage_path implementations.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[stsewd]

@copilot make sure we have tests that cover the cases where a user tries to serve resources from internal versions from an external domain and the other way around as well. Check the same for downloads.

[Copilot]

@copilot make sure we have tests that cover the cases where a user tries to serve resources from internal versions from an external domain and the other way around as well. Check the same for download...

The cross-domain tests were already present, but there was a critical bug: two methods in TestDocServingBackends shared the name test_download_files_from_external_version (lines 632 and 666). Python silently overwrites the first with the second, so the test covering "external version downloaded from main domain → 404" was dead code and never ran.

Fixed in commit 83411de:

• Renamed the dead test to test_download_external_version_on_main_domain (external version slug on main domain → 404)
• Renamed the misleadingly named test_download_files_from_external_version_from_main_domain to test_download_internal_version_on_external_domain (it was testing an internal version slug from an external domain, not the other way around)

All 4 cross-domain scenarios now have active, clearly-named tests:

• Serve: test_serve_external_version_on_main_domain and test_serve_non_external_version_on_external_domain
• Download: test_download_external_version_on_main_domain and test_download_internal_version_on_external_domain

[agjohnson]

This looks great, the pattern is much nicer this way. I'm guessing the AWS policy code can use these pretty easily now as well.

[agjohnson]

How are these chunks able to be removed? I wasn't able to figure this out looking over this, I didn't see where we moved the logic to test the unresolved_domain.

[stsewd]

The unresolver already makes sure that only external versions are resolved when working on an external domain.

readthedocs.org/readthedocs/core/unresolver.py

Lines 337 to 338 in 50e8d78

	manager = EXTERNAL if external_version_slug else INTERNAL
	version = project.versions(manager=manager).filter(slug=version_slug).first()

We also have tests in place to verify this behavior #12906 (comment)

[agjohnson]

Ah gotcha, thanks. Yeah figured it was well tested, I just wasn't seeing it