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

feat($http): provide a config object as an argument to header functions #10622

Closed
wants to merge 1 commit into from
Closed

feat($http): provide a config object as an argument to header functions #10622

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
8 changes: 4 additions & 4 deletions src/ng/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -615,7 +615,7 @@ function $HttpProvider() {
* - **data** – `{string|Object}` – Data to be sent as the request message data.
* - **headers** – `{Object}` – Map of strings or functions which return strings representing
* HTTP headers to send to the server. If the return value of a function is null, the
* header will not be sent.
* header will not be sent. Functions accept a config object as an argument.
* - **xsrfHeaderName** – `{string}` – Name of HTTP header to populate with the XSRF token.
* - **xsrfCookieName** – `{string}` – Name of cookie containing the XSRF token.
* - **transformRequest** –
Expand Down Expand Up @@ -834,12 +834,12 @@ function $HttpProvider() {
: $q.reject(resp);
}

function executeHeaderFns(headers) {
function executeHeaderFns(headers, config) {
var headerContent, processedHeaders = {};

forEach(headers, function(headerFn, header) {
if (isFunction(headerFn)) {
headerContent = headerFn();
headerContent = headerFn(config);
if (headerContent != null) {
processedHeaders[header] = headerContent;
}
Expand Down Expand Up @@ -873,7 +873,7 @@ function $HttpProvider() {
}

// execute if header value is a function for merged headers
return executeHeaderFns(reqHeaders);
return executeHeaderFns(reqHeaders, shallowCopy(config));
}
}

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

it('should expose a config object to header functions', function() {
var config = {
foo: 'Rewritten',
headers: {'Accept': function(config) {
return config.foo;
}}
};

$httpBackend.expect('GET', '/url', undefined, {Accept: 'Rewritten'}).respond('');
$http.get('/url', config);
$httpBackend.flush();
});

it('should not allow modifications to a config object in header functions', function() {
Copy link
Contributor

Choose a reason for hiding this comment

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

should ignore modifications (but no big deal)

Copy link
Contributor

Choose a reason for hiding this comment

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

this will also only "ignore" changes to root properties of the config object, so changes to child object properties won't be ignored :( but I don't think it's worth worrying about it until someone complains

Copy link
Member Author

Choose a reason for hiding this comment

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

yeh... but I agree that we shouldn't worry about it too much right now...

var config = {
headers: {'Accept': function(config) {
config.foo = 'bar';
return 'Rewritten';
}}
};

$httpBackend.expect('GET', '/url', undefined, {Accept: 'Rewritten'}).respond('');
$http.get('/url', config);
$httpBackend.flush();

expect(config.foo).toBeUndefined();
});

Copy link
Contributor

Choose a reason for hiding this comment

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

should we also test the main functionality of this feature? :p

Copy link
Member Author

Choose a reason for hiding this comment

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

@caitp it is tested as otherwise a mocked request would fail, no?

Copy link
Contributor

Choose a reason for hiding this comment

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

let me check ngMock one sec... But in spite of that, I think you should have a different spec for that, because it's not explicitly testing that, it's explicitly testing that the passed in config object is a shallow copy

Copy link
Member Author

Choose a reason for hiding this comment

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

Yep, I thought of splitting it into 2 separate specs when I saw your comment. Will create 2 separate specs.

it('should check the cache before checking the XSRF cookie', inject(function($browser, $cacheFactory) {
var testCache = $cacheFactory('testCache'),
executionOrder = [];
Expand Down