Skip to content
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.

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

Add an unresolver similar to our resolver #6944

Merged
merged 2 commits into from
May 5, 2020
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
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
2 changes: 1 addition & 1 deletion common
Submodule common updated 0 files
68 changes: 68 additions & 0 deletions readthedocs/core/unresolver.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,68 @@
import logging
from urllib.parse import urlparse
from collections import namedtuple

from django.urls import resolve as url_resolve
from django.test.client import RequestFactory

from readthedocs.core.utils.extend import SettingsOverrideObject
from readthedocs.proxito.middleware import map_host_to_project_slug
from readthedocs.proxito.views.utils import _get_project_data_from_request
Copy link
Contributor

Choose a reason for hiding this comment

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

If we're using it outside of proxito, it probably shouldn't be private (underscore prefixed).

from readthedocs.proxito.views.mixins import ServeDocsMixin

log = logging.getLogger(__name__)

UnresolvedObject = namedtuple(
'Unresolved', 'project, language_slug, version_slug, filename, fragment')


class UnresolverBase:

def unresolve(self, uri):
"""
Turn a URL into the component parts that our views would use to process them.

This is useful for lots of places,
like where we want to figure out exactly what file a URL maps to.
"""
parsed = urlparse(uri)
domain = parsed.netloc.split(':', 1)[0]
path = parsed.path

# TODO: Make this not depend on the request object,
# but instead move all this logic here working on strings.
request = RequestFactory().get(path=path, HTTP_HOST=domain)
project_slug = request.host_project_slug = map_host_to_project_slug(request)

# Handle returning a response
if hasattr(project_slug, 'status_code'):
return UnresolvedObject(None, None, None, None, None)
Copy link
Member

Choose a reason for hiding this comment

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

Why not just return None instead?

Copy link
Member

Choose a reason for hiding this comment

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

It will be easier to perform checks by checking if parts than if parts.<somefield> --looks more intuitive to me, since <somefield> could be None and be a valid object as well (fragment, for example)

Copy link
Member Author

Choose a reason for hiding this comment

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

Makes sense 👍


_, __, kwargs = url_resolve(
path,
urlconf='readthedocs.proxito.urls',
)

mixin = ServeDocsMixin()
version_slug = mixin.get_version_from_host(request, kwargs.get('version_slug'))

final_project, lang_slug, version_slug, filename = _get_project_data_from_request( # noqa
request,
project_slug=project_slug,
subproject_slug=kwargs.get('subproject_slug'),
lang_slug=kwargs.get('lang_slug'),
version_slug=version_slug,
filename=kwargs.get('filename', ''),
)

log.info('Unresolved: %s', locals())
return UnresolvedObject(final_project, lang_slug, version_slug, filename, parsed.fragment)


class Unresolver(SettingsOverrideObject):

_default_class = UnresolverBase


unresolver = Unresolver()
unresolve = unresolver.unresolve
85 changes: 85 additions & 0 deletions readthedocs/rtd_tests/tests/test_unresolver.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,85 @@
from django.test import override_settings
import django_dynamic_fixture as fixture
import pytest

from readthedocs.rtd_tests.tests.test_resolver import ResolverBase
from readthedocs.core.unresolver import unresolve
from readthedocs.projects.models import Domain


@override_settings(
PUBLIC_DOMAIN='readthedocs.io',
RTD_EXTERNAL_VERSION_DOMAIN='dev.readthedocs.build',
Copy link
Member

Choose a reason for hiding this comment

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

nit: I think it's better if we use the real production value. It's easier to visualize and compare.

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 the dev value, which I think makes sense for testing.

)
@pytest.mark.proxito
Copy link
Member

Choose a reason for hiding this comment

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

I don't think proxito settings are required for this. We are just testing an object.

Copy link
Member Author

Choose a reason for hiding this comment

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

They import the proxito code tho, at least currently

class UnResolverTests(ResolverBase):

