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

fix(http): preserve config object when resolving from cache #9030

Closed
wants to merge 1 commit into from
Closed

fix(http): preserve config object when resolving from cache #9030

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: 4 additions & 2 deletions src/ng/http.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1027,8 +1027,7 @@ function $HttpProvider() {
if (isDefined(cachedResp)) {
if (isPromiseLike(cachedResp)) {
// cached request has already been sent, but there is no response yet
cachedResp.then(removePendingReq, removePendingReq);
return cachedResp;
cachedResp.then(resolvePromiseWithResult, resolvePromiseWithResult);
Copy link

Choose a reason for hiding this comment

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

May it be that the Travis CI is failing because cachedResp is not returned any more here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

travis is just flaky as usual, cachedResp shouldn't be returned here

Copy link

Choose a reason for hiding this comment

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

:)

Copy link
Contributor

Choose a reason for hiding this comment

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

well, it's supposed to return the cached promise... now it's just returning a promise that will never be resolved, and won't even make a new request for it --- so operations on the returned promise are basically pointless.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, how is this "preserving the config object", anyway?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@caitp I think you are reading this wrong... We don't want to return the cached promise since this promise is resolved with the wrong config, this is the bug this PR fixes. Instead, we return a new promise that gets resolved with the correct result once the cached promise is resolved (this happens in resolvePromiseWithResult)

Copy link

Choose a reason for hiding this comment

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

exactly - the problem is that the cached promise returns the cached config which breaks a scenario where developers would add their "requestIds" in config that would never be resolved (the response would contain a previous request id).

unless there's some other way to push something with the request and have it in the callback.

} else {
// serving from cache
if (isArray(cachedResp)) {
Expand Down Expand Up @@ -1106,6 +1105,9 @@ function $HttpProvider() {
});
}

function resolvePromiseWithResult(result) {
resolvePromise(result.data, result.status, shallowCopy(result.headers()), result.statusText);
}

function removePendingReq() {
var idx = $http.pendingRequests.indexOf(config);
Expand Down
52 changes: 52 additions & 0 deletions test/ng/httpSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1352,6 +1352,24 @@ describe('$http', function() {
}));


it('should not share the pending cached headers object instance', inject(function($rootScope) {
var firstResult;
callback.andCallFake(function(result) {
expect(result.headers()).toEqual(firstResult.headers());
expect(result.headers()).not.toBe(firstResult.headers());
});

$httpBackend.expect('GET', '/url').respond(200, 'content', {'content-encoding': 'gzip', 'server': 'Apache'});
$http({method: 'GET', url: '/url', cache: cache}).then(function(result) {
firstResult = result;
});
$http({method: 'GET', url: '/url', cache: cache}).then(callback);
$httpBackend.flush();

expect(callback).toHaveBeenCalledOnce();
}));


it('should cache status code as well', inject(function($rootScope) {
doFirstCacheRequest('GET', 201);
callback.andCallFake(function(r, status, h) {
Expand Down Expand Up @@ -1381,6 +1399,40 @@ describe('$http', function() {
});


it('should preserve config object when resolving from cache', function() {
$httpBackend.expect('GET', '/url').respond(200, 'content');
$http({method: 'GET', url: '/url', cache: cache, headers: {foo: 'bar'}});
$httpBackend.flush();

$http({method: 'GET', url: '/url', cache: cache, headers: {foo: 'baz'}}).then(callback);
$rootScope.$digest();

expect(callback.mostRecentCall.args[0].config.headers.foo).toBe('baz');
});


it('should preserve config object when resolving from pending cache', function() {
$httpBackend.expect('GET', '/url').respond(200, 'content');
$http({method: 'GET', url: '/url', cache: cache, headers: {foo: 'bar'}});

$http({method: 'GET', url: '/url', cache: cache, headers: {foo: 'baz'}}).then(callback);
$httpBackend.flush();

expect(callback.mostRecentCall.args[0].config.headers.foo).toBe('baz');
});


it('should preserve config object when rejecting from pending cache', function() {
$httpBackend.expect('GET', '/url').respond(404, 'content');
$http({method: 'GET', url: '/url', cache: cache, headers: {foo: 'bar'}});

$http({method: 'GET', url: '/url', cache: cache, headers: {foo: 'baz'}}).catch(callback);
$httpBackend.flush();

expect(callback.mostRecentCall.args[0].config.headers.foo).toBe('baz');
});


it('should allow the cached value to be an empty string', function() {
cache.put('/abc', '');

Expand Down