bravado
bravado copied to clipboard
Small refactor and improvement of headers conversion to strings
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 ?
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.
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
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.
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).
Yeah makes sense!