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

Set urlconf to None after changing SUBDOMAIN setting #4032

Merged
merged 5 commits into from May 30, 2018

Conversation

Projects
None yet
3 participants
@stsewd
Member

stsewd commented Apr 26, 2018

The urlconf value is preserved for the current thread, this causes errors when using the reverse function.

When the request is made inside that test, django overrides the url_conf (readthedocs.core.urls.subdomain) for the current thread! https://docs.djangoproject.com/en/1.9/_modules/django/core/urlresolvers/ (set_urlconf). So, all tests are running in that thread with that configuration, till the value get's override (to readthedocs.urls) doing another request without the HTTP_HOST header.

So, is this a bug on django or on the rtd code? I'm not sure, but adding this teardown method solves the issue.

You can reproduce this with this branch #4017 (comment)

Also this bug is affecting this other PR #3913 (comment)

@ericholscher

This comment has been minimized.

Member

ericholscher commented Apr 26, 2018

Hmm. Does this happen in prod? We are setting the urlconf in middleware, but should we be unsetting it also? Or is Django reseting it during normal prod usage, but not in the test cases?

@ericholscher

This comment has been minimized.

Member

ericholscher commented Apr 26, 2018

Looks like Django resets it on Response, so in prod it will be reset here I believe: https://github.com/django/django/blob/3df8ccf6fc3fa0ab2acf9a03da43fea87f8ff392/django/core/handlers/base.py#L113

@ericholscher

This comment has been minimized.

Member

ericholscher commented Apr 26, 2018

Still feels like this fix should probably go somewhere else, so that it doesn't keep happening across test cases. I wonder if we should be unsetting it in the middleware here, where it's set: https://github.com/rtfd/readthedocs.org/blob/master/readthedocs/core/middleware.py#L36

@stsewd

This comment has been minimized.

Member

stsewd commented Apr 26, 2018

I'm not sure if this happens on prod, this can be replicated with any url_name (I was testing this with a generated url from rest_framework only), I just replicate this same behavior with projects_detail url_name.

So, at least we aren't using any reverse function, I think this only happens on tests.

@stsewd

This comment has been minimized.

Member

stsewd commented Apr 26, 2018

Still feels like this fix should probably go somewhere else, so that it doesn't keep happening across test cases.

Yeah, I was thinking the same, but this was the only idea I was able to think, I going to take a deeper look to the code you linked and see if this can be fixed for all tests cases.

@stsewd

This comment has been minimized.

Member

stsewd commented Apr 27, 2018

This is what I found about request.urlconf https://docs.djangoproject.com/en/1.9/ref/request-response/#django.http.HttpRequest.urlconf

urlconf can be set to None to revert any changes made by previous middleware and return to using the ROOT_URLCONF

And this from a lib that changes the urlconf attr of the request

https://github.com/jazzband/django-hosts/blob/fa820630bf66113f1be3e4cf60e9a730fd52eb1a/django_hosts/middleware.py#L51-L89

But I think this behavior is different on the Django version that rtd uses according to the code that handle the middlewars

https://github.com/django/django/blob/335aa088245a4cc6f1b04ba098458845344290bd/django/core/handlers/base.py#L122-L132

@stsewd

This comment has been minimized.

Member

stsewd commented Apr 27, 2018

I did some tests with this solution (restoring the urlconf attr when processing the response) and it works, but I'm not sure if this can break other stuff. Also, maybe we want to apply this solution to the others middlewares that modify urlconf on the process_request method. I'm going to look deeper on the django code of >1.10 and see if the comments of the previous lib make sense there (edit: actually the logic there is completely different from what we do :)).

@stsewd

This comment has been minimized.

Member

stsewd commented Apr 27, 2018

@stsewd

This comment has been minimized.

Member

stsewd commented Apr 27, 2018

I'll try to write some tests for this later

@stsewd

This comment has been minimized.

Member

stsewd commented May 1, 2018

ok, I managed to write a test for this, it fails if the new code is deleted

@ericholscher

This comment has been minimized.

Member

ericholscher commented May 22, 2018

This looks good to me, but does scare me a little bit. It will be pretty obvious if it breaks in prod though, so I think it makes sense to merge it.

@humitos

This comment has been minimized.

Member

humitos commented May 30, 2018

This makes sense to me.

I pull down the branch, run the test with and without the changes and it worked as expected.

It seems that it won't fail in production and I think we are ready to merge it.

@humitos humitos merged commit dec550f into rtfd:master May 30, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

@stsewd stsewd deleted the stsewd:teardown-after-change-config-subdomain branch May 30, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment