diff options
author | Travis Tripp <travis.tripp@hpe.com> | 2016-07-08 17:04:31 -0600 |
---|---|---|
committer | Travis Tripp <travis.tripp@hpe.com> | 2016-08-01 19:04:09 -0600 |
commit | deda07c4c35c1bc5a80a76c0e8041a8a4bd57874 (patch) | |
tree | 94f3cebf9d81b8b0c56ffec509669dbfb81a02a3 | |
parent | f9ba1ecc2f47a2b3fd2816dfba62f8ab9eaf343f (diff) | |
download | horizon-deda07c4c35c1bc5a80a76c0e8041a8a4bd57874.tar.gz |
Memoize policy service
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
3 files changed, 51 insertions, 57 deletions
diff --git a/openstack_dashboard/dashboards/identity/static/dashboard/identity/users/table/table.controller.spec.js b/openstack_dashboard/dashboards/identity/static/dashboard/identity/users/table/table.controller.spec.js index 3a186df8b..c62801943 100644 --- a/openstack_dashboard/dashboards/identity/static/dashboard/identity/users/table/table.controller.spec.js +++ b/openstack_dashboard/dashboards/identity/static/dashboard/identity/users/table/table.controller.spec.js @@ -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')); diff --git a/openstack_dashboard/static/app/core/openstack-service-api/policy.service.js b/openstack_dashboard/static/app/core/openstack-service-api/policy.service.js index 479e0c9ef..9e955b9d8 100644 --- a/openstack_dashboard/static/app/core/openstack-service-api/policy.service.js +++ b/openstack_dashboard/static/app/core/openstack-service-api/policy.service.js @@ -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); + } } }()); diff --git a/openstack_dashboard/static/app/core/openstack-service-api/policy.service.spec.js b/openstack_dashboard/static/app/core/openstack-service-api/policy.service.spec.js index 0b1266265..dc637469e 100644 --- a/openstack_dashboard/static/app/core/openstack-service-api/policy.service.spec.js +++ b/openstack_dashboard/static/app/core/openstack-service-api/policy.service.spec.js @@ -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(); }); }); |