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

fix($templateRequest): Handle empty templates in cache correctly. #14496

Closed
Closed
Show file tree
Hide file tree
Changes from 1 commit
Commits
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: 7 additions & 1 deletion src/ng/templateRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -68,8 +68,14 @@ function $TemplateRequestProvider() {
// are included in there. This also makes Angular accept any script
// directive, no matter its name. However, we still need to unwrap trusted
// types.
if (!isString(tpl) || !$templateCache.get(tpl)) {
if (!isString(tpl)) { // Safe types need unwrapping.
Copy link
Member

Choose a reason for hiding this comment

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

Couldn't you just change this line to ... || isUndefined($templateCache.get(tpl))... and leave everythng else unchanged ?

Copy link
Contributor Author

@rjamet rjamet Apr 22, 2016

Choose a reason for hiding this comment

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

The reasoning is that people asked for wrong fixes several times, so my original code was probably confusing. I've unrolled the if to accomodate clearer comments, but that can be reverted if you prefer.

Edit : No point making this longer than needed, actually. Reverted to the simpler version.

tpl = $sce.getTrustedResourceUrl(tpl);
} else { // Strings can be tested against the cache.
var cacheAnswer = $templateCache.get(tpl);
if (typeof cacheAnswer === 'undefined') {
// It is not in the cache, hence we don't trust it and it has to go through the $sce.
tpl = $sce.getTrustedResourceUrl(tpl);
} // Anything else means the cache has a record of it, so no need to check.
}

var transformResponse = $http.defaults && $http.defaults.transformResponse;
Expand Down
31 changes: 31 additions & 0 deletions test/ng/templateRequestSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,37 @@ describe('$templateRequest', function() {
}).not.toThrow();
}));

it('should accept empty templates and refuse null or undefined templates in cache',
inject(function($rootScope, $templateRequest, $templateCache, $sce) {

// Will throw on any template not in cache.
spyOn($sce, 'getTrustedResourceUrl').and.returnValue(false);

expect(function() {
$templateRequest('tpl.html'); // should go through $sce
$rootScope.$digest();
}).toThrow();

$templateCache.put('tpl.html'); // is a no-op, so $sce check as well.
expect(function() {
$templateRequest('tpl.html');
$rootScope.$digest();
}).toThrow();

$templateCache.put('tpl.html', null); // makes no sense, but it's been added, so trust it.
expect(function() {
$templateRequest('tpl.html');
$rootScope.$digest();
}).not.toThrow();

$templateCache.put('tpl.html', ''); // should work (empty template)
Copy link
Member

Choose a reason for hiding this comment

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

Since you have already called $templateCache.put() previously, this test is not so reliable.
I would either empty the cache first ot use the they helper to make each case its own testcase.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yup, good point. Fixed with deleteAll after each of the blocks.

expect(function() {
$templateRequest('tpl.html');
$rootScope.$digest();
}).not.toThrow();

}));

it('should keep track of how many requests are going on',
inject(function($rootScope, $templateRequest, $httpBackend) {

Expand Down