Expose redirect chain from test client

[twolfson]

I am currently with a project, splinter, which emulates browser-like interactions via the Flask/Werkzeug test client. Their current URL behavior is broken redirects as its currently serving the originally requested URL, not the final one.

Currently, the test client has no way of telling what the final request location was (nor its hops along the way). I was wondering if we could return the redirect_chain somehow (e.g. as an attribute of the Response object).

https://github.com/mitsuhiko/werkzeug/blob/0.10.4/werkzeug/test.py#L744-L760

[untitaker]

The final location lives on the response object.

[twolfson]

I don't see any attributes for it. What property are we talking about?

http://werkzeug.pocoo.org/docs/0.10/wrappers/#werkzeug.wrappers.BaseResponse

[untitaker]

Ah sorry, I misremembered.

[twolfson]

No worries. I felt like it should exist as well. What is the interface you would consider adding? I am open to writing a PR for this.

[untitaker]

I don't really know. I think keeping the API similar to requests is useful.

On Wed, Aug 19, 2015 at 01:49:13PM -0700, Todd Wolfson wrote:

No worries. I felt like it should exist as well. What is the interface you would consider adding? I am open to writing a PR for this.

---

Reply to this email directly or view it on GitHub:
#763 (comment)

[twolfson]

I was hoping there was a way to define BaseResponse.redirect_chain (that's how splinter is interacting with Django) but that might be a bit of a hack.

https://github.com/cobrateam/splinter/blob/0.7.3/splinter/driver/djangoclient.py#L94-L98

For my interim hack/patch, I have extended on as_tuple but I don't believe that's practical as it's not backwards compatible:

cobrateam/splinter@cobrateam:5317b71...underdogio:f7e0da6#diff-df553efba1487ab0f96b4053dee86c25R87

[davidism]

I looked into this too, and it's tricky to fit this in to the existing behavior. The problem is that what we return isn't flexible. Either we return a Response class passed in by the user, which isn't going to have test-related attributes like redirect_chain. Or we return a tuple that users are probably unpacking, so adding another value would break existing tests without a clear way to have a deprecation window.

I understand why we allow returning a tuple, since that maps directly to WSGI start_response, but I don't think anyone should be doing serious testing with it any more. You're going to have a way easier time inspecting a Response object, it's the way other tools like httpx work.

I think I have a compromise, but someone would need to try to implement it to see if it's viable. I'd like to always return a Response object, using a new default TestResponse subclass. This class can provide new things like redirect_chain as well as backwards compatibility with unpacking by overriding __iter__ or something to make it look unpackable. Since the user can pass in a class, and TestResponse would provide extra functionality, we would create a new subclass based on both. UserTestResponse(TestResponse, response_class).

Speaking of httpx, if you need this and other features in the mean time, it has great support for making requests to WSGI apps built-in. Requests and responses look exactly the same as they would if you were making an HTTP request.

[michaelbukachi]

@davidism For us to hook up the TestResponse with redirect_chain we'd need to override werkzeug's Client since we need to tap into its open method. Let me see if I can come up with a snippet.

[davidism]

This is a Werkzeug issue, you can change the client code directly. 😉

[michaelbukachi]

@davidism this is what I was thinking. It adds the redirect_chain to the response. Not a super elegant solution though:sweat_smile:

....

class Client():

    def open(self, *args, **kwargs):
        as_tuple = kwargs.pop("as_tuple", False)
        buffered = kwargs.pop("buffered", False)
        follow_redirects = kwargs.pop("follow_redirects", False)
        environ = None
        if not kwargs and len(args) == 1:
            if isinstance(args[0], EnvironBuilder):
                environ = args[0].get_environ()
            elif isinstance(args[0], dict):
                environ = args[0]
        if environ is None:
            builder = EnvironBuilder(*args, **kwargs)
            try:
                environ = builder.get_environ()
            finally:
                builder.close()

        response = self.run_wsgi_app(environ.copy(), buffered=buffered)

        # handle redirects
        redirect_chain = []
        while 1:
            status_code = int(response[1].split(None, 1)[0])
            if (
                status_code not in {301, 302, 303, 305, 307, 308}
                or not follow_redirects
            ):
                break

            # Exhaust intermediate response bodies to ensure middleware
            # that returns an iterator runs any cleanup code.
            if not buffered:
                for _ in response[0]:
                    pass

            new_location = response[2]["location"]
            new_redirect_entry = (new_location, status_code)
            if new_redirect_entry in redirect_chain:
                raise ClientRedirectError("loop detected")
            redirect_chain.append(new_redirect_entry)
            environ, response = self.resolve_redirect(
                response, new_location, environ, buffered=buffered
            )

        if self.response_wrapper is not None:
            response = self.response_wrapper(*response)
            setattr(response, 'redirect_chain', redirect_chain) # This will enable us to access the redirect chain

        if as_tuple:
            return environ, response
        return response

[davidism]

Makes sense. My comment above was addressing how we return the chain. I want to avoid setting extra attributes (using setattr or regular assignment). I also want to phase out the three different return types that can happen. A TestResponse class could address both these issues.

[michaelbukachi]

@davidism Have you been able to take a look at the PR?