search

cdent / gabbi

Declarative HTTP Testing for Python and anything else
http://gabbi.readthedocs.org/
Other
148 stars 34 forks source link

Find the hostname for SNI regardless of case used in `request_headers` #316

Closed scottwallacesh closed 1 year ago

scottwallacesh commented 1 year ago

This change ensures that the hostname needed for SNI can be found regardless of the case used for the keys in request_headers.

cdent commented 1 year ago

I think this is fine as a fix, but it does make me wonder if we should canonicalise the header names when reading in the tests.

Any thoughts on that?

The reason it occurs to me is because I've been working on a golang version and go's libraries do a similar kind of canonicalisation.

I've resisted it in the past for people who for some reason actually do want to test sending a header with weird case and now that the precedent is set, changing it would be odd.

I'm happy to merge this (and do a new release) once the pep8 issue is resolved.

scottwallacesh commented 1 year ago

I think this is fine as a fix, but it does make me wonder if we should canonicalise the header names when reading in the tests.

Any thoughts on that?

The RFCs for HTTP say the headers should be case-insensitive, so I'm inclined to agree that they should be canonicalised but I'm also aware of breaking other people's tests. If you're migrating to a Go-based version then I would leave the Python version as-is for now unless there are other more important reasons why they should be canonicalised.

FND commented 1 year ago

I agree that header normalization would be desirable, but backwards compatibility trumps that now. We might introduce some kinda opt-in mechanisms, but this seems too arcane to warrant user choice.

FWIW, my first instinct with the dictionary comprehension was "this seems very roundabout and not very Pythonic" (I didn't grok right away what was happening there) - only to realize there's probably no more straightforward or efficient alternative. However, that thought did lead me to Requests's CaseInsensitiveDict; might be handy if we do go with normalization at some point.

scottwallacesh commented 1 year ago

I'm not entirely sure what to do about the current broken test as it doesn't appear to have been caused by this change. I don't have Python 3.7 on my machine and can't test it locally.

Any suggestions? Should I attempt to fix it or is it a known issue that can be safely ignored?

cdent commented 1 year ago

If you're migrating to a Go-based version

It's not so much a migration as playing around to see what's possible and if there are any advantages. If it works out I don't intend to abandon gabbi in favour of gobbi or anything like that.

Note that we probably already has case-insensitivity for evaluating response-headers. It's in the send of request headers where we do not, and may not want to, have it.

The placement 3.7 tests failing is something in placement itself. I'll fix that separately. Once that's done this should be good to go.

cdent commented 1 year ago

2.7.2 is on pypi now https://pypi.org/project/gabbi/