Opened 7 years ago

Closed 5 years ago

#28337 closed Cleanup/optimization (fixed)

fetch_redirect_response from assertRedirects doesn't honor HTTP_HOST setting in self.client.get

Reported by: Roland van Laar Owned by: Hasan Ramezani
Component: Testing framework Version: 1.11
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

I would like to see assertRedirects honor the HTTP_HOST settings in self.client.get.

I'm writing middleware that checks if request.get_host() is coupled to a site, else we do a redirect to an external domain.

def test_redirect(self):
    response = self.client.get("/", follow=False, HTTP_HOST=self.site1.domain)
    self.assertRedirects(response, '/accounts/login/?next=', fetch_redirect_response=True)

The assertRedirect does an extra request to follow the redirect. What goes wrong is that the HTTP_HOST with the request is not set to
'{{ self.site1.domain }}' but to testserver. This results in a redirect to our external domain.

Middleware code:

class SiteMiddleware(object):
    def __init__(self, get_response):
        self.get_response = get_response
    def __call__(self, request):
        host = self.request.get_host()
        try:
            request.site = Site.objects.get(domain=host)
        except ObjectDoesNotExist:
            return HttpResponseRedirect("http://example.com")
        return self.get_response(request)

Change History (7)

comment:1 by Tim Graham, 7 years ago

Triage Stage: UnreviewedAccepted
Type: BugCleanup/optimization

comment:2 by Patrick Jenkins, 7 years ago

Owner: changed from nobody to Patrick Jenkins
Status: newassigned

comment:3 by Patrick Jenkins, 7 years ago

The problem with the provided example is that under certain conditions assertRedirect will replay the request with different parameters than originally specified. The conditions necessary for this to occur are passing follow=False to the original client.get and also passing 'extra' parameters that will be interpreted as HTTP header's. Assert redirect (version 1.11.4: https://github.com/django/django/blob/1a34dfcf797640d5d580d261694cb54e6f97c552/django/test/testcases.py#L317) makes an assumption that it is possible to exactly recreate the original request with the information that is available.

To properly recreate the the original request it would be necessary to pass ALL method parameters AND all of the kwarg parameters that were originally passed in. This would require storing the 'extras' in the Client (django.test.client.py) so they could be acccessed in assertRedrrects and properly replayed.

I believe this is the proper solution because it removes the assumptions present when assertRedirect attempts to recreate the request. Although this is a corner case, it is certainly acceptable for Django users to utilize the infinite space that is HTTP headers and provide varying logic depending on the values present. The downside of this implementation is that it bloats the Client by requiring it to store the extra parameters passed in. These extra parameters will rarely be used but I believe the tradeoff of adding properties to Client and potentially increasing memory footprint are small enough to warrant this change.

comment:4 by Patrick Jenkins, 7 years ago

Has patch: set

comment:5 by Tim Graham, 7 years ago

Patch needs improvement: set

comment:6 by Hasan Ramezani, 5 years ago

Owner: changed from Patrick Jenkins to Hasan Ramezani
Patch needs improvement: unset
Version 0, edited 5 years ago by Hasan Ramezani (next)

comment:7 by Mariusz Felisiak <felisiak.mariusz@…>, 5 years ago

Resolution: fixed
Status: assignedclosed

In 46e74a52:

Fixed #28337 -- Preserved extra headers of requests made with django.test.Client in assertRedirects().

Co-Authored-By: Hasan Ramezani <hasan.r67@…>

Note: See TracTickets for help on using tickets.
Back to Top