Sonar fixes for policy-core locks

Made a number of changes to the locking code in policy-core, to address
sonar issues.  This entaileed changing some of the Lock API methods to
return OperResult instead of Boolean.
Updated distributed locking with the new API return types.
Simplified Thread creation using functional methods.

Change-Id: If32bf7a435d2aedb969de1b77c7e7e27e110ecb0
Issue-ID: POLICY-728
Signed-off-by: Jim Hahn <jrh3@att.com>

5 files changed, 69 insertions, 109 deletions

diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockRequestFutureTest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockRequestFutureTest.java
index 883778eb..2dd90f00 100644
--- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockRequestFutureTest.java
+++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockRequestFutureTest.java
@@ -80,13 +80,13 @@ public class LockRequestFutureTest {
     public void testLockRequestFutureStringStringBoolean_ArgEx() throws Exception {
 
         // null resource id
-        IllegalArgumentException ex = expectException(IllegalArgumentException.class,
-                        xxx -> new LockRequestFuture(null, OWNER, true));
+        IllegalArgumentException ex =
+                        expectException(IllegalArgumentException.class, () -> new LockRequestFuture(null, OWNER, true));
         assertEquals("null resourceId", ex.getMessage());
 
 
         // null owner
-        ex = expectException(IllegalArgumentException.class, xxx -> new LockRequestFuture(RESOURCE, null, true));
+        ex = expectException(IllegalArgumentException.class, () -> new LockRequestFuture(RESOURCE, null, true));
         assertEquals("null owner", ex.getMessage());
     }
 
@@ -108,12 +108,12 @@ public class LockRequestFutureTest {
 
         // null resource id
         IllegalArgumentException ex = expectException(IllegalArgumentException.class,
-                        xxx -> new LockRequestFuture(null, OWNER, callback));
+                        () -> new LockRequestFuture(null, OWNER, callback));
         assertEquals("null resourceId", ex.getMessage());
 
 
         // null owner
-        ex = expectException(IllegalArgumentException.class, xxx -> new LockRequestFuture(RESOURCE, null, callback));
+        ex = expectException(IllegalArgumentException.class, () -> new LockRequestFuture(RESOURCE, null, callback));
         assertEquals("null owner", ex.getMessage());
 
 
@@ -141,7 +141,7 @@ public class LockRequestFutureTest {
         assertTrue(fut.isDone());
 
         // should not block now
-        expectException(CancellationException.class, xxx -> fut.get(0, TimeUnit.SECONDS));
+        expectException(CancellationException.class, () -> fut.get(0, TimeUnit.SECONDS));
 
     }
 
@@ -153,7 +153,7 @@ public class LockRequestFutureTest {
         assertFalse(fut.cancel(true));
         assertTrue(fut.isDone());
 
-        expectException(CancellationException.class, xxx -> fut.get(0, TimeUnit.SECONDS));
+        expectException(CancellationException.class, () -> fut.get(0, TimeUnit.SECONDS));
     }
 
     @Test
@@ -201,7 +201,7 @@ public class LockRequestFutureTest {
         assertFalse(fut.setLocked(false));
 
         assertTrue(fut.isDone());
-        expectException(CancellationException.class, xxx -> fut.get(0, TimeUnit.SECONDS));
+        expectException(CancellationException.class, () -> fut.get(0, TimeUnit.SECONDS));
     }
 
     @Test
@@ -296,43 +296,25 @@ public class LockRequestFutureTest {
 
     @Test
     public void testGet_Cancelled() throws Exception {
-        new Thread() {
-            @Override
-            public void run() {
-                fut.cancel(false);
-            }
-        }.start();
-
-        expectException(CancellationException.class, xxx -> fut.get());
+        new Thread(() -> fut.cancel(false)).start();
+        expectException(CancellationException.class, () -> fut.get());
     }
 
     @Test
     public void testGet_Acquired() throws Exception {
-        new Thread() {
-            @Override
-            public void run() {
-                fut.setLocked(true);
-            }
-        }.start();
-
+        new Thread(() -> fut.setLocked(true)).start();
         assertTrue(fut.get());
     }
 
     @Test
     public void testGet_Denied() throws Exception {
-        new Thread() {
-            @Override
-            public void run() {
-                fut.setLocked(false);
-            }
-        }.start();
-
+        new Thread(() -> fut.setLocked(false)).start();
         assertFalse(fut.get());
     }
 
     @Test
     public void testGetLongTimeUnit() throws Exception {
-        expectException(TimeoutException.class, xxx -> fut.get(0, TimeUnit.SECONDS));
+        expectException(TimeoutException.class, () -> fut.get(0, TimeUnit.SECONDS));
 
         fut.setLocked(true);
         assertTrue(fut.get(0, TimeUnit.SECONDS));
@@ -340,43 +322,25 @@ public class LockRequestFutureTest {
 
     @Test
     public void testGetLongTimeUnit_Timeout() throws Exception {
-        expectException(TimeoutException.class, xxx -> fut.get(0, TimeUnit.SECONDS));
-        expectException(TimeoutException.class, xxx -> fut.get(2, TimeUnit.MILLISECONDS));
+        expectException(TimeoutException.class, () -> fut.get(0, TimeUnit.SECONDS));
+        expectException(TimeoutException.class, () -> fut.get(2, TimeUnit.MILLISECONDS));
     }
 
     @Test
     public void testGetLongTimeUnit_Cancelled() throws Exception {
-        new Thread() {
-            @Override
-            public void run() {
-                fut.cancel(false);
-            }
-        }.start();
-
-        expectException(CancellationException.class, xxx -> fut.get(WAIT_SEC, TimeUnit.SECONDS));
+        new Thread(() -> fut.cancel(false)).start();
+        expectException(CancellationException.class, () -> fut.get(WAIT_SEC, TimeUnit.SECONDS));
     }
 
     @Test
     public void testGetLongTimeUnit_Acquired() throws Exception {
-        new Thread() {
-            @Override
-            public void run() {
-                fut.setLocked(true);
-            }
-        }.start();
-
+        new Thread(() -> fut.setLocked(true)).start();
         assertTrue(fut.get(WAIT_SEC, TimeUnit.SECONDS));
     }
 
     @Test
     public void testGetLongTimeUnit_Denied() throws Exception {
-        new Thread() {
-            @Override
-            public void run() {
-                fut.setLocked(false);
-            }
-        }.start();
-
+        new Thread(() -> fut.setLocked(false)).start();
         assertFalse(fut.get(WAIT_SEC, TimeUnit.SECONDS));
     }
 
diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPITest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPITest.java
index 5a826bb7..57adc847 100644
--- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPITest.java
+++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockFeatureAPITest.java
@@ -20,10 +20,12 @@
 
 package org.onap.policy.drools.core.lock;
 
+import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
 import org.junit.Before;
 import org.junit.Test;
+import org.onap.policy.drools.core.lock.PolicyResourceLockFeatureAPI.OperResult;
 
 public class PolicyResourceLockFeatureAPITest {
 
@@ -55,7 +57,7 @@ public class PolicyResourceLockFeatureAPITest {
 
     @Test
     public void testBeforeUnlock() {
-        assertNull(api.beforeUnlock(RESOURCE_ID, OWNER));
+        assertEquals(OperResult.OPER_UNHANDLED, api.beforeUnlock(RESOURCE_ID, OWNER));
     }
 
     @Test
@@ -66,11 +68,11 @@ public class PolicyResourceLockFeatureAPITest {
 
     @Test
     public void testBeforeIsLocked() {
-        assertNull(api.beforeIsLocked(RESOURCE_ID));
+        assertEquals(OperResult.OPER_UNHANDLED, api.beforeIsLocked(RESOURCE_ID));
     }
 
     @Test
     public void testBeforeIsLockedBy() {
-        assertNull(api.beforeIsLockedBy(RESOURCE_ID, OWNER));
+        assertEquals(OperResult.OPER_UNHANDLED, api.beforeIsLockedBy(RESOURCE_ID, OWNER));
     }
 }
diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockManagerTest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockManagerTest.java
index 2f8b5f83..fe883acf 100644
--- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockManagerTest.java
+++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/PolicyResourceLockManagerTest.java
@@ -42,6 +42,7 @@ import org.junit.Before;
 import org.junit.BeforeClass;
 import org.junit.Test;
 import org.onap.policy.drools.core.lock.PolicyResourceLockFeatureAPI.Callback;
+import org.onap.policy.drools.core.lock.PolicyResourceLockFeatureAPI.OperResult;
 import org.onap.policy.drools.core.lock.PolicyResourceLockManager.Factory;
 
 public class PolicyResourceLockManagerTest {
@@ -111,9 +112,9 @@ public class PolicyResourceLockManagerTest {
      */
     private void initImplementer(PolicyResourceLockFeatureAPI impl) {
         when(impl.beforeLock(anyString(), anyString(), any(Callback.class))).thenReturn(null);
-        when(impl.beforeUnlock(anyString(), anyString())).thenReturn(null);
-        when(impl.beforeIsLocked(anyString())).thenReturn(null);
-        when(impl.beforeIsLockedBy(anyString(), anyString())).thenReturn(null);
+        when(impl.beforeUnlock(anyString(), anyString())).thenReturn(OperResult.OPER_UNHANDLED);
+        when(impl.beforeIsLocked(anyString())).thenReturn(OperResult.OPER_UNHANDLED);
+        when(impl.beforeIsLockedBy(anyString(), anyString())).thenReturn(OperResult.OPER_UNHANDLED);
     }
 
     @Test
@@ -147,10 +148,10 @@ public class PolicyResourceLockManagerTest {
     @Test
     public void testLock_ArgEx() {
         IllegalArgumentException ex =
-                        expectException(IllegalArgumentException.class, xxx -> mgr.lock(null, OWNER1, callback1));
+                        expectException(IllegalArgumentException.class, () -> mgr.lock(null, OWNER1, callback1));
         assertEquals(NULL_RESOURCE_ID, ex.getMessage());
 
-        ex = expectException(IllegalArgumentException.class, xxx -> mgr.lock(RESOURCE_A, null, callback1));
+        ex = expectException(IllegalArgumentException.class, () -> mgr.lock(RESOURCE_A, null, callback1));
         assertEquals(NULL_OWNER, ex.getMessage());
 
         // this should not throw an exception
@@ -250,10 +251,10 @@ public class PolicyResourceLockManagerTest {
 
     @Test
     public void testUnlock_ArgEx() {
-        IllegalArgumentException ex = expectException(IllegalArgumentException.class, xxx -> mgr.unlock(null, OWNER1));
+        IllegalArgumentException ex = expectException(IllegalArgumentException.class, () -> mgr.unlock(null, OWNER1));
         assertEquals(NULL_RESOURCE_ID, ex.getMessage());
 
-        ex = expectException(IllegalArgumentException.class, xxx -> mgr.unlock(RESOURCE_A, null));
+        ex = expectException(IllegalArgumentException.class, () -> mgr.unlock(RESOURCE_A, null));
         assertEquals(NULL_OWNER, ex.getMessage());
     }
 
@@ -263,7 +264,7 @@ public class PolicyResourceLockManagerTest {
         mgr.lock(RESOURCE_A, OWNER1, null);
 
         // have impl1 intercept
-        when(impl1.beforeUnlock(RESOURCE_A, OWNER1)).thenReturn(true);
+        when(impl1.beforeUnlock(RESOURCE_A, OWNER1)).thenReturn(OperResult.OPER_ACCEPTED);
 
         assertTrue(mgr.unlock(RESOURCE_A, OWNER1));
 
@@ -280,7 +281,7 @@ public class PolicyResourceLockManagerTest {
         mgr.lock(RESOURCE_A, OWNER1, null);
 
         // have impl1 intercept
-        when(impl1.beforeUnlock(RESOURCE_A, OWNER1)).thenReturn(false);
+        when(impl1.beforeUnlock(RESOURCE_A, OWNER1)).thenReturn(OperResult.OPER_DENIED);
 
         assertFalse(mgr.unlock(RESOURCE_A, OWNER1));
 
@@ -365,7 +366,7 @@ public class PolicyResourceLockManagerTest {
 
     @Test
     public void testIsLocked_ArgEx() {
-        IllegalArgumentException ex = expectException(IllegalArgumentException.class, xxx -> mgr.isLocked(null));
+        IllegalArgumentException ex = expectException(IllegalArgumentException.class, () -> mgr.isLocked(null));
         assertEquals(NULL_RESOURCE_ID, ex.getMessage());
     }
 
@@ -373,7 +374,7 @@ public class PolicyResourceLockManagerTest {
     public void testIsLocked_BeforeIntercepted_True() {
 
         // have impl1 intercept
-        when(impl1.beforeIsLocked(RESOURCE_A)).thenReturn(true);
+        when(impl1.beforeIsLocked(RESOURCE_A)).thenReturn(OperResult.OPER_ACCEPTED);;
 
         assertTrue(mgr.isLocked(RESOURCE_A));
 
@@ -388,7 +389,7 @@ public class PolicyResourceLockManagerTest {
         mgr.lock(RESOURCE_A, OWNER1, null);
 
         // have impl1 intercept
-        when(impl1.beforeIsLocked(RESOURCE_A)).thenReturn(false);
+        when(impl1.beforeIsLocked(RESOURCE_A)).thenReturn(OperResult.OPER_DENIED);
 
         assertFalse(mgr.isLocked(RESOURCE_A));
 
@@ -420,10 +421,10 @@ public class PolicyResourceLockManagerTest {
     @Test
     public void testIsLockedBy_ArgEx() {
         IllegalArgumentException ex =
-                        expectException(IllegalArgumentException.class, xxx -> mgr.isLockedBy(null, OWNER1));
+                        expectException(IllegalArgumentException.class, () -> mgr.isLockedBy(null, OWNER1));
         assertEquals(NULL_RESOURCE_ID, ex.getMessage());
 
-        ex = expectException(IllegalArgumentException.class, xxx -> mgr.isLockedBy(RESOURCE_A, null));
+        ex = expectException(IllegalArgumentException.class, () -> mgr.isLockedBy(RESOURCE_A, null));
         assertEquals(NULL_OWNER, ex.getMessage());
     }
 
@@ -431,7 +432,7 @@ public class PolicyResourceLockManagerTest {
     public void testIsLockedBy_BeforeIntercepted_True() {
 
         // have impl1 intercept
-        when(impl1.beforeIsLockedBy(RESOURCE_A, OWNER1)).thenReturn(true);
+        when(impl1.beforeIsLockedBy(RESOURCE_A, OWNER1)).thenReturn(OperResult.OPER_ACCEPTED);;
 
         assertTrue(mgr.isLockedBy(RESOURCE_A, OWNER1));
 
@@ -446,7 +447,7 @@ public class PolicyResourceLockManagerTest {
         mgr.lock(RESOURCE_A, OWNER1, null);
 
         // have impl1 intercept
-        when(impl1.beforeIsLockedBy(RESOURCE_A, OWNER1)).thenReturn(false);
+        when(impl1.beforeIsLockedBy(RESOURCE_A, OWNER1)).thenReturn(OperResult.OPER_DENIED);
 
         assertFalse(mgr.isLockedBy(RESOURCE_A, OWNER1));
 
@@ -479,7 +480,7 @@ public class PolicyResourceLockManagerTest {
 
     @Test
     public void testDoIntercept_Impl1() {
-        when(impl1.beforeIsLocked(RESOURCE_A)).thenReturn(true);
+        when(impl1.beforeIsLocked(RESOURCE_A)).thenReturn(OperResult.OPER_ACCEPTED);;
 
         assertTrue(mgr.isLocked(RESOURCE_A));
 
@@ -489,7 +490,7 @@ public class PolicyResourceLockManagerTest {
 
     @Test
     public void testDoIntercept_Impl2() {
-        when(impl2.beforeIsLocked(RESOURCE_A)).thenReturn(true);
+        when(impl2.beforeIsLocked(RESOURCE_A)).thenReturn(OperResult.OPER_ACCEPTED);;
 
         assertTrue(mgr.isLocked(RESOURCE_A));
 
diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/SimpleLockManagerTest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/SimpleLockManagerTest.java
index 6abc5bf9..833e1c7d 100644
--- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/SimpleLockManagerTest.java
+++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/SimpleLockManagerTest.java
@@ -98,10 +98,10 @@ public class SimpleLockManagerTest {
     @Test
     public void testLock_ArgEx() {
         IllegalArgumentException ex =
-                        expectException(IllegalArgumentException.class, xxx -> mgr.lock(null, OWNER1, null));
+                        expectException(IllegalArgumentException.class, () -> mgr.lock(null, OWNER1, null));
         assertEquals(NULL_RESOURCE_ID, ex.getMessage());
 
-        ex = expectException(IllegalArgumentException.class, xxx -> mgr.lock(RESOURCE_A, null, null));
+        ex = expectException(IllegalArgumentException.class, () -> mgr.lock(RESOURCE_A, null, null));
         assertEquals(NULL_OWNER, ex.getMessage());
 
         // this should not throw an exception
@@ -118,10 +118,10 @@ public class SimpleLockManagerTest {
 
     @Test
     public void testUnlock_ArgEx() {
-        IllegalArgumentException ex = expectException(IllegalArgumentException.class, xxx -> mgr.unlock(null, OWNER1));
+        IllegalArgumentException ex = expectException(IllegalArgumentException.class, () -> mgr.unlock(null, OWNER1));
         assertEquals(NULL_RESOURCE_ID, ex.getMessage());
 
-        ex = expectException(IllegalArgumentException.class, xxx -> mgr.unlock(RESOURCE_A, null));
+        ex = expectException(IllegalArgumentException.class, () -> mgr.unlock(RESOURCE_A, null));
         assertEquals(NULL_OWNER, ex.getMessage());
     }
 
@@ -156,7 +156,7 @@ public class SimpleLockManagerTest {
 
     @Test
     public void testIsLocked_ArgEx() {
-        IllegalArgumentException ex = expectException(IllegalArgumentException.class, xxx -> mgr.isLocked(null));
+        IllegalArgumentException ex = expectException(IllegalArgumentException.class, () -> mgr.isLocked(null));
         assertEquals(NULL_RESOURCE_ID, ex.getMessage());
     }
 
@@ -181,10 +181,10 @@ public class SimpleLockManagerTest {
     @Test
     public void testIsLockedBy_ArgEx() {
         IllegalArgumentException ex =
-                        expectException(IllegalArgumentException.class, xxx -> mgr.isLockedBy(null, OWNER1));
+                        expectException(IllegalArgumentException.class, () -> mgr.isLockedBy(null, OWNER1));
         assertEquals(NULL_RESOURCE_ID, ex.getMessage());
 
-        ex = expectException(IllegalArgumentException.class, xxx -> mgr.isLockedBy(RESOURCE_A, null));
+        ex = expectException(IllegalArgumentException.class, () -> mgr.isLockedBy(RESOURCE_A, null));
         assertEquals(NULL_OWNER, ex.getMessage());
     }
 
@@ -221,34 +221,31 @@ public class SimpleLockManagerTest {
         for (int x = 0; x < nthreads; ++x) {
             String owner = "owner." + x;
 
-            Thread t = new Thread() {
-                @Override
-                public void run() {
+            Thread t = new Thread(() -> {
 
-                    for (int y = 0; y < nlocks; ++y) {
-                        String res = resources[y % resources.length];
+                for (int y = 0; y < nlocks; ++y) {
+                    String res = resources[y % resources.length];
 
-                        try {
-                            // some locks will be acquired, some denied
-                            mgr.lock(res, owner, null).get();
+                    try {
+                        // some locks will be acquired, some denied
+                        mgr.lock(res, owner, null).get();
 
-                            // do some "work"
-                            stopper.await(1L, TimeUnit.MILLISECONDS);
+                        // do some "work"
+                        stopper.await(1L, TimeUnit.MILLISECONDS);
 
-                            mgr.unlock(res, owner);
+                        mgr.unlock(res, owner);
 
-                        } catch (CancellationException | ExecutionException e) {
-                            nfail.incrementAndGet();
+                    } catch (CancellationException | ExecutionException e) {
+                        nfail.incrementAndGet();
 
-                        } catch (InterruptedException expected) {
-                            Thread.currentThread().interrupt();
-                            break;
-                        }
+                    } catch (InterruptedException expected) {
+                        Thread.currentThread().interrupt();
+                        break;
                     }
-
-                    completed.countDown();
                 }
-            };
+
+                completed.countDown();
+            });
 
             t.setDaemon(true);
             threads.add(t);
diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/TestUtils.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/TestUtils.java
index 2e353936..f843f6ab 100644
--- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/TestUtils.java
+++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/TestUtils.java
@@ -31,7 +31,7 @@ public class TestUtils {
      */
     public static <T> T expectException(Class<T> clazz, VoidFunction func) {
         try {
-            func.apply(null);
+            func.apply();
             throw new AssertionError("missing exception");
 
         } catch (Exception e) {
@@ -50,10 +50,6 @@ public class TestUtils {
     @FunctionalInterface
     public static interface VoidFunction {
 
-        /**
-         * 
-         * @param arg always {@code null}
-         */
-        public void apply(Void arg) throws Exception;
+        public void apply() throws Exception;
     }
 }