Avoid failing on headers conflict

[nfroidure]

The folowing error happen when a header has been set by the client library twice (one time in uppercase and another time in lower case. Failed to convert header keys to lower case due to field name conflict: {headerName}.

It is certainly a bad practice to do so but the fact is that nock is used mostly to mock external libraries calls so it would be useful to be able to disable error throwing for such cases with some options.

[felipead]

Hi, I vote for this issue to be fixed. It's annoying me too when I use .defaultReplyHeaders. For example, if I define a nock as:

const scope = nock(URL, {encodedQueryParams: true})
    .defaultReplyHeaders({
      'Content-Type': 'application/json;charset=UTF-8'
    })

Then if I try to override the Content-Type for a specific test needs:

  scope
    .post('/login', `login=${USER}&password=${PASSWORD}`)
    .reply(200, '<login>ok</login>', [
      'Content-Type', 'application/xml',
      'Set-Cookie', 'JSESSIONID=foobar;Path=/foobar;Secure;HttpOnly'
    ])

I get the following error:

  Error: Failed to convert header keys to lower case due to field name conflict: content-type

The only solution is to convert all headers to lowercase. But nock should not require me to do this, it should be case insensitive.

[ianwsperber]

@felipead Your example makes clear that this example is a bug/undesired. If you or @nfroidure are able to open a PR fixing this please do so. Otherwise I will try to get to when I have free time.

[felipead]

Thanks!

[moaxaca]

@ianwsperber Hey there appears to be an open PR, can you get the resolved?

[ianwsperber]

@moaxaca Just left a comment on the PR! You can help get the PR merged faster if you could confirm whether it fixes the bug on your machine 😄

[moaxaca]

@ianwsperber PR seems to works on my machine. 👍 I will dig into the change later tonight.

[ianwsperber]

Thanks @moaxaca! Please comment on the PR when you have a chance.

[dcstone09]

@ianwsperber @moaxaca The PR has been updated 👍

[moaxaca]

@dcstone09 Sounds good pulling it in 40m to see if everything passes.

[felipead]

🎉

[CFreeAetna]

Any updates here?

[dcstone09]

@CFreeAtEbsco I just updated the test, everything looks good to me.

[tedeh]

Why hasn't this been merged yet? It would appear to be a mostly harmless PR. Last given option should override earlier ones, like every other node library around.

[gr2m]

@tedeh snarky comments won't get you anywhere. I understand your frustration, but please refrain from doing it on any open source project, ever <3

[tedeh]

You are right and I'm sorry.

Is there anything in your opinion that prevents this from being merged as-is? Maybe I can help with the verification.

[gr2m]

Thanks Tedde, it's already merged :)
#862

But the build failed so it's not released yet, if you could have a look at it that would be much appreciated: https://travis-ci.org/node-nock/nock/builds/340676385

[gkatsanos]

@gr2m is this released?

[RichardLitt]

@gkatsanos Should have been! v9.1.7 got it. https://github.com/nock/nock/releases/tag/v9.1.7

[lock]

This thread has been automatically locked since there has not been any recent activity after it was closed. Please open a new issue and add a reference to this one if it’s related. Thank you!