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

fix($http): fix response headers with an empty value #10091

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
4 participants
@jamshid
Copy link
Contributor

jamshid commented Nov 17, 2014

Empty response header values are legal (http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html).
Return an empty string instead of null, which is returned when the header does not exist.

Closes #7779

fix($http): fix response headers with an empty value
Empty response header values are legal (http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html).
Return an empty string instead of null, which is returned when the header does not exist.

Closes #7779
expect(callback).toHaveBeenCalledOnce();
expect(callback.mostRecentCall.args[2]('custom-empty-response-Header')).toBe('');
});

This comment has been minimized.

Copy link
@caitp

caitp Nov 17, 2014

Contributor

Since this change uses hasOwnProperty, add a test for header values in the object prototype, like ToString or Constructor, too.

@jamshid

This comment has been minimized.

Copy link
Contributor Author

jamshid commented Nov 17, 2014

Do you mean adding a test like this?

@@ -818,11 +818,13 @@ describe('$http', function() {

       it('should handle empty response header', function() {
        $httpBackend.expect('GET', '/url', undefined)
-           .respond(200, '', { 'Custom-Empty-Response-Header': '' });
+           .respond(200, '', { 'Custom-Empty-Response-Header': '', 'Constructor': '' });
        $http.get('/url').success(callback);
        $httpBackend.flush();
        expect(callback).toHaveBeenCalledOnce();
        expect(callback.mostRecentCall.args[2]('custom-empty-response-Header')).toBe('');
+       expect(callback.mostRecentCall.args[2]('ToString')).toBe(null);
+       expect(callback.mostRecentCall.args[2]('Constructor')).toBe('');
      });

I see, the Constructor test is failing. How do I make headersObject['Constructor'] return the header value, not the Constructor function itself?

      return Object.prototype.hasOwnProperty.call(headersObj, name) ? headersObj[name] : null;
Chrome 38.0.2125 (Mac OS X 10.10.0) $http the instance short methods should handle empty response header FAILED
    Expected 'function Object() { [native code] }, ' to be ''.
    Error: Expected 'function Object() { [native code] }, ' to be ''.
        at null.<anonymous> (/Users/jamshid/work/angular.js/test/ng/httpSpec.js:827:63)
fix($http): fix response headers with an empty value
Empty response header values are legal (http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html).
Return an empty string instead of null, which is returned when the header does not exist.
The "Constructor" test is failing, not sure how to fix.

Closes #7779
@jamshid

This comment has been minimized.

Copy link
Contributor Author

jamshid commented Nov 18, 2014

I assume @cruz210 or @caitp saw my note about not knowing how to fix the failing "Constructor" test case. If you know how to fix please let me know and I'll make a final fix and squash commits.

@caitp

This comment has been minimized.

Copy link
Contributor

caitp commented Nov 18, 2014

I didn't, but I'll take another look at this (you can just ping me with PTAL in the message, it works)

@@ -63,7 +63,8 @@ function headersGetter(headers) {
if (!headersObj) headersObj = parseHeaders(headers);

if (name) {
return headersObj[lowercase(name)] || null;
name = lowercase(name);
return Object.prototype.hasOwnProperty.call(headersObj, name) ? headersObj[name] : null;

This comment has been minimized.

Copy link
@caitp

caitp Nov 18, 2014

Contributor

You can use just hasOwnProperty.call(), it's fine

This comment has been minimized.

Copy link
@caitp

caitp Nov 18, 2014

Contributor

So the test is failing on the Constructor header --- I think the reason is we need to fix the parseHeaders function so that it uses createMap() instead of {} when initializing parsed.

EG:

parsed = createMap(); // rather than `parsed = {}`

This comment has been minimized.

Copy link
@caitp

caitp Nov 18, 2014

Contributor

Pretty sure that will fix it and help us merge it.

fix($http): fix response headers with an empty value
Empty response header values are legal (http://www.w3.org/Protocols/rfc2616/rfc2616-sec4.html).
Return an empty string instead of null, which is returned when the header does not exist.

Closes #7779
@jamshid

This comment has been minimized.

Copy link
Contributor Author

jamshid commented Nov 18, 2014

Thanks, createMap() fixed it. Now I need to squash these 3 commits but don't know how. I did not create a "feature" branch when I first forked and made the changes, is that a necessary step? Not very familiar with git, any tips appreciated.

@caitp

This comment has been minimized.

Copy link
Contributor

caitp commented Nov 18, 2014

you should be good with git rebase -i HEAD~3 and setting the first letter on each of the last 2 commits to "f" --- then after that you might want to git pull --rebase origin master

@jamshid

This comment has been minimized.

Copy link
Contributor Author

jamshid commented Nov 18, 2014

Did I do this right, it doesn't seem like the single squashed commit is showing up here.

$ git rebase -i HEAD~3
[detached HEAD 0ef0dce] fix($http): fix response headers with an empty value
 2 files changed, 14 insertions(+), 2 deletions(-)
Successfully rebased and updated refs/heads/master.
$ git pull --rebase origin master
From https://github.com/jamshid/angular.js
 * branch            master     -> FETCH_HEAD
First, rewinding head to replay your work on top of it...
Applying: fix($http): fix response headers with an empty value
Using index info to reconstruct a base tree...
M   src/ng/http.js
M   test/ng/httpSpec.js
Falling back to patching base and 3-way merge...
No changes -- Patch already applied.
$ git push origin master
Everything up-to-date
@pkozlowski-opensource

This comment has been minimized.

Copy link
Member

pkozlowski-opensource commented Nov 18, 2014

@jamshid you would have to force-push to see your single commit here: git push origin master --force

But don't worry about this too much, we can always sqash commits when merging.

@caitp

This comment has been minimized.

Copy link
Contributor

caitp commented Nov 18, 2014

Anyways, LGTM, will land

@caitp caitp closed this in 637c020 Nov 18, 2014

@caitp

This comment has been minimized.

Copy link
Contributor

caitp commented Nov 18, 2014

@jamshid I made some very minor changes to it because the hasOwnProperty is not necessary given that there is a null prototype (unless someone does something unsupported like Object.setPrototypeOf(headersGetter(), new Object()) or something, but I'm not too worried there)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.