Skip to content
This repository was archived by the owner on Apr 12, 2024. It is now read-only.

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

Closed
wants to merge 2 commits into from
Closed

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

wants to merge 2 commits into from

Conversation

jamshid
Copy link
Contributor

@jamshid jamshid commented Jun 11, 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

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 jamshid added cla: yes and removed cla: no labels Jun 11, 2014
@rodyhaddad rodyhaddad modified the milestone: Backlog Jun 11, 2014
@@ -549,6 +549,12 @@ describe('$http', function() {
});


it('getter function should allow headers without value', function() {
// I don't know how to fix: 'headersGetter' is not defined.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're better off having your test somewhere here. And instead of testing the headersGetter, try testing something similar to the actual code the user will write, so with $httpBackend and a $http call.

@rodyhaddad rodyhaddad added this to the Purgatory milestone Jun 11, 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
@jamshid
Copy link
Contributor Author

jamshid commented Jun 13, 2014

I don't think the build failures are related to my changes, but let me know. "grunt test" passes for me.

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

if (name) {
return headersObj[lowercase(name)] || null;
name = lowercase(name);
return name in headersObj ? headersObj[name] : null;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't totally like this form. 'constructor' in {} === true, and some browsers have weird things in Object.prototype which you wouldn't expect (like watch, unwatch in Firefox). These don't frequently hit header names, but they definitely could, so I say use hasOwnProperty.call(headersObj, name) instead.

@pkozlowski-opensource
Copy link
Member

While this is not very frequent and kind of corner case it is still a valid issue and a fix is easy so I guess we still should fix it.

@jamshid do you still want to put some effort into this one and:

  • squash commits
  • rebase it on top of the current master
  • address @caitp comment

If not I can quickly put together an alternative patch.

@jamshid
Copy link
Contributor Author

jamshid commented Nov 16, 2014

sure, will try to get it done later today

@pkozlowski-opensource pkozlowski-opensource added this to the Backlog milestone Nov 16, 2014
@pkozlowski-opensource pkozlowski-opensource removed this from the Purgatory milestone Nov 16, 2014
@pkozlowski-opensource
Copy link
Member

Closing this one - we are going to merge #10091

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Response headers with empty values should return empty string, not null
4 participants