openstack/horizon
Code Issues Proposed changes

Memoize policy service

Browse Source 
There is a hole in the policy service caching layer
that has something to do with ng-repeat. The angular
http cache service doesn't seem to work within a single
digest cycle or something... so I simplfied and
improved performance by using memoize to
prevent redundant checks which were happening in
hz-dynamic-table.

See bug for details on verifying this manually.

Change-Id: Ib0b3a806d1c6b065e20cb22b8255048bd836dd1b
Closes-Bug: 1608623
This commit is contained in:
Travis Tripp
2016-07-08 17:04:31 -06:00
parent f9ba1ecc2f
commit deda07c4c3
3 changed files with 51 additions and 57 deletions
Download Patch File Download Diff File Expand all files Collapse all files

1
openstack_dashboard/dashboards/identity/static/dashboard/identity/users/table/table.controller.spec.js
View File

@@ -42,6 +42,7 @@
///////////////////////
beforeEach(module('horizon.framework.util.filters'));
beforeEach(module('horizon.framework.util.http'));
beforeEach(module('horizon.framework.util.i18n'));
beforeEach(module('horizon.framework.conf'));

40
openstack_dashboard/static/app/core/openstack-service-api/policy.service.js
View File

@@ -20,8 +20,8 @@
.factory('horizon.app.core.openstack-service-api.policy', PolicyService);
PolicyService.$inject = [
'$cacheFactory',
'$q',
'horizon.framework.util.filters.$memoize',
'horizon.framework.util.http.service',
'horizon.framework.widgets.toast.service'
];
@@ -30,21 +30,18 @@
* @ngdoc service
* @name PolicyService
* @param {Object} $q
* @param {Object} memoize
* @param {Object} apiService
* @param {Object} toastService
* @description Provides a direct pass through to the policy engine in
* Horizon.
* @returns {Object} The service
*/
function PolicyService($cacheFactory, $q, apiService, toastService) {
function PolicyService($q, memoize, apiService, toastService) {
var service = {
cache: $cacheFactory(
'horizon.app.core.openstack-service-api.policy',
{capacity: 200}
),
check: check,
ifAllowed: ifAllowed
check: memoize(check, memoizeHasher),
ifAllowed: memoize(ifAllowed, memoizeHasher)
};
return service;
@@ -95,22 +92,15 @@
// The .error is already overriden in this function to just display toast, so this should
// work the same as if only success was returned.
var deferred = $q.defer();
var cacheId = angular.toJson(policyRules);
var cachedData = service.cache.get(cacheId);
if (cachedData) {
deferred.resolve(cachedData);
} else {
apiService.post('/api/policy/', policyRules)
.success(function successPath(result) {
service.cache.put(cacheId, result);
deferred.resolve(result);
})
.error(function failurePath(result) {
toastService.add('warning', gettext('Policy check failed.'));
deferred.reject(result);
});
}
apiService.post('/api/policy/', policyRules)
.success(function successPath(result) {
deferred.resolve(result);
})
.error(function failurePath(result) {
toastService.add('warning', gettext('Policy check failed.'));
deferred.reject(result);
});
deferred.promise.success = deferred.promise.then;
return deferred.promise;
@@ -147,5 +137,9 @@
}
}
}
function memoizeHasher(policyRules) {
return angular.toJson(policyRules);
}
}
}());

67
openstack_dashboard/static/app/core/openstack-service-api/policy.service.spec.js
View File

