Address sonar issues in drools-pdp

Addressed the following sonar issues:
- add "final" to public static fields
- commented code; some were bogus - just updated the comments
  so sonar is happy
- use "{}" instead of string concatenation
- junit should assert something
- when using logger, invoke compute-intensive tasks conditionally
- use superclass name instead of subclass name to access static fields
- don't always return the same value
- remove "transient" from fields of classes that aren't Serializable
- don't nest try/catch blocks
- use appropriate class name in Logger.getLogger()
- use Predicate<T> instead of Function<T,Boolean>
- remove unused parameters from private methods
- either log or throw
- remove duplicate methods
- use remove() TLS instead of set(null)
- null check is implicit in instanceof check
- do something with return value
- don't use volatile
- don't return "null" list; used Optional instead
- add no-arg constructor to non-Serializable superclass
- add callSuper=true for EqualsAndHashCode
- don't declare "throws XXX" where XXX is a subclass of RuntimeException
- remove serialVersionUID field if the class isn't Serializable

Also addressed some eclipse warnings:
- unused fields
- suppress generic typic cast warnings

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

8 files changed, 77 insertions, 72 deletions

diff --git a/policy-core/src/main/java/org/onap/policy/drools/core/PolicyContainer.java b/policy-core/src/main/java/org/onap/policy/drools/core/PolicyContainer.java
index 0fe1f855..fee18a05 100644
--- a/policy-core/src/main/java/org/onap/policy/drools/core/PolicyContainer.java
+++ b/policy-core/src/main/java/org/onap/policy/drools/core/PolicyContainer.java
@@ -2,7 +2,7 @@
  * ============LICENSE_START=======================================================
  * policy-core
  * ================================================================================
