Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Leave rawHeaders case untouched on nock's replies #707

Closed
hugoduraes opened this issue Sep 30, 2016 · 4 comments
Closed

Leave rawHeaders case untouched on nock's replies #707

hugoduraes opened this issue Sep 30, 2016 · 4 comments

Comments

@hugoduraes
Copy link
Contributor

hugoduraes commented Sep 30, 2016

I've noticed that nock is creating rawHeaders from the response headers object, which keys are lowercased. This is causing rawHeaders keys to also be lowercased, which I think is wrong because nock's rawHeaders may be getting different from the real response's rawHeaders. I believe rawHeaders case should be returned untouched.

This is the code responsible for it on lib/interceptor.js:62:

//  We use lower-case headers throughout Nock.
headers = common.headersFieldNamesToLowerCase(headers);

_.defaults(this.options, this.scope.scopeOptions);

if (this.scope._defaultReplyHeaders) {
    headers = headers || {};
    headers = mixin(this.scope._defaultReplyHeaders, headers);
}

if (this.scope.date) {
    headers = headers || {};
    headers['date'] = this.scope.date.toUTCString();
}

if (headers !== undefined) {
    this.headers = {};
    this.rawHeaders = [];

    // makes sure all keys in headers are in lower case
    for (var key2 in headers) {
        if (headers.hasOwnProperty(key2)) {
            this.headers[key2.toLowerCase()] = headers[key2];
            this.rawHeaders.push(key2);
            this.rawHeaders.push(headers[key2]);
        }
    }
    debug('reply.rawHeaders:', this.rawHeaders);
}

Why don't nock builds the headers object based on the rawHeaders array? Like that rawHeaders would remain untouched and headers could still have lowercased keys.

What do you think?

@hugoduraes hugoduraes changed the title Include rawHeaders in the recorded object Leave rawHeaders untouched on nock's replies Oct 3, 2016
@hugoduraes hugoduraes changed the title Leave rawHeaders untouched on nock's replies Leave rawHeaders case untouched on nock's replies Oct 3, 2016
@vrinek
Copy link
Contributor

vrinek commented Oct 6, 2016

I agree with your point that rawHeaders should be left intact. I also commented on your PR.

In case this goes in, it sounds like a breaking change for a few users that may be relying on rawHeaders being lower case. I'll double-check the documentation to verify whether that was expected to be exposed or not.

@hugoduraes
Copy link
Contributor Author

Yes, that'll be a breaking change. I've updated my PR to fix it for Node 0.10.x. Please take a look at the comment I left there.

@vrinek
Copy link
Contributor

vrinek commented Oct 7, 2016

PR merged!

@vrinek vrinek closed this as completed Oct 7, 2016
@lock
Copy link

lock bot commented Sep 13, 2018

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!

@lock lock bot locked as resolved and limited conversation to collaborators Sep 13, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants