Allow python client to accept custom http headers

[shouichi]

Twirp version: 5.8.0

It seems like that the generated python client does not allow us to pass custom http headers. See the following excerpt.

def __make_request(self, body, full_method):
	req = Request(
		url=self.__target + "/twirp" + full_method,
		data=body,
		headers={"Content-Type": "application/protobuf"},
	)

Do you plan to add that feature or accept PR?

Thank you.

[spenczar]

I would be happy to add this feature, yes.

[shouichi]

Is adding optional headers=None argument to generated methods sufficient? For example,

# A generated PRC method
def hello(self, hello_request, headers=None):
	...

For twirp, HTTP is an implementation detail and naively exposing HTTP headers may not be a very good idea. Go client does not directly expose HTTP by wrapping it with context, in other words, generated methods do not accept HTTP headers. In this way, it discourages the overuse of HTTP headers which I believe a good idea.

This can be used to set custom HTTP headers like authorization tokens or client IDs. But note that HTTP headers are a Twirp implementation detail, only visible by middleware, not by the server implementation.

https://godoc.org/github.com/twitchtv/twirp#WithHTTPRequestHeaders

Though adding an optional headers argument may not be a real problem, just wanted to express my concern.

Thank you.

[spenczar]

Right - I think what you're seeing is that the Python code doesn't have a very well-thought-out API today. In Go generated clients, users can pass in their own HTTP Client; we have nothing like that for Python. On the other hand, Go isn't Python. In Go, the tradition is to be strict about hiding implementation details, and to discourage bad behavior very strongly. In Python, shooting yourself in the foot is discouraged, but rarely completely banned :)

I agree that headers=None is probably not the best way to do this. Is there some way we can make the generated client a little more flexible for Python.

[shouichi]

Disclaimer: I rarely write python.

One idea is to allow users to decorate twirp client with http request handler.
For example,

def handler(request):
    request.add_header('X-Foo', 'bar')
    return request

client = HelloAPIClient('http://localhost:3000')
decorated = client.decorate(handler)
response = decorated.hello(request)

This way, the generated method signature stays the same and it should
discourage overuse of http implementation details.

This can be accomplished by using a custom url opener and works both python 2
and 3.

https://docs.python.org/2/library/urllib2.html
https://docs.python.org/3/library/urllib.request.html

Any feedback would be appreciated 😄

[shan98]

I like the idea but why not with a simpler approach by extending the constructor of the generated client class to accept an optional request handler fuction.

    def __init__(self, server_address, request_decorator=None):
        ...
        self.__reqeust_decorator = request_decorator

    ...
    ...
    
    def __make_request(self, body, full_method):
        req = Request(
            url=self.__target + "/twirp" + full_method,
            data=body,
            headers={"Content-Type": "application/protobuf"},
        )

        if self.__request_decorator:
            req = self.__request_decorator(req)

        try:
            resp = urlopen(req)
        except HTTPError as err:
            raise TwirpException.from_http_err(err)

        return resp.read()

You could also add a class check on req to see if its urllib.Request

This is also matches the Go client signature.

[shouichi]

Thanks for the feedback!

One reason is to change headers dynamically.

user_id = get_user_id_from_request()
decorated = client.decorate(with_user_id(user_id))

We can accomplish the same thing with your idea but it seems somewhat strange to pass server_address every time when we just want to change headers.

[shan98]

Well we could change it to self.request_decorator (instead of the underscores to indicate its private), then you could just change that property as needed.

[shouichi]

That won't be safe with multi-threaded servers. We need to make clients immutable.

[shan98]

Hm, im not too familiar with the internals, but how would dynamically changing headers through a decorator function be any different? Maybe if you could show a more concrete example of the implementation you had in mind.

[ofpiyush]

Our implementation of the python client and server allow you to set headers via the context object.

https://github.com/verloop/vtwirp

[ofpiyush]

@dpolansky Readme mentions this so can close this issue unless you want to add support in this repo as well.

[marioizquierdo]

I don't think we will support the Python implementation in this repo. It mainly works as an example, and we may end up removing it. For now, mentioning the verloop/vtwirp implementation should be good to offer a better alternative.