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();
     });
   });