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

[perek]

Adds additional testing and fixes #5784

CLA is signed [email protected], 2 additional tests (1 that failed) in the latest build of angular.js

[IgorMinar]

I'm sorry, but I wasn't able to verify your CLA signature. CLA signature is required for any code contributions to AngularJS.

Please sign our CLA and ensure that the CLA signature email address and the email address in this PR's commits match.

If you signed the CLA as a corporation, please let me know the company's name.

Thanks a bunch!

PS: If you signed the CLA in the past then most likely the email addresses don't match. Please sign the CLA again or update the email address in the commit of this PR.
PS2: If you are a Googler, please sign the CLA as well to simplify the CLA verification process.

[perek]

I have signed the CLA yesterday and again now to ensure it was correct.

[perek]

Is my CLA verified now?

[perek]

Seems I was using my work alias for this commit, have resigned the CLA

[mary-poppins]

CLA signature verified! Thank you!

Someone from the team will now triage your PR and it will be processed based on the determined priority (doc updates and fixes with tests are prioritized over other changes).

[perek]

Any update/feedback on this?

[caitp]

This is really a feature, not a bugfix, and I think some additional info and probably more testing in addition to review would be a big help.

Looking at the tests, it's not totally clear what is being tested. Anyways, we'll look at this closer in a week or 2

[perek]

Thanks for the response caitp. In previous versions of Angular you could over-ride default headers for $http with either a null value or some new value. It seems in 1.2 a functional header value was supported and according to docs returning a 'null' value from the function should remove a header from the requests even if it is a default header. This is needed for certain functionality, especially for cases where passing an Auth header is unexpected (hmac signed urls). There should be a way to override all defaults via the $http options as this was a feature in previous versions and is currently supported in the documentation. I can update the tests to be more clear.

[perek]

Checking in to see if there is any other info I need to provide.

[perek]

Just checking in again to see if there is anything I can do. Thanks.

[pkozlowski-opensource]

@perek thnx for this PR, I just went over it and I see what you are trying to fix. Your fix is working allright but the code and tests could be simplified, I've left some comments in the code. Could you please have a look at them and push a squashed / amended commit? This will make my life easier when merging.

@caitp I think it is a legit bug / inconsistency that we should merge. Let's chat about it if needed.

[perek]

@pkozlowski-opensource thanks so much for the feedback. I will put something together today.

[perek]

Something happened to your origin comments so I am going to respond here:

if you decide to move the execHeaders call here you can remove the earlier execHeaders(defHeaders); call, no? Since here execHeaders(reqHeaders); will be a result of merging default and per-request headers, so there is no need to execute functions twice. Unless there is a use case-that I'm missing.

I noticed this as well, but was hesitant to change it since it had been done this way for so long.

[perek]

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(n²) to O(n)

[pkozlowski-opensource]

@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?

[perek]

Commits are now squashed, let me know of anything else you need.

[pkozlowski-opensource]

@perek ok, it looks good for me now.

[perek]

👍 let me know if you need help with anything else. Maybe an optimizations PR?

[perek]

Are we good to go?

[perek]

Checking in on this PR. Is it going to get merged?

[IgorMinar]

merged! sorry about the delay. The PR slipped off of our radar.