bravado icon indicating copy to clipboard operation
bravado copied to clipboard

Small refactor and improvement of headers conversion to strings

Open macisamuele opened this issue 7 years ago • 5 comments

As highlighted on #300 we were converting to string headers in a really "aggressive" way which could lead to undesiderable behaviors. I would suggest to unify the headers conversion to a single util method to reduce duplication between RequestsClient and FidoClient and to convert only headers that needs to be converted. We can use https://github.com/requests/requests/blob/master/requests/utils.py#L854 to determine if the header needs to be converted.

@jdb8, @sjaensch: any thought ?

macisamuele avatar Jun 28 '17 08:06 macisamuele

Isn't that something that could be done after the header has been converted to str? What we are doing is making sure non-str values are converted to str, how would that function help? I think calling that code with e.g. a bool header value would raise a TypeError.

sjaensch avatar Jun 28 '17 08:06 sjaensch

We need to convert headers to string because check_header_validity will raise an exception on invalid non-string headers. To make more general and avoid un-needed conversion that could lead to cases like in #300 I was thinking to something similar to

def is_header_valid(header_name, header_value):
    try:
        check_header_validity((header_name, header_value))
        return True
    except InvalidHeader:
        return False

and adding an unified function that takes care of headers string conversion

def convert_headers_to_strings(headers):
    return {
        k: v if is_header_valid(k, v) else str(v)
        for k, v in iteritems(headers)
    }

Doing so we can offload the check of header validity to requests library, which is actually enforcing string headers

macisamuele avatar Jun 28 '17 09:06 macisamuele

Isn't that just a heavier version (involving regexes) for what we're currently doing? Plus it will still cause us to call str() on a bytestring header value if that value is not valid according to requests.

sjaensch avatar Jun 28 '17 09:06 sjaensch

you're right ... it could result in being quite heavier in checking only types! So let's get rid of that!

I would still unify the conversion logic within HttpClient class, so it will de defined only in one place and made accessible by all our clients (and eventual custom clients developed by library users).

macisamuele avatar Jun 28 '17 10:06 macisamuele

Yeah makes sense!

sjaensch avatar Jun 28 '17 11:06 sjaensch