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.

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

check for matching alias before subproject slug #2787

Merged
merged 6 commits into from
Mar 7, 2018
Merged

check for matching alias before subproject slug #2787

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
9130f38
check for matching alias before subproject slug
funkyHat Apr 10, 2017
e07be83
Merge remote-tracking branch 'origin/master' into subproject_check_al…
funkyHat Jun 29, 2017
c3d2ddc
Address PR comments
funkyHat Oct 26, 2017
a4bb8e3
use get_object_or_404 shortcut
funkyHat Jan 12, 2018
9bb0f28
revert style changes (where did they even come from?)
funkyHat Jan 18, 2018
6a66934
Merge remote-tracking branch 'origin/master' into subproject_check_al…
funkyHat Jan 18, 2018
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
20 changes: 11 additions & 9 deletions readthedocs/core/views/serve.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -57,17 +57,19 @@ def map_subproject_slug(view_func):
@wraps(view_func)
def inner_view(request, subproject=None, subproject_slug=None, *args, **kwargs):
if subproject is None and subproject_slug:
# Try to fetch by subproject alias first, otherwise we might end up
# redirected to an unrelated project.
try:
subproject = Project.objects.get(slug=subproject_slug)
except Project.DoesNotExist:
# Depends on a project passed into kwargs
rel = ProjectRelationship.objects.get(
parent=kwargs['project'],
alias=subproject_slug,
)
subproject = rel.child
except (ProjectRelationship.DoesNotExist, KeyError):
try:
# Depends on a project passed into kwargs
rel = ProjectRelationship.objects.get(
parent=kwargs['project'],
alias=subproject_slug,
)
subproject = rel.child
except (ProjectRelationship.DoesNotExist, KeyError):
subproject = Project.objects.get(slug=subproject_slug)
Copy link
Member

Choose a reason for hiding this comment

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

We could replace the whole try...except...raise block with get_object_or_404(): https://docs.djangoproject.com/en/1.9/topics/http/shortcuts/#get-object-or-404

except Project.DoesNotExist:
raise Http404
return view_func(request, subproject=subproject, *args, **kwargs)
return inner_view
Expand Down
29 changes: 29 additions & 0 deletions readthedocs/rtd_tests/tests/test_resolver.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -385,3 +385,32 @@ def test_resolver_public_domain_overrides(self):
self.assertEqual(url, 'http://docs.foobar.com/en/latest/')
url = resolve(project=self.pip, private=False)
self.assertEqual(url, 'http://docs.foobar.com/en/latest/')


class ResolverAltSetUp(object):

def setUp(self):
Copy link
Member

Choose a reason for hiding this comment

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

The use of setUpTestData would make tests faster: https://docs.djangoproject.com/en/1.9/topics/testing/tools/#django.test.TestCase.setUpTestData

Copy link
Contributor Author

@funkyHat funkyHat Jan 18, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

It does save 8s (out of ~20 for the module 👍), but it also seems to change the results:


self = <readthedocs.rtd_tests.tests.test_resolver.SmartResolverPathTests testMethod=test_resolver_subproject_subdomain>

    def test_resolver_subproject_subdomain(self):
        with override_settings(USE_SUBDOMAIN=False):
            import pdb; pdb.set_trace()
            url = resolve_path(project=self.subproject, filename='index.html')
>           self.assertEqual(url, '/docs/pip/projects/sub/ja/latest/')
E           AssertionError: u'/docs/pip/projects/sub/' != '/docs/pip/projects/sub/ja/latest/'

rtd_tests/tests/test_resolver.py:105: AssertionError
_______________________________ SmartResolverPathTestsAlt.test_resolver_subproject_subdomain _______________________________

self = <readthedocs.rtd_tests.tests.test_resolver.SmartResolverPathTestsAlt testMethod=test_resolver_subproject_subdomain>

    def test_resolver_subproject_subdomain(self):
        with override_settings(USE_SUBDOMAIN=False):
            import pdb; pdb.set_trace()
>           url = resolve_path(project=self.subproject, filename='index.html')
E           AssertionError: u'/docs/pip/projects/sub/' != '/docs/pip/projects/sub/ja/latest/'

I'm not really sure what's going on to cause that, unfortunately. Any clues?

Copy link
Member

Choose a reason for hiding this comment

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

Is SmartResolverPathTestsAlt.test_resolver_subproject_subdomain the only failing test? SmartResolverPathTests is the base class of SmartResolverPathTestsAlt and ResolverBase is the base class of SmartResolverPathTests. There is a setUp method defined in ResolverBase:

https://github.com/rtfd/readthedocs.org/blob/6a669349cf5349087a2369de8a6011b434ac858e/readthedocs/rtd_tests/tests/test_resolver.py#L18-L26

That method can be cause of the test failure.

