From 1ac3b46417c5f04cc6a27bd5e7a527bde65f66bf Mon Sep 17 00:00:00 2001 From: Santos Gallegos Date: Thu, 26 Oct 2023 15:41:23 -0500 Subject: [PATCH] Resolver: refactor (#10813) Simplify a couple of things, and we can remove the overrides from .com after this. - _get_canonical_project and _get_canonical_project_data are basically the same, the former tries to recursively resolve the project for cases that we don't really support, and the second just checks for the cases that we do support and returns the relationship in case of a subproject. So I just renamed _get_canonical_project_data to _get_canonical_project. - We were passing the project slug when resolving the path (this was a residual from where we were allowing `USE_SUBDOMAIN=False`). - Resolving is now split into two steps: resolving the domain, and resolving the path. - We were using `require_https` for .com only, this was since on .com we were using the https attribute to track the progress of a custom domain, this is no longer the case, all custom domains on .com are https. - `_use_custom_domain` is the same as `_use_cname`. - Two more methods to resolve a path were added, they are basically the same as `resolve`, but they work on the object itself, instead of passing each part separately. This results in fewer queries in case the version object is already in memory. --- readthedocs/core/resolver.py | 199 +++++++++---------- readthedocs/rtd_tests/tests/test_resolver.py | 118 +++++++++-- 2 files changed, 187 insertions(+), 130 deletions(-) diff --git a/readthedocs/core/resolver.py b/readthedocs/core/resolver.py index dd90dc2282c..8b6b6bc6d4c 100644 --- a/readthedocs/core/resolver.py +++ b/readthedocs/core/resolver.py @@ -4,8 +4,7 @@ import structlog from django.conf import settings -from readthedocs.builds.constants import EXTERNAL -from readthedocs.core.utils.extend import SettingsOverrideObject +from readthedocs.builds.constants import EXTERNAL, INTERNAL from readthedocs.core.utils.url import unsafe_join_url_path from readthedocs.subscriptions.constants import TYPE_CNAME from readthedocs.subscriptions.products import get_feature @@ -13,7 +12,7 @@ log = structlog.get_logger(__name__) -class ResolverBase: +class Resolver: """ Read the Docs URL Resolver. @@ -56,7 +55,6 @@ class ResolverBase: def base_resolve_path( self, - project_slug, filename, version_slug=None, language=None, @@ -91,7 +89,6 @@ def base_resolve_path( subproject_alias = project_relationship.alias if project_relationship else "" return path.format( - project=project_slug, filename=filename, version=version_slug, language=language, @@ -112,7 +109,7 @@ def resolve_path( filename = self._fix_filename(filename) - parent_project, project_relationship = self._get_canonical_project_data(project) + parent_project, project_relationship = self._get_canonical_project(project) single_version = bool(project.single_version or single_version) # If the project is a subproject, we use the custom prefix @@ -125,7 +122,6 @@ def resolve_path( custom_prefix = parent_project.custom_prefix return self.base_resolve_path( - project_slug=parent_project.slug, filename=filename, version_slug=version_slug, language=language, @@ -134,25 +130,94 @@ def resolve_path( custom_prefix=custom_prefix, ) - def resolve_domain(self, project, use_canonical_domain=True): + def resolve_version(self, project, version=None, filename="/"): + """ + Get the URL for a specific version of a project. + + If no version is given, the default version is used. + + Use this instead of ``resolve`` if you have the version object already. + """ + if not version: + default_version_slug = project.get_default_version() + version = project.versions(manager=INTERNAL).get(slug=default_version_slug) + + domain, use_https = self._get_project_domain( + project, + external_version_slug=version.slug if version.is_external else None, + ) + path = self.resolve_path( + project=project, + filename=filename, + version_slug=version.slug, + language=project.language, + single_version=project.single_version, + ) + protocol = "https" if use_https else "http" + return urlunparse((protocol, domain, path, "", "", "")) + + def resolve_project(self, project, filename="/"): + """ + Get the URL for a project. + + This is the URL where the project is served from, + it doesn't include the version or language. + + Useful to link to a known filename in the project. + """ + domain, use_https = self._get_project_domain(project) + protocol = "https" if use_https else "http" + return urlunparse((protocol, domain, filename, "", "", "")) + + def _get_project_domain( + self, project, external_version_slug=None, use_canonical_domain=True + ): """ Get the domain from where the documentation of ``project`` is served from. :param project: Project object :param bool use_canonical_domain: If `True` use its canonical custom domain if available. + :returns: Tuple of ``(domain, use_https)``. """ - canonical_project = self._get_canonical_project(project) - if use_canonical_domain and self._use_cname(canonical_project): - domain = canonical_project.get_canonical_custom_domain() - if domain: - return domain.domain + use_https = settings.PUBLIC_DOMAIN_USES_HTTPS + canonical_project, _ = self._get_canonical_project(project) + domain = self._get_project_subdomain(canonical_project) + if external_version_slug: + domain = self._get_external_subdomain( + canonical_project, external_version_slug + ) + elif use_canonical_domain and self._use_cname(canonical_project): + domain_object = canonical_project.get_canonical_custom_domain() + if domain_object: + use_https = domain_object.https + domain = domain_object.domain - return self._get_project_subdomain(canonical_project) + return domain, use_https + + def get_domain(self, project, use_canonical_domain=True): + domain, use_https = self._get_project_domain( + project, use_canonical_domain=use_canonical_domain + ) + protocol = "https" if use_https else "http" + return urlunparse((protocol, domain, "", "", "", "")) + + def get_domain_without_protocol(self, project, use_canonical_domain=True): + """ + Get the domain from where the documentation of ``project`` is served from. + + This doesn't include the protocol. + + :param project: Project object + :param bool use_canonical_domain: If `True` use its canonical custom domain if available. + """ + domain, _ = self._get_project_domain( + project, use_canonical_domain=use_canonical_domain + ) + return domain def resolve( self, project, - require_https=False, filename="", query_params="", external=None, @@ -165,36 +230,11 @@ def resolve( if external is None: external = self._is_external(project, version_slug) - canonical_project = self._get_canonical_project(project) - custom_domain = canonical_project.get_canonical_custom_domain() - use_custom_domain = self._use_custom_domain(custom_domain) - - if external: - domain = self._get_external_subdomain(canonical_project, version_slug) - elif use_custom_domain: - domain = custom_domain.domain - else: - domain = self._get_project_subdomain(canonical_project) - - use_https_protocol = any( - [ - # Rely on the ``Domain.https`` field - use_custom_domain and custom_domain.https, - # or force it if specified - require_https, - # or fallback to settings - settings.PUBLIC_DOMAIN_USES_HTTPS - and settings.PUBLIC_DOMAIN - and any( - [ - settings.PUBLIC_DOMAIN in domain, - settings.RTD_EXTERNAL_VERSION_DOMAIN in domain, - ] - ), - ] + domain, use_https = self._get_project_domain( + project, + external_version_slug=version_slug if external else None, ) - protocol = "https" if use_https_protocol else "http" - + protocol = "https" if use_https else "http" path = self.resolve_path(project, filename=filename, **kwargs) return urlunparse((protocol, domain, path, "", query_params, "")) @@ -211,26 +251,14 @@ def get_subproject_url_prefix(self, project, external_version_slug=None): :param project: Project object to get the root URL from :param external_version_slug: If given, resolve using the external version domain. """ - canonical_project = self._get_canonical_project(project) - use_custom_domain = self._use_cname(canonical_project) - custom_domain = canonical_project.get_canonical_custom_domain() - if external_version_slug: - domain = self._get_external_subdomain( - canonical_project, external_version_slug - ) - use_https = settings.PUBLIC_DOMAIN_USES_HTTPS - elif use_custom_domain and custom_domain: - domain = custom_domain.domain - use_https = custom_domain.https - else: - domain = self._get_project_subdomain(canonical_project) - use_https = settings.PUBLIC_DOMAIN_USES_HTTPS - + domain, use_https = self._get_project_domain( + project, external_version_slug=external_version_slug + ) protocol = "https" if use_https else "http" path = project.subproject_prefix return urlunparse((protocol, domain, path, "", "", "")) - def _get_canonical_project_data(self, project): + def _get_canonical_project(self, project): """ Get the parent project and subproject relationship from the canonical project of `project`. @@ -287,38 +315,7 @@ def _get_canonical_project_data(self, project): if relationship: parent_project = relationship.parent - return (parent_project, relationship) - - def _get_canonical_project(self, project, projects=None): - """ - Recursively get canonical project for subproject or translations. - - We need to recursively search here as a nested translations inside - subprojects, and vice versa, are supported. - - :type project: Project - :type projects: List of projects for iteration - :rtype: Project - """ - # Track what projects have already been traversed to avoid infinite - # recursion. We can't determine a root project well here, so you get - # what you get if you have configured your project in a strange manner - if projects is None: - projects = {project} - else: - projects.add(project) - - next_project = None - if project.main_language_project: - next_project = project.main_language_project - else: - relation = project.parent_relationship - if relation: - next_project = relation.parent - - if next_project and next_project not in projects: - return self._get_canonical_project(next_project, projects) - return project + return parent_project, relationship def _get_external_subdomain(self, project, version_slug): """Determine domain for an external version.""" @@ -351,28 +348,12 @@ def _fix_filename(self, filename): filename = filename.lstrip("/") return filename - def _use_custom_domain(self, custom_domain): - """ - Make decision about whether to use a custom domain to serve docs. - - Always use the custom domain if it exists. - - :param custom_domain: Domain instance or ``None`` - :type custom_domain: readthedocs.projects.models.Domain - """ - return custom_domain is not None - def _use_cname(self, project): """Test if to allow direct serving for project on CNAME.""" return bool(get_feature(project, feature_type=TYPE_CNAME)) -class Resolver(SettingsOverrideObject): - _default_class = ResolverBase - _override_setting = "RESOLVER_CLASS" - - resolver = Resolver() resolve_path = resolver.resolve_path -resolve_domain = resolver.resolve_domain +resolve_domain = resolver.get_domain_without_protocol resolve = resolver.resolve diff --git a/readthedocs/rtd_tests/tests/test_resolver.py b/readthedocs/rtd_tests/tests/test_resolver.py index 4ff8be50442..09e8087b78e 100644 --- a/readthedocs/rtd_tests/tests/test_resolver.py +++ b/readthedocs/rtd_tests/tests/test_resolver.py @@ -1,9 +1,16 @@ import django_dynamic_fixture as fixture -import pytest from django.test import TestCase, override_settings +from django_dynamic_fixture import get from readthedocs.builds.constants import EXTERNAL -from readthedocs.core.resolver import Resolver, resolve, resolve_domain, resolve_path +from readthedocs.builds.models import Version +from readthedocs.core.resolver import ( + Resolver, + resolve, + resolve_domain, + resolve_path, + resolver, +) from readthedocs.projects.constants import PRIVATE from readthedocs.projects.models import Domain, Project, ProjectRelationship from readthedocs.rtd_tests.utils import create_user @@ -210,7 +217,7 @@ def test_project_with_same_translation_and_main_language(self): # This tests that we aren't going to re-recurse back to resolving proj1 r = Resolver() - self.assertEqual(r._get_canonical_project(proj1), proj2) + self.assertEqual(r._get_canonical_project(proj1), (proj2, None)) def test_project_with_same_superproject_and_translation(self): proj1 = fixture.get(Project, main_language_project=None) @@ -234,7 +241,7 @@ def test_project_with_same_superproject_and_translation(self): # This tests that we aren't going to re-recurse back to resolving proj1 r = Resolver() - self.assertEqual(r._get_canonical_project(proj1), proj2) + self.assertEqual(r._get_canonical_project(proj1), (proj2, None)) def test_project_with_same_grandchild_project(self): # Note: we don't disallow this, but we also don't support this in our @@ -268,11 +275,12 @@ def test_project_with_same_grandchild_project(self): # This tests that we aren't going to re-recurse back to resolving proj1 r = Resolver() - self.assertEqual(r._get_canonical_project(proj1), proj3) + self.assertEqual( + r._get_canonical_project(proj1), (proj2, proj1.parent_relationship) + ) class ResolverDomainTests(ResolverBase): - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_domain_resolver(self): url = resolve_domain(project=self.pip) self.assertEqual(url, "pip.readthedocs.org") @@ -306,7 +314,6 @@ def test_domain_resolver_subproject(self): url = resolve_domain(project=self.subproject, use_canonical_domain=False) self.assertEqual(url, "pip.readthedocs.io") - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_domain_resolver_subproject_itself(self): """ Test inconsistent project/subproject relationship. @@ -334,7 +341,6 @@ def test_domain_resolver_translation(self): url = resolve_domain(project=self.translation, use_canonical_domain=False) self.assertEqual(url, "pip.readthedocs.io") - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_domain_resolver_translation_itself(self): """ Test inconsistent project/translation relationship. @@ -381,12 +387,10 @@ def test_domain_external(self): class ResolverTests(ResolverBase): - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_resolver(self): url = resolve(project=self.pip) self.assertEqual(url, "http://pip.readthedocs.org/en/latest/") - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_resolver_domain(self): self.domain = fixture.get( Domain, @@ -398,7 +402,6 @@ def test_resolver_domain(self): url = resolve(project=self.pip) self.assertEqual(url, "http://docs.foobar.com/en/latest/") - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_resolver_domain_https(self): self.domain = fixture.get( Domain, @@ -410,7 +413,6 @@ def test_resolver_domain_https(self): url = resolve(project=self.pip) self.assertEqual(url, "https://docs.foobar.com/en/latest/") - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_resolver_subproject(self): url = resolve(project=self.subproject) self.assertEqual( @@ -418,12 +420,10 @@ def test_resolver_subproject(self): "http://pip.readthedocs.org/projects/sub/ja/latest/", ) - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_resolver_translation(self): url = resolve(project=self.translation) self.assertEqual(url, "http://pip.readthedocs.org/ja/latest/") - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_resolver_nested_translation_of_a_subproject(self): """The project is a translation, and the main translation is a subproject of a project.""" translation = fixture.get( @@ -440,8 +440,6 @@ def test_resolver_nested_translation_of_a_subproject(self): "http://pip.readthedocs.org/projects/sub/es/latest/", ) - @pytest.mark.xfail(reason="We do not support this for now", strict=True) - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_resolver_nested_subproject_of_a_translation(self): """The project is a subproject, and the superproject is a translation of a project.""" project = fixture.get( @@ -473,13 +471,11 @@ def test_resolver_nested_subproject_of_a_translation(self): url, "http://docs-es.readthedocs.org/projects/api-es/es/latest/" ) - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_resolver_single_version(self): self.pip.single_version = True url = resolve(project=self.pip) self.assertEqual(url, "http://pip.readthedocs.org/") - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_resolver_subproject_alias(self): relation = self.pip.subprojects.first() relation.alias = "sub_alias" @@ -490,14 +486,12 @@ def test_resolver_subproject_alias(self): "http://pip.readthedocs.org/projects/sub_alias/ja/latest/", ) - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_resolver_private_project(self): self.pip.privacy_level = PRIVATE self.pip.save() url = resolve(project=self.pip) self.assertEqual(url, "http://pip.readthedocs.org/en/latest/") - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_resolver_private_project_override(self): self.pip.privacy_level = PRIVATE self.pip.save() @@ -506,7 +500,6 @@ def test_resolver_private_project_override(self): url = resolve(project=self.pip) self.assertEqual(url, "http://pip.readthedocs.org/en/latest/") - @override_settings(PRODUCTION_DOMAIN="readthedocs.org") def test_resolver_private_version_override(self): latest = self.pip.versions.first() latest.privacy_level = PRIVATE @@ -561,6 +554,75 @@ def test_resolver_domain_https(self): url = resolve(project=self.pip) self.assertEqual(url, "http://pip.readthedocs.io/en/latest/") + def test_resolve_project_object(self): + url = resolver.resolve_project(self.pip) + self.assertEqual(url, "http://pip.readthedocs.org/") + + url = resolver.resolve_project(self.pip, filename="index.html") + self.assertEqual(url, "http://pip.readthedocs.org/index.html") + + url = resolver.resolve_project(self.pip, filename="/_/api/v2/footer_html") + self.assertEqual(url, "http://pip.readthedocs.org/_/api/v2/footer_html") + + def test_resolve_subproject_object(self): + url = resolver.resolve_project(self.subproject) + self.assertEqual(url, "http://pip.readthedocs.org/") + + url = resolver.resolve_project(self.subproject, filename="index.html") + self.assertEqual(url, "http://pip.readthedocs.org/index.html") + + url = resolver.resolve_project( + self.subproject, filename="/_/api/v2/footer_html" + ) + self.assertEqual(url, "http://pip.readthedocs.org/_/api/v2/footer_html") + + def test_resolve_translation_object(self): + url = resolver.resolve_project(self.translation) + self.assertEqual(url, "http://pip.readthedocs.org/") + + url = resolver.resolve_project(self.translation, filename="index.html") + self.assertEqual(url, "http://pip.readthedocs.org/index.html") + + url = resolver.resolve_project( + self.translation, filename="/_/api/v2/footer_html" + ) + self.assertEqual(url, "http://pip.readthedocs.org/_/api/v2/footer_html") + + def test_resolve_version_object(self): + url = resolver.resolve_version(self.pip) + self.assertEqual(url, "http://pip.readthedocs.org/en/latest/") + + url = resolver.resolve_version(self.pip, version=self.version) + self.assertEqual(url, "http://pip.readthedocs.org/en/latest/") + + version = get(Version, project=self.pip, slug="v2") + url = resolver.resolve_version(self.pip, version=version) + self.assertEqual(url, "http://pip.readthedocs.org/en/v2/") + + def test_resolve_version_from_subproject(self): + url = resolver.resolve_version(self.subproject) + self.assertEqual(url, "http://pip.readthedocs.org/projects/sub/ja/latest/") + + version = self.subproject.versions.first() + url = resolver.resolve_version(self.subproject, version=version) + self.assertEqual(url, "http://pip.readthedocs.org/projects/sub/ja/latest/") + + version = get(Version, project=self.subproject, slug="v2") + url = resolver.resolve_version(self.subproject, version=version) + self.assertEqual(url, "http://pip.readthedocs.org/projects/sub/ja/v2/") + + def test_resolve_version_from_translation(self): + url = resolver.resolve_version(self.translation) + self.assertEqual(url, "http://pip.readthedocs.org/ja/latest/") + + version = self.translation.versions.first() + url = resolver.resolve_version(self.translation, version=version) + self.assertEqual(url, "http://pip.readthedocs.org/ja/latest/") + + version = get(Version, project=self.translation, slug="v2") + url = resolver.resolve_version(self.translation, version=version) + self.assertEqual(url, "http://pip.readthedocs.org/ja/v2/") + class ResolverAltSetUp: def setUp(self): @@ -572,6 +634,7 @@ def setUp(self): users=[self.owner], main_language_project=None, ) + self.version = self.pip.versions.first() self.seed = fixture.get( Project, slug="sub", @@ -957,3 +1020,16 @@ def test_custom_prefix_in_subproject_and_custom_prefix_in_superproject(self): self.assertEqual( url, "http://pip.readthedocs.io/s/sub/prefix/es/latest/api/index.html" ) + + def test_get_project_domain(self): + domain = resolver.get_domain(self.pip) + self.assertEqual(domain, "http://pip.readthedocs.io") + + domain = resolver.get_domain(self.subproject) + self.assertEqual(domain, "http://pip.readthedocs.io") + + domain = resolver.get_domain(self.translation) + self.assertEqual(domain, "http://pip.readthedocs.io") + + domain = resolver.get_domain(self.subproject_translation) + self.assertEqual(domain, "http://pip.readthedocs.io")