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

fix($http): ensure case-insens. header overriding #2105

Closed
wants to merge 3 commits into from

Conversation

caiotoon
Copy link
Contributor

@caiotoon caiotoon commented Mar 6, 2013

If user send content-type header, both content-type and default
Content-Type headers were sent. Now default header overriding is
case-insensitive.

As per RFC 2616, "[header] Field
names are case-insensitive".

Before this fix, the following test would fail:

it('should override default headers with custom in a case insensitive manner', function() {
  $httpBackend.expect('POST', '/url', 'messageBody', function(headers) {
    return headers['accept'] == 'Rewritten' &&
           headers['content-type'] == 'Rewritten' &&
           headers['Accept'] === undefined &&
           headers['Content-Type'] === undefined;
  }).respond('');

  $http({url: '/url', method: 'POST', data: 'messageBody', headers: {
    'accept': 'Rewritten',
    'content-type': 'Rewritten'
  }});
  $httpBackend.flush();
});

@pkozlowski-opensource
Copy link
Member

This is valuable change and improves existing functionality but in terms of implementation we could probably save some bytes. @caiotoon could you try to have another implementation and see if this couldn't be simplified / made smaller? Let me know if you need any help.

@pkozlowski-opensource
Copy link
Member

  • Contributor signed CLA now or in the past (if you just signed, leave a comment here with your real name for cross reference)
  • Feature improves existing core functionality
  • API is compatible with existing Angular apis and relevant standards (if applicable)
  • PR doesn't contain a breaking change
  • PR contains unit tests
  • PR contains e2e tests (if suitable)
  • PR contains documentation update
  • PR passes all tests on Travis (sanity)
  • PR passes all tests on ci.angularjs.org (cross-browser compatibility)
  • PR is rebased against recent master
  • PR is squashed into one commit per logical change
  • PR's commit messages are descriptive and allows us to autogenerate release notes (required [commit message

@pkozlowski-opensource
Copy link
Member

@caiotoon You will need to sign the CLA as well.

@caiotoon
Copy link
Contributor Author

caiotoon commented Mar 9, 2013

Hi, @pkozlowski-opensource. Made it a bit smaller at the trade off of performance at some unnecessary loops (as long as I saw, it's not possible to interrupt a forEach). Also removed some variables and a function declaration. Do you have any other suggestion that could make the code smaller without changing code interface?

Regarding the pendencies,

  • CLA: Had already signed the CLA. Name is Caio Gomes Cunha.
  • PR contains documentation update: there is no need for doc update, as this is the expected behavior. Am I right?
  • PR passes all tests on ci.angularjs.org...: it did passed all tests for me (except for those that were already failing on my machine at master code), which are the failing tests?
  • PR's commit messages are descriptive...: how could I make it better? I tried to following commit message convetions. And how would I change it? Should I delete the branch, recreate and pull a request once again?

Thanks!

@IgorMinar
Copy link
Contributor

this PR conflicts with bigger changes in #2130

we are going to land #2130 likely today, I'll ping this PR once we do. can you then rebase your PR please?

@caiotoon
Copy link
Contributor Author

Sure. Just let me know.

@ghost ghost assigned IgorMinar Mar 15, 2013
@caiotoon
Copy link
Contributor Author

@IgorMinar any update on this one?

@petebacondarwin
Copy link
Contributor

@caiotoon - #2130 has landed. Can you try to rebase your chanages off master and then force a push back to this branch?

@caiotoon
Copy link
Contributor Author

@petebacondarwin rebased and resolved. I'm not pretty sure if I did everything right, feel free to instruct further.

@caiotoon
Copy link
Contributor Author

I'm not pretty sure, but I believe I did something wrong while rebasing and other changes are being included in this commit as it was mine. I'll try to figure out where I messed up rebasing and will commit again. I'll post here when I believe this pull is ready to be accepted.

caiotoon and others added 3 commits June 3, 2013 13:52
If user send content-type header, both content-type and default
Content-Type headers were sent. Now default header overriding is
case-insensitive.
Refactored the code to save some bytes.
@caiotoon
Copy link
Contributor Author

caiotoon commented Jun 4, 2013

@petebacondarwin it's now properly rebased and resolved. Ready to merge.

@petebacondarwin
Copy link
Contributor

Merged to master at 53359d5.
Just reworking into stable...

@petebacondarwin
Copy link
Contributor

And landed in stable as 25d9f5a

@petebacondarwin
Copy link
Contributor

Thanks @caiotoon for your work and patience!

@caiotoon caiotoon deleted the http-case-insensitive branch June 20, 2013 12:45
@caiotoon
Copy link
Contributor Author

Nice! No problem.

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

Successfully merging this pull request may close these issues.

4 participants