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

fix($http) - allow for user removal of default headers by passing header functions #5785

Closed
wants to merge 1 commit into from
Closed

fix($http) - allow for user removal of default headers by passing header functions #5785

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 2 additions & 4 deletions src/ng/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -756,10 +756,6 @@ function $HttpProvider() {

defHeaders = extend({}, defHeaders.common, defHeaders[lowercase(config.method)]);

// execute if header value is function
execHeaders(defHeaders);
execHeaders(reqHeaders);

// using for-in instead of forEach to avoid unecessary iteration after header has been found
defaultHeadersIteration:
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is there a reason to use a label here? I would propose indexing the reqHeaders before the loop and then call hasOwnProperty. Like such:

       for (reqHeaderName in reqHeaders) {
            reqHeadersIndex[lowercase(reqHeaderName)] = true;
        }

        // using for-in instead of forEach to avoid unecessary iteration after header has been found
        for (defHeaderName in defHeaders) {
          if(reqHeadersIndex.hasOwnProperty(lowercase(defHeaderName))){
            continue;
          }

          reqHeaders[defHeaderName] = defHeaders[defHeaderName];
        }

This removes use of the label and nested for loop bringing the overall computational complexity of mergeHeaders from O(n2) to O(n)

Copy link
Member

Choose a reason for hiding this comment

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

@perek actually I was asking myself the same question while looking at this code today! It was introduced to properly handle insensitive headers merging in 53359d5

Then again, with indexing we would be trading memory for speed as we would need to keep an indexing array in memory. In any case I don't think it has much practical implications as numbers of headers to consider is usually < 5 so no need to split hairs over it. But yeh, it could have been written differently.

My suggestion: let's fix the underlying issue in this PR and think of optimisations in a separate PR.

Could you remove duplicated tests and squash commits?

for (defHeaderName in defHeaders) {
Expand All @@ -774,6 +770,8 @@ function $HttpProvider() {
reqHeaders[defHeaderName] = defHeaders[defHeaderName];
}

// execute if header value is a function for merged headers
execHeaders(reqHeaders);
return reqHeaders;

function execHeaders(headers) {
Expand Down
12 changes: 12 additions & 0 deletions test/ng/httpSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -747,6 +747,18 @@ describe('$http', function() {
$httpBackend.flush();
});

it('should delete default headers if custom header function returns null', function () {

$httpBackend.expect('POST', '/url', 'messageBody', function(headers) {
return headers['Accept'] === undefined;
}).respond('');

$http({url: '/url', method: 'POST', data: 'messageBody', headers: {
'Accept': function() { return null; }
}});
$httpBackend.flush();
});

it('should override default headers with custom in a case insensitive manner', function() {
$httpBackend.expect('POST', '/url', 'messageBody', function(headers) {
return headers['accept'] == 'Rewritten' &&
Expand Down