fix(roles-sync): fix CallableCache's NPE exception for caching synchronization strategy (#1077) (#1080)

* fix(roles-sync): fix callable cache NPE exception for caching synchronization strategy

* fix(roles-sync): added tests

---------

Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
(cherry picked from commit 6d29188)

Co-authored-by: Kiran Godishala <53332225+kirangodishala@users.noreply.github.com>

Author: mergify[bot]

fiat-roles/src/main/java/com/netflix/spinnaker/fiat/roles/CallableCache.java

@@ -57,6 +57,9 @@ Future<Result> runAndGetResult(Key key, Callable<Result> callable) {
   }
 
   void clear(Key key) {
+    if (key == null) {
+      return;
+    }
     try {
       lock.lock();
       var future = cache.get(key);

fiat-roles/src/main/java/com/netflix/spinnaker/fiat/roles/UserRolesSyncStrategy.java

@@ -16,6 +16,7 @@
 
 package com.netflix.spinnaker.fiat.roles;
 
+import java.util.ArrayList;
 import java.util.List;
 import lombok.NonNull;
 import lombok.RequiredArgsConstructor;
@@ -61,15 +62,16 @@ public CachedSynchronizationStrategy(
 
     @Override
     public long syncAndReturn(List<String> roles) {
+      final List<String> nonNullRoles = (roles != null) ? roles : new ArrayList<>();
       try {
         return this.callableCache
-            .runAndGetResult(roles, () -> this.synchronizer.syncAndReturn(roles))
+            .runAndGetResult(nonNullRoles, () -> this.synchronizer.syncAndReturn(nonNullRoles))
             .get();
       } catch (Exception e) {
         log.error(e.getMessage());
         throw new RolesSynchronizationException();
       } finally {
-        this.callableCache.clear(roles);
+        this.callableCache.clear(nonNullRoles);
       }
     }
 

fiat-roles/src/test/groovy/com/netflix/spinnaker/fiat/roles/CachedSynchronizationSpec.groovy

@@ -75,4 +75,18 @@ class CachedSynchronizationSpec extends Specification {
         then:
         1 * mockedCache.clear(["rolea", "roleb"])
     }
+
+    def "should not fail when the roles list is null"(){
+        setup:
+        def mockedSynchronizer = Mock(Synchronizer)
+        mockedSynchronizer.syncAndReturn(_ as List<String>) >> 1L
+        def cachedSynchronization = new CachedSynchronizationStrategy(mockedSynchronizer)
+
+        when:
+        cachedSynchronization.syncAndReturn(null)
+
+        then:
+        noExceptionThrown()
+    }
+
 }

fiat-roles/src/test/groovy/com/netflix/spinnaker/fiat/roles/CallableCacheSpec.groovy

@@ -66,6 +66,16 @@ class CallableCacheSpec extends Specification {
         firstExecution != secondExecution
     }
 
+    def "should not throw exception when null key is supplied to clear"() {
+        setup:
+        def cache = new CallableCache<String, Long>()
+        when:
+        cache.clear(null)
+
+        then:
+        noExceptionThrown()
+    }
+
     def returnValueCallable() {
         return { -> 1L }
     }