Skip to content

Resolver: allow to ignore custom domains #9089

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 3 commits into from
Apr 26, 2022
Merged

Resolver: allow to ignore custom domains #9089

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
2f61471
Resolver: allow to ignore custom domains
stsewd Apr 7, 2022
cba64e2
Merge branch 'main' into resolver-allow-ignore-custom-domain
stsewd Apr 26, 2022
a241b36
Rename parameter
stsewd Apr 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 17 additions & 6 deletions readthedocs/core/resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
"""URL resolver for documentation."""

import structlog
from urllib.parse import urlunparse

import structlog
from django.conf import settings

from readthedocs.core.utils.extend import SettingsOverrideObject
from readthedocs.builds.constants import EXTERNAL
from readthedocs.core.utils.extend import SettingsOverrideObject

log = structlog.get_logger(__name__)

Expand Down Expand Up @@ -152,11 +152,18 @@ def resolve_path(
urlconf=urlconf or project.urlconf,
)

def resolve_domain(self, project):
def resolve_domain(self, project, use_custom_domain=True):
"""
Get the domain from where the documentation of ``project`` is served from.

:param project: Project object
:param bool use_custom_domain: If `True` use its canonical custom domain if available.
"""
canonical_project = self._get_canonical_project(project)
domain = canonical_project.get_canonical_custom_domain()
if domain:
return domain.domain
if use_custom_domain and self._use_cname(canonical_project):
domain = canonical_project.get_canonical_custom_domain()
if domain:
return domain.domain

if self._use_subdomain():
return self._get_project_subdomain(canonical_project)
Expand Down Expand Up @@ -345,6 +352,10 @@ def _use_subdomain(self):
"""Make decision about whether to use a subdomain to serve docs."""
return settings.USE_SUBDOMAIN and settings.PUBLIC_DOMAIN is not None

def _use_cname(self, project):
"""Test if to allow direct serving for project on CNAME."""
return True


class Resolver(SettingsOverrideObject):

Expand Down
4 changes: 2 additions & 2 deletions readthedocs/projects/models.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -731,9 +731,9 @@ def alias(self):
if self.is_subproject:
return self.superprojects.first().alias

def subdomain(self):
def subdomain(self, use_custom_domain=True):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually want use_canonical_domain here? Seems like that is the actual case where the domain would be changed, and is clearer in cases of multiple custom domains?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we actually want use_canonical_domain here? Seems like that is the actual case where the domain would be changed, and is clearer in cases of multiple custom domains?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure I understood what you meant p: do you mean changing the parameter to something else? or not exposing this "feature" on that method and instead manually call resolve_domain?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, changing the name of the kwarg.

"""Get project subdomain from resolver."""
return resolve_domain(self)
return resolve_domain(self, use_custom_domain=use_custom_domain)

def get_downloads(self):
downloads = {}
Expand Down
40 changes: 27 additions & 13 deletions readthedocs/rtd_tests/tests/test_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,16 +1,9 @@
from unittest import mock

import django_dynamic_fixture as fixture
import pytest
from django.test import TestCase, override_settings

from readthedocs.builds.constants import EXTERNAL
from readthedocs.core.resolver import (
Resolver,
resolve,
resolve_domain,
resolve_path,
)
from readthedocs.core.resolver import Resolver, resolve, resolve_domain, resolve_path
from readthedocs.projects.constants import PRIVATE
from readthedocs.projects.models import Domain, Project, ProjectRelationship
from readthedocs.rtd_tests.utils import create_user
Expand Down Expand Up @@ -359,7 +352,10 @@ def test_domain_resolver(self):
url = resolve_domain(project=self.pip)
self.assertEqual(url, 'pip.readthedocs.org')

@override_settings(PRODUCTION_DOMAIN='readthedocs.org')
@override_settings(
PRODUCTION_DOMAIN="readthedocs.org",
PUBLIC_DOMAIN="readthedocs.io",
)
def test_domain_resolver_with_domain_object(self):
self.domain = fixture.get(
Domain,
Expand All @@ -374,14 +370,23 @@ def test_domain_resolver_with_domain_object(self):
url = resolve_domain(project=self.pip)
self.assertEqual(url, 'docs.foobar.com')

@override_settings(PRODUCTION_DOMAIN='readthedocs.org')
url = resolve_domain(project=self.pip, use_custom_domain=False)
self.assertEqual(url, "pip.readthedocs.io")
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is probably the main test, since the other ones should return the same response.


@override_settings(
PRODUCTION_DOMAIN="readthedocs.org",
PUBLIC_DOMAIN="readthedocs.io",
)
def test_domain_resolver_subproject(self):
with override_settings(USE_SUBDOMAIN=False):
url = resolve_domain(project=self.subproject)
self.assertEqual(url, 'readthedocs.org')
with override_settings(USE_SUBDOMAIN=True):
url = resolve_domain(project=self.subproject)
self.assertEqual(url, 'pip.readthedocs.org')
self.assertEqual(url, "pip.readthedocs.io")

url = resolve_domain(project=self.subproject, use_custom_domain=False)
self.assertEqual(url, "pip.readthedocs.io")

@override_settings(PRODUCTION_DOMAIN='readthedocs.org')
def test_domain_resolver_subproject_itself(self):
Expand All @@ -404,14 +409,20 @@ def test_domain_resolver_subproject_itself(self):
url = resolve_domain(project=self.pip)
self.assertEqual(url, 'pip.readthedocs.org')

@override_settings(PRODUCTION_DOMAIN='readthedocs.org')
@override_settings(
PRODUCTION_DOMAIN="readthedocs.org",
PUBLIC_DOMAIN="readthedocs.io",
)
def test_domain_resolver_translation(self):
with override_settings(USE_SUBDOMAIN=False):
url = resolve_domain(project=self.translation)
self.assertEqual(url, 'readthedocs.org')
with override_settings(USE_SUBDOMAIN=True):
url = resolve_domain(project=self.translation)
self.assertEqual(url, 'pip.readthedocs.org')
self.assertEqual(url, "pip.readthedocs.io")

url = resolve_domain(project=self.translation, use_custom_domain=False)
self.assertEqual(url, "pip.readthedocs.io")

@override_settings(PRODUCTION_DOMAIN='readthedocs.org')
def test_domain_resolver_translation_itself(self):
Expand Down Expand Up @@ -444,6 +455,9 @@ def test_domain_public(self):
self.assertEqual(url, 'readthedocs.org')
with override_settings(USE_SUBDOMAIN=True):
url = resolve_domain(project=self.translation)
self.assertEqual(url, "pip.public.readthedocs.org")

url = resolve_domain(project=self.translation, use_custom_domain=False)
self.assertEqual(url, 'pip.public.readthedocs.org')

@override_settings(
Expand Down