def test_unresolver(self):
parts = unresolve('http://pip.readthedocs.io/en/latest/foo.html#fragment')
self.assertEqual(parts.project.slug, 'pip')
self.assertEqual(parts.language_slug, 'en')
self.assertEqual(parts.version_slug, 'latest')
self.assertEqual(parts.filename, 'foo.html')
self.assertEqual(parts.fragment, 'fragment')
Comment on lines +18 to +23
Copy link
Member

Choose a reason for hiding this comment

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

I love these tests! Super easy to follow! 💯 More "unit test" that our current tests that needs to call Django internals


def test_unresolver_subproject(self):
parts = unresolve('http://pip.readthedocs.io/projects/sub/ja/latest/foo.html')
self.assertEqual(parts.project.slug, 'sub')
self.assertEqual(parts.language_slug, 'ja')
self.assertEqual(parts.version_slug, 'latest')
self.assertEqual(parts.filename, 'foo.html')

def test_unresolver_translation(self):
parts = unresolve('http://pip.readthedocs.io/ja/latest/foo.html')
self.assertEqual(parts.project.slug, 'trans')
self.assertEqual(parts.language_slug, 'ja')
self.assertEqual(parts.version_slug, 'latest')
self.assertEqual(parts.filename, 'foo.html')

def test_unresolver_domain(self):
self.domain = fixture.get(
Domain,
domain='docs.foobar.com',
project=self.pip,
canonical=True,
)
parts = unresolve('http://docs.foobar.com/en/latest/')
self.assertEqual(parts.project.slug, 'pip')
self.assertEqual(parts.language_slug, 'en')
self.assertEqual(parts.version_slug, 'latest')
self.assertEqual(parts.filename, '')

def test_unresolver_single_version(self):
self.pip.single_version = True
Copy link
Member

Choose a reason for hiding this comment

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

Isn't it required to .save the project here? 🤔

parts = unresolve('http://pip.readthedocs.io/')
self.assertEqual(parts.project.slug, 'pip')
self.assertEqual(parts.language_slug, None)
self.assertEqual(parts.version_slug, None)
self.assertEqual(parts.filename, '')

def test_unresolver_subproject_alias(self):
relation = self.pip.subprojects.first()
relation.alias = 'sub_alias'
relation.save()
parts = unresolve('http://pip.readthedocs.io/projects/sub_alias/ja/latest/')
self.assertEqual(parts.project.slug, 'sub')
self.assertEqual(parts.language_slug, 'ja')
self.assertEqual(parts.version_slug, 'latest')
self.assertEqual(parts.filename, '')

def test_unresolver_external_version(self):
ver = self.pip.versions.first()
ver.type = 'external'
ver.slug = '10'
parts = unresolve('http://pip--10.dev.readthedocs.build/en/10/')
self.assertEqual(parts.project.slug, 'pip')
self.assertEqual(parts.language_slug, 'en')
self.assertEqual(parts.version_slug, '10')
self.assertEqual(parts.filename, '')

def test_unresolver_unknown_host(self):
parts = unresolve('http://random.stuff.com/en/latest/')
self.assertEqual(parts.project, None)
self.assertEqual(parts.language_slug, None)
self.assertEqual(parts.version_slug, None)
self.assertEqual(parts.filename, None)
7 changes: 5 additions & 2 deletions readthedocs/settings/docker_compose.py
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,11 @@ class DockerBaseSettings(CommunityDevSettings):
if ips and not HOSTIP:
HOSTIP = ips[0][:-1] + "1"

# Turn this on to test ads
USE_PROMOS = False
ADSERVER_API_BASE = f'http://{HOSTIP}:5000'

# Create a Token for an admin User and set it here.
ADSERVER_API_KEY = None

ADSERVER_API_TIMEOUT = 2 # seconds - Docker for Mac is very slow

# Enable auto syncing elasticsearch documents
Expand Down Expand Up @@ -136,3 +136,6 @@ def DATABASES(self): # noqa
# Remove the checks on the number of fields being submitted
# This limit is mostly hit on large forms in the Django admin
DATA_UPLOAD_MAX_NUMBER_FIELDS = None

# This allows us to have CORS work well in dev
CORS_ORIGIN_ALLOW_ALL = True