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

fix(cacheFactory): check key exists before decreasing size #12329

Closed
wants to merge 1 commit into from
Closed

fix(cacheFactory): check key exists before decreasing size #12329

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
11 changes: 6 additions & 5 deletions src/ng/cacheFactory.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -93,9 +93,9 @@ function $CacheFactoryProvider() {

var size = 0,
stats = extend({}, options, {id: cacheId}),
data = {},
data = createMap(),
capacity = (options && options.capacity) || Number.MAX_VALUE,
lruHash = {},
lruHash = createMap(),
freshEnd = null,
staleEnd = null;

Expand Down Expand Up @@ -223,6 +223,8 @@ function $CacheFactoryProvider() {
delete lruHash[key];
}

if (!(key in data)) return;
Copy link
Member

Choose a reason for hiding this comment

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

This will still has unexpected results if key is prototypally inherited by data.
You could use !data.hasOwnProperty(key). It might also be possible to use data = createMap() and not have to deal with the prototype chain (but I haven't checked if more modifications are needed in other places).

Copy link
Contributor

Choose a reason for hiding this comment

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

I think that the simplest and best of the two options would be to do data = createMap()

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Is that literally what this line should be? Sorry, this is my first time contributing here and I'm having trouble finding anything about a createMap() function. Is this just pseudo-code, or something internal and already available?

Copy link
Contributor

Choose a reason for hiding this comment

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

createMap is defined in src/Angular.js. It just performs Object.create(null); to build an object with a null prototype

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, thanks. Could I get a bit more guidance as to how I should make use of createMap() in this case, though? If I'm setting data = createMap(), wouldn't that effectively empty the data object?

Copy link
Contributor

Choose a reason for hiding this comment

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

Change https://github.com/angular/angular.js/blob/master/src/ng/cacheFactory.js#L96 and https://github.com/angular/angular.js/blob/master/src/ng/cacheFactory.js#L240 to data = createMap();


delete data[key];
size--;
},
Expand All @@ -237,9 +239,9 @@ function $CacheFactoryProvider() {
* Clears the cache object of any entries.
*/
removeAll: function() {
data = {};
data = createMap();
size = 0;
lruHash = {};
lruHash = createMap();
freshEnd = staleEnd = null;
},

Expand Down Expand Up @@ -399,4 +401,3 @@ function $TemplateCacheProvider() {
return $cacheFactory('templates');
}];
}

13 changes: 13 additions & 0 deletions test/ng/cacheFactorySpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,19 @@ describe('$cacheFactory', function() {
expect(cache.info().size).toBe(0);
}));

it('should only decrement size when an element is actually removed via remove', inject(function($cacheFactory) {
cache.put('foo', 'bar');
expect(cache.info().size).toBe(1);

cache.remove('undefined');
Copy link
Contributor

Choose a reason for hiding this comment

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

would it be possible to add a case that the key is hasOwnProperty (or some other property that is standard to Object.prototype?

expect(cache.info().size).toBe(1);

cache.remove('hasOwnProperty');
expect(cache.info().size).toBe(1);

cache.remove('foo');
expect(cache.info().size).toBe(0);
}));

it('should return cache id', inject(function($cacheFactory) {
expect(cache.info().id).toBe('test');
Expand Down