Adding

setUp = None

to SmartResolverPathTestsAlt may help.

There is also the following warning in setUpTestData documentation:

Be careful not to modify any objects created in setUpTestData() in your test methods. Modifications to in-memory objects from setup work done at the class level will persist between test methods. If you do need to modify them, you could reload them in the setUp() method with refresh_from_db(), for example.

And we indeed do modify objects created in setUpTestData. For example:

https://github.com/rtfd/readthedocs.org/blob/6a669349cf5349087a2369de8a6011b434ac858e/readthedocs/rtd_tests/tests/test_resolver.py#L61

Copy link
Contributor Author

Choose a reason for hiding this comment

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

test_resolver_subproject_subdomain from both SmartResolverPathTests and SmartResolverPathTestsAlt (i.e. yes, the one defined test).
These 2 failures are happening with both setUp methods (i.e. in ResolverBase and ResolverAltSetUp) converted to seUpTestData classmethods.

Are you happy to leave this optimisation for now? Reworking all the modifications seems like overkill for 8s savings (maybe should be a separate PR in any case)?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, let's leave it to a separate PR.

with mock.patch('readthedocs.projects.models.broadcast'):
self.owner = create_user(username='owner', password='test')
self.tester = create_user(username='tester', password='test')
self.pip = get(Project, slug='pip', users=[self.owner], main_language_project=None)
self.seed = get(Project, slug='sub', users=[self.owner], main_language_project=None)
self.subproject = get(Project, slug='subproject', language='ja', users=[self.owner], main_language_project=None)
self.translation = get(Project, slug='trans', language='ja', users=[self.owner], main_language_project=None)
self.pip.add_subproject(self.subproject, alias='sub')
self.pip.translations.add(self.translation)


@override_settings(PUBLIC_DOMAIN='readthedocs.org')
class ResolverDomainTestsAlt(ResolverAltSetUp, ResolverDomainTests):
pass


@override_settings(PUBLIC_DOMAIN='readthedocs.org')
class SmartResolverPathTestsAlt(ResolverAltSetUp, SmartResolverPathTests):
pass


@override_settings(PUBLIC_DOMAIN='readthedocs.org')
class ResolverTestsAlt(ResolverAltSetUp, ResolverTests):
pass
26 changes: 18 additions & 8 deletions readthedocs/rtd_tests/tests/test_subprojects.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -152,24 +152,34 @@ def setUp(self):
self.owner = create_user(username='owner', password='test')
self.tester = create_user(username='tester', password='test')
self.pip = fixture.get(Project, slug='pip', users=[self.owner], main_language_project=None)
self.subproject = fixture.get(Project, slug='sub', language='ja',
users=[ self.owner],
main_language_project=None)
self.translation = fixture.get(Project, slug='trans', language='ja',
users=[ self.owner],
main_language_project=None)
self.subproject = fixture.get(Project, slug='sub', language='ja', users=[
Copy link
Member

Choose a reason for hiding this comment

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

I'd revert these style changes if the linting tool is happy with the old style.

self.owner], main_language_project=None)
self.translation = fixture.get(Project, slug='trans', language='ja', users=[
self.owner], main_language_project=None)
self.pip.add_subproject(self.subproject)
self.pip.translations.add(self.translation)

@override_settings(PRODUCTION_DOMAIN='readthedocs.org')
def test_resolver_subproject_alias(self):
relation = self.pip.subprojects.first()
relation.alias = 'sub_alias'
relation.save()
fixture.get(Project, slug='sub_alias', language='ya')


@override_settings(PRODUCTION_DOMAIN='readthedocs.org')
def test_resolver_subproject_alias(self):
with override_settings(USE_SUBDOMAIN=False):
resp = self.client.get('/docs/pip/projects/sub_alias/')
self.assertEqual(resp.status_code, 302)
self.assertEqual(
resp._headers['location'][1],
'http://readthedocs.org/docs/pip/projects/sub_alias/ja/latest/'
)

def test_resolver_subproject_subdomain_alias(self):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This test causes test_resolver_subproject_alias to fail if it runs first ("fixed" by creatively renaming so it runs after).
I had a bit of a look around to see if there's a cache I can easily clean but couldn't find anything.
Depends how much of an issue you think this is, but if you're worried & can give me a pointer I'll happily update it to dtrt

with override_settings(USE_SUBDOMAIN=True):
Copy link
Member

Choose a reason for hiding this comment

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

Can you use override_settings as decorator?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done :)

resp = self.client.get('/projects/sub_alias/', HTTP_HOST='pip.readthedocs.org')
self.assertEqual(resp.status_code, 302)
self.assertEqual(
resp._headers['location'][1],
'http://pip.readthedocs.org/projects/sub_alias/ja/latest/'
)