@@ -29,6 +29,7 @@
})
);
beforeEach(module('horizon.framework.util.filters'));
beforeEach(module('horizon.app.core.openstack-service-api'));
beforeEach(inject(['horizon.app.core.openstack-service-api.policy', function(policyAPI) {
@@ -69,6 +70,7 @@
////////////////
beforeEach(module('horizon.framework.conf'));
beforeEach(module('horizon.framework.util.filters'));
beforeEach(module('horizon.framework.widgets.toast'));
beforeEach(module('horizon.app.core.openstack-service-api'));
beforeEach(module('horizon.framework.util.http'));
@@ -79,7 +81,6 @@
service = policyAPI;
apiService = _apiService;
$timeout = _$timeout_;
service.cache.removeAll();
}
]));
@@ -89,45 +90,26 @@
expect(service.check).toBeDefined();
});
it("should use the cache if it's populated", function defined() {
service.cache.put(angular.toJson('abcdef'), {mission: 'impossible'});
var data;
function verifyData(x) {
data = x;
}
service.check('abcdef').then(verifyData);
$timeout.flush();
expect(data).toEqual({mission: 'impossible'});
});
it("returns results from api if no cache", function defined() {
var input = 'abcdef';
var successFunc, gotObject;
var retVal = {
success: function(x) {
successFunc = x; return {error: angular.noop};
}
};
spyOn(apiService, 'post').and.returnValue(retVal);
service.check('abcdef').then(function(x) { gotObject = x; });
var spy = spyOn(apiService, 'post').and.returnValue(retVal);
service.check(input).then(function(x) { gotObject = x; });
successFunc({hello: 'there'});
$timeout.flush();
expect(gotObject).toEqual({hello: 'there'});
});
expect(apiService.post).toHaveBeenCalled();
it("sets cache with results from apiService if no cache already", function defined() {
var successFunc;
var retVal = {
success: function(x) {
successFunc = x;
return { error: angular.noop };
}
};
spyOn(apiService, 'post').and.returnValue(retVal);
service.check('abcdef').then(angular.noop);
successFunc({hello: 'there'});
spy.calls.reset();
service.check(input).then(function(x) { gotObject = x; });
$timeout.flush();
expect(service.cache.get(angular.toJson('abcdef'))).toEqual({hello: 'there'});
expect(gotObject).toEqual({hello: 'there'});
expect(apiService.post).not.toHaveBeenCalled();
});
});
@@ -139,6 +121,7 @@
////////////////
beforeEach(module('horizon.framework.conf'));
beforeEach(module('horizon.framework.util.filters'));
beforeEach(module('horizon.framework.widgets.toast'));
beforeEach(module('horizon.app.core.openstack-service-api'));
beforeEach(module('horizon.framework.util.http'));
@@ -158,20 +141,36 @@
expect(service.ifAllowed).toBeDefined();
});
it("rejects when check() resolves with an object without 'allowed'", function() {
it("rejects when check() resolves to 'allowed = false'", function() {
var input = {'expect': 'fail'};
var def = $q.defer();
def.resolve({allowed: false});
spyOn(service, 'check').and.returnValue(def.promise);
service.ifAllowed({}).then(failWhenCalled, passWhenCalled);
var spy = spyOn(service, 'check').and.returnValue(def.promise);
service.ifAllowed(input).then(failWhenCalled, passWhenCalled);
$timeout.flush();
expect(service.check).toHaveBeenCalled();
//Repeat the call with same inputs and expect same result but to be cached
spy.calls.reset();
service.ifAllowed(input).then(failWhenCalled, passWhenCalled);
$timeout.flush();
expect(service.check).not.toHaveBeenCalled();
});
it("passes when check() resolves with an object with 'allowed'", function() {
it("passes when check() resolves to 'allowed = true'", function() {
var input = {'expect': 'pass'};
var def = $q.defer();
def.resolve({allowed: true});
spyOn(service, 'check').and.returnValue(def.promise);
service.ifAllowed({}).then(passWhenCalled, failWhenCalled);
var spy = spyOn(service, 'check').and.returnValue(def.promise);
service.ifAllowed(input).then(passWhenCalled, failWhenCalled);
$timeout.flush();
expect(service.check).toHaveBeenCalled();
//Repeat the call with same inputs and expect same result but to be cached
spy.calls.reset();
service.ifAllowed(input).then(passWhenCalled, failWhenCalled);
$timeout.flush();
expect(service.check).not.toHaveBeenCalled();
});
});