webmock icon indicating copy to clipboard operation
webmock copied to clipboard

Keep underscores when normalizing headers

Open mvz opened this issue 3 years ago • 2 comments

Some servers distinguish underscores from dashes in request headers. To allow users to test that the correct character is used, checking the sent headers should make the same distinction.

With this change, webmock no longer normalizes underscores to dashes for headers defined using strings.

Since symbols are commonly used with underscores to specify headers that use dashes, headers specified with symbol keys are still normalized to dashes.

Fixes #474.

mvz avatar Jan 05 '22 21:01 mvz

@mvz thank you for the PR. I'm not sure about the new logic though. It has a different behaviour for symbols than strings based on assumption "commonly used with underscores" yet strings are not.

I think the behaviour should be unified and deterministic for every case.

Change like that will require a major version bump anyway.

I'm keen to stop normalizing headers completely, do a major version bump, yet have a way of headers matching, that has a mapping of acceptable common symbols used for some headers. E.g. :content_type => 'Content-Type' That mapping could be extended by developers.

bblimke avatar Jan 06 '22 22:01 bblimke

It has a different behaviour for symbols than strings based on assumption "commonly used with underscores" yet strings are not.

Yes, that is on purpose since symbols with dashes are harder to write so many developers will probably have symbols used like that. However, I'm all for being more strict here and just unifying the behavior.

In fact, given https://github.com/bblimke/webmock/issues/863#issuecomment-571339772, normalization could then become just a simple .downcase on the stringified key.

Change like that will require a major version bump anyway.

To make this smoother, this could also be introduced as an optional change controlled by a setting. The old behavior can than be removed with a major version bump later.

mvz avatar Jan 07 '22 07:01 mvz