- * Copyright (C) 2017-2019 AT&T Intellectual Property. All rights reserved.
+ * Copyright (C) 2017-2020 AT&T Intellectual Property. All rights reserved.
  * Modifications Copyright (C) 2018 Samsung Electronics Co., Ltd.
  * ================================================================================
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -74,7 +74,7 @@ public class PolicyContainer implements Startable {
     // (it can block for a long time)
     private boolean scannerStarted = false;
 
-    private static final String ERROR_STRING = "ERROR: Feature API: ";
+    private static final String ERROR_STRING = "ERROR: Feature API: {}";
 
     // packages that are included in all 'KieContainer' instances
     private static Collection<KiePackage> commonPackages = null;
@@ -260,50 +260,52 @@ public class PolicyContainer implements Startable {
     private PolicySession activatePolicySession(String name, String kieBaseName) {
         synchronized (sessions) {
             logger.info("activatePolicySession:name :{}", name);
-            PolicySession session = sessions.get(name);
-            if (session != null) {
-                logger.info("activatePolicySession:session - {} is returned.", session.getFullName());
-                return session;
-            }
-            KieSession kieSession = null;
+            PolicySession session = sessions.computeIfAbsent(name, key -> makeSession(name, kieBaseName));
 
-            // loop through all of the features, and give each one
-            // a chance to create the 'KieSession'
-            for (PolicySessionFeatureApi feature : PolicySessionFeatureApiConstants.getImpl().getList()) {
-                try {
-                    if ((kieSession = feature.activatePolicySession(this, name, kieBaseName)) != null) {
-                        break;
-                    }
-                } catch (Exception e) {
-                    logger.error(ERROR_STRING + feature.getClass().getName(), e);
+            logger.info("activatePolicySession:session - {} is returned.",
+                            session == null ? "null" : session.getFullName());
+            return session;
+        }
+    }
+
+    private PolicySession makeSession(String name, String kieBaseName) {
+        PolicySession session = null;
+        KieSession kieSession = null;
+
+        // loop through all of the features, and give each one
+        // a chance to create the 'KieSession'
+        for (PolicySessionFeatureApi feature : PolicySessionFeatureApiConstants.getImpl().getList()) {
+            try {
+                if ((kieSession = feature.activatePolicySession(this, name, kieBaseName)) != null) {
+                    break;
                 }
+            } catch (Exception e) {
+                logger.error(ERROR_STRING, feature.getClass().getName(), e);
             }
+        }
 
-            // if none of the features created the session, create one now
-            if (kieSession == null) {
-                kieSession = kieContainer.newKieSession(name);
-            }
+        // if none of the features created the session, create one now
+        if (kieSession == null) {
+            kieSession = kieContainer.newKieSession(name);
+        }
+
+        if (kieSession != null) {
+            // creation of 'KieSession' was successful - build
+            // a PolicySession
+            session = new PolicySession(name, this, kieSession);
 
-            if (kieSession != null) {
-                // creation of 'KieSession' was successful - build
-                // a PolicySession
-                session = new PolicySession(name, this, kieSession);
-                sessions.put(name, session);
-
-                // notify features
-                for (PolicySessionFeatureApi feature : PolicySessionFeatureApiConstants.getImpl().getList()) {
-                    try {
-                        feature.newPolicySession(session);
-                    } catch (Exception e) {
-                        logger.error(ERROR_STRING + feature.getClass().getName(), e);
-                    }
+            // notify features
+            for (PolicySessionFeatureApi feature : PolicySessionFeatureApiConstants.getImpl().getList()) {
+                try {
+                    feature.newPolicySession(session);
+                } catch (Exception e) {
+                    logger.error(ERROR_STRING, feature.getClass().getName(), e);
                 }
-                logger.info("activatePolicySession:new session was added in sessions with name {}", name);
             }
-            logger.info("activatePolicySession:session - {} is returned.",
-                    session == null ? "null" : session.getFullName());
-            return session;
+            logger.info("activatePolicySession:new session was added in sessions with name {}", name);
         }
+
+        return session;
     }
 
     /**
@@ -365,7 +367,7 @@ public class PolicyContainer implements Startable {
                 try {
                     feature.newPolicySession(policySession);
                 } catch (Exception e) {
-                    logger.error(ERROR_STRING + feature.getClass().getName(), e);
+                    logger.error(ERROR_STRING, feature.getClass().getName(), e);
                 }
             }
             return policySession;
@@ -562,10 +564,10 @@ public class PolicyContainer implements Startable {
      */
     @Override
     public synchronized boolean stop() {
-        if (!isStarted) {
-            return true;
-        }
+        return (!isStarted || doStop());
+    }
 
+    private boolean doStop() {
         Collection<PolicySession> localSessions;
 
         synchronized (sessions) {
@@ -587,7 +589,7 @@ public class PolicyContainer implements Startable {
                 try {
                     feature.disposeKieSession(session);
                 } catch (Exception e) {
-                    logger.error(ERROR_STRING + feature.getClass().getName(), e);
+                    logger.error(ERROR_STRING, feature.getClass().getName(), e);
                 }
             }
         }
@@ -650,7 +652,7 @@ public class PolicyContainer implements Startable {
                 try {
                     feature.destroyKieSession(session);
                 } catch (Exception e) {
-                    logger.error(ERROR_STRING + feature.getClass().getName(), e);
+                    logger.error(ERROR_STRING, feature.getClass().getName(), e);
                 }
             }
         }
@@ -710,7 +712,7 @@ public class PolicyContainer implements Startable {
             try {
                 feature.globalInit(args, configDir);
             } catch (Exception e) {
-                logger.error(ERROR_STRING + feature.getClass().getName(), e);
+                logger.error(ERROR_STRING, feature.getClass().getName(), e);
             }
         }
     }
@@ -751,7 +753,7 @@ public class PolicyContainer implements Startable {
         synchronized (PolicyContainer.class) {
             if (commonPackages == null) {
                 commonPackages = KieUtils.resourceToPackages(
-                    PolicyContainer.class.getClassLoader(), COMMON_PACKAGES_RESOURCE_NAME);
+                    PolicyContainer.class.getClassLoader(), COMMON_PACKAGES_RESOURCE_NAME).orElse(null);
                 if (commonPackages == null) {
                     // a problem occurred, which has already been logged --
                     // just store an empty collection, so we don't keep doing
diff --git a/policy-core/src/main/java/org/onap/policy/drools/core/PolicySession.java b/policy-core/src/main/java/org/onap/policy/drools/core/PolicySession.java
index 69ca87cb..cee7c368 100644
--- a/policy-core/src/main/java/org/onap/policy/drools/core/PolicySession.java
+++ b/policy-core/src/main/java/org/onap/policy/drools/core/PolicySession.java
@@ -2,7 +2,7 @@
  * ============LICENSE_START=======================================================
  * policy-core
  * ================================================================================
- * Copyright (C) 2017-2019 AT&T Intellectual Property. All rights reserved.
+ * Copyright (C) 2017-2020 AT&T Intellectual Property. All rights reserved.
  * Modifications Copyright (C) 2018 Samsung Electronics Co., Ltd.
  * ================================================================================
  * Licensed under the Apache License, Version 2.0 (the "License");
@@ -149,15 +149,14 @@ public class PolicySession
                     break;
                 }
             } catch (Exception e) {
-                logger.error("ERROR: Feature API: "
-                        + feature.getClass().getName(), e);
+                logger.error("ERROR: Feature API: {}", feature.getClass().getName(), e);
             }
         }
         if (threadModel == null) {
             // no feature created a ThreadModel -- select the default
             threadModel = new DefaultThreadModel(this);
         }
-        logger.info("starting ThreadModel for session " + getFullName());
+        logger.info("starting ThreadModel for session {}", getFullName());
         threadModel.start();
     }
 
diff --git a/policy-core/src/main/java/org/onap/policy/drools/core/lock/AlwaysFailLock.java b/policy-core/src/main/java/org/onap/policy/drools/core/lock/AlwaysFailLock.java
index 0a4d327b..03024f52 100644
--- a/policy-core/src/main/java/org/onap/policy/drools/core/lock/AlwaysFailLock.java
+++ b/policy-core/src/main/java/org/onap/policy/drools/core/lock/AlwaysFailLock.java
@@ -2,7 +2,7 @@
  * ============LICENSE_START=======================================================
  * ONAP
  * ================================================================================
- * Copyright (C) 2019 AT&T Intellectual Property. All rights reserved.
+ * Copyright (C) 2019-2020 AT&T Intellectual Property. All rights reserved.
  * ================================================================================
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -52,7 +52,7 @@ public class AlwaysFailLock extends LockImpl {
      * Always returns false.
      */
     @Override
-    public boolean free() {
+    public synchronized boolean free() {
         return false;
     }
 
diff --git a/policy-core/src/main/java/org/onap/policy/drools/util/KieUtils.java b/policy-core/src/main/java/org/onap/policy/drools/util/KieUtils.java
index c260924b..bcad792c 100644
--- a/policy-core/src/main/java/org/onap/policy/drools/util/KieUtils.java
+++ b/policy-core/src/main/java/org/onap/policy/drools/util/KieUtils.java
@@ -31,6 +31,7 @@ import java.util.Collections;
 import java.util.Enumeration;
 import java.util.HashSet;
 import java.util.List;
+import java.util.Optional;
 import java.util.stream.Collectors;
 
 import lombok.NonNull;
@@ -183,19 +184,19 @@ public class KieUtils {
      * @return a collection of 'KiePackage' instances, or 'null' in case of
      *     failure
      */
-    public static Collection<KiePackage> resourceToPackages(ClassLoader classLoader, String resourceName) {
+    public static Optional<Collection<KiePackage>> resourceToPackages(ClassLoader classLoader, String resourceName) {
 
         // find all resources matching 'resourceName'
         Enumeration<URL> resources;
         try {
             resources = classLoader.getResources(resourceName);
         } catch (IOException e) {
-            logger.error("Exception fetching resources: " + resourceName, e);
-            return null;
+            logger.error("Exception fetching resources: {}", resourceName, e);
+            return Optional.empty();
         }
         if (!resources.hasMoreElements()) {
             // no resources found
-            return null;
+            return Optional.empty();
         }
 
         // generate a 'KieFileSystem' from these resources
@@ -211,8 +212,8 @@ public class KieUtils {
                 // add a new '.drl' entry to the KieFileSystem
                 kfs.write(RESOURCE_PREFIX + index++ + RESOURCE_SUFFIX, drl);
             } catch (IOException e) {
-                logger.error("Couldn't read in " + url, e);
-                return null;
+                logger.error("Couldn't read in {}", url, e);
+                return Optional.empty();
             }
         }
 
@@ -221,13 +222,13 @@ public class KieUtils {
         builder.buildAll();
         List<Message> results = builder.getResults().getMessages();
         if (!results.isEmpty()) {
-            logger.error("Kie build failed:\n" + results);
-            return null;
+            logger.error("Kie build failed:\n{}", results);
+            return Optional.empty();
         }
 
         // generate a KieContainer, and extract the package list
-        return kieServices.newKieContainer(builder.getKieModule().getReleaseId(), classLoader)
-               .getKieBase().getKiePackages();
+        return Optional.of(kieServices.newKieContainer(builder.getKieModule().getReleaseId(), classLoader)
+               .getKieBase().getKiePackages());
     }
 
     /**
diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/PolicySessionTest.java b/policy-core/src/test/java/org/onap/policy/drools/core/PolicySessionTest.java
index 1ed27890..63c71608 100644
--- a/policy-core/src/test/java/org/onap/policy/drools/core/PolicySessionTest.java
+++ b/policy-core/src/test/java/org/onap/policy/drools/core/PolicySessionTest.java
@@ -20,6 +20,7 @@
 
 package org.onap.policy.drools.core;
 
+import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertNull;
 import static org.mockito.Mockito.doAnswer;
@@ -97,7 +98,7 @@ public class PolicySessionTest {
         // re-start
         session.startThread();
 
-        session.stopThread();
+        assertThatCode(() -> session.stopThread()).doesNotThrowAnyException();
     }
 
     @Test
@@ -140,7 +141,7 @@ public class PolicySessionTest {
             }
         };
 
-        model.updated();
+        assertThatCode(() -> model.updated()).doesNotThrowAnyException();
     }
 
     @Test
@@ -156,7 +157,7 @@ public class PolicySessionTest {
     /**
      * Starts a thread and then invokes a function to generate an exception within the
      * fireUntilHalt() method.
-     * 
+     *
      * @param genEx function to generate an exception
      * @throws Exception if an error occurs
      */
diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/AlwaysFailLockTest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/AlwaysFailLockTest.java
index ce4ca5fd..06489a5c 100644
--- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/AlwaysFailLockTest.java
+++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/AlwaysFailLockTest.java
@@ -2,7 +2,7 @@
  * ============LICENSE_START=======================================================
  * ONAP
  * ================================================================================
- * Copyright (C) 2019 AT&T Intellectual Property. All rights reserved.
+ * Copyright (C) 2019-2020 AT&T Intellectual Property. All rights reserved.
  * ================================================================================
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -20,6 +20,7 @@
 
 package org.onap.policy.drools.core.lock;
 
+import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.junit.Assert.assertEquals;
 import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertNull;
@@ -77,7 +78,7 @@ public class AlwaysFailLockTest {
     @Test
     public void testAlwaysFailLockNoArgs() {
         // verify that no-arg constructor doesn't throw an exception
-        new AlwaysFailLock();
+        assertThatCode(() -> new AlwaysFailLock()).doesNotThrowAnyException();
     }
 
     @Test
diff --git a/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockImplTest.java b/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockImplTest.java
index 902f3043..56cd5090 100644
--- a/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockImplTest.java
+++ b/policy-core/src/test/java/org/onap/policy/drools/core/lock/LockImplTest.java
@@ -21,6 +21,7 @@
 package org.onap.policy.drools.core.lock;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.assertj.core.api.Assertions.assertThatCode;
 import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException;
 import static org.assertj.core.api.Assertions.assertThatThrownBy;
 import static org.junit.Assert.assertEquals;
@@ -193,7 +194,7 @@ public class LockImplTest {
         doThrow(new IllegalArgumentException(EXPECTED_EXCEPTION)).when(callback).lockUnavailable(any());
 
         // should not throw an exception
-        lock.notifyAvailable();
+        assertThatCode(() -> lock.notifyAvailable()).doesNotThrowAnyException();
     }
 
     @Test
@@ -210,7 +211,7 @@ public class LockImplTest {
         doThrow(new IllegalArgumentException(EXPECTED_EXCEPTION)).when(callback).lockUnavailable(any());
 
         // should not throw an exception
-        lock.notifyUnavailable();
+        assertThatCode(() -> lock.notifyUnavailable()).doesNotThrowAnyException();
     }
 
     @Test
diff --git a/policy-core/src/test/java/org/onap/policy/drools/util/KieUtilsTest.java b/policy-core/src/test/java/org/onap/policy/drools/util/KieUtilsTest.java
index 97110744..f230d4f9 100644
--- a/policy-core/src/test/java/org/onap/policy/drools/util/KieUtilsTest.java
+++ b/policy-core/src/test/java/org/onap/policy/drools/util/KieUtilsTest.java
@@ -130,15 +130,15 @@ public class KieUtilsTest {
 
         // test IOException from ClassLoader
         log = new StringBuffer();
-        assertNull(KieUtils.resourceToPackages(new BogusClassLoader(log), "BogusClassLoader"));
+        assertNull(KieUtils.resourceToPackages(new BogusClassLoader(log), "BogusClassLoader").orElse(null));
         assertEquals("IOException(BogusClassLoader)", log.toString());
 
         // test 'null' return when no resources are found
-        assertNull(KieUtils.resourceToPackages(ClassLoader.getSystemClassLoader(), "no/such/url"));
+        assertNull(KieUtils.resourceToPackages(ClassLoader.getSystemClassLoader(), "no/such/url").orElse(null));
 
         // test IOException in 'IOUtils.toByteArray()' -> 'InputStream.read()'
         log = new StringBuffer();
-        assertNull(KieUtils.resourceToPackages(new BogusClassLoader(log), "BogusUrl"));
+        assertNull(KieUtils.resourceToPackages(new BogusClassLoader(log), "BogusUrl").orElse(null));
         assertEquals("", log.toString());
 
         // don't know how to test 'KieBuilder' errors at this point