Cleanup

- shutdown() ExecutorService after task execution - javadoc - improve unit test coverage
2026-01-14 07:43:40 +01:00 · 2016-10-09 13:42:10 +02:00
parent b2149ff4b9
commit 9b43bf004a
8 changed files with 252 additions and 33 deletions
--- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/AnalysisTask.java
+++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/AnalysisTask.java
@@ -11,6 +11,9 @@ import org.slf4j.LoggerFactory;
 import java.util.List;
 import java.util.concurrent.Callable;

+/**
+ * The task of analyzing a single {@link Dependency} by a specific {@link Analyzer}.
+ */
 class AnalysisTask implements Callable<Void> {

    private static final Logger LOGGER = LoggerFactory.getLogger(AnalysisTask.class);
@@ -28,7 +31,7 @@ class AnalysisTask implements Callable<Void> {
    }

    @Override
-    public Void call() throws Exception {
+    public Void call() {
        Settings.initialize();

        if (shouldAnalyze()) {
@@ -50,10 +53,10 @@ class AnalysisTask implements Callable<Void> {
        return null;
    }

-    private boolean shouldAnalyze() {
+    boolean shouldAnalyze() {
        if (analyzer instanceof FileTypeAnalyzer) {
-            final FileTypeAnalyzer fAnalyzer = (FileTypeAnalyzer) analyzer;
-            return fAnalyzer.accept(dependency.getActualFile());
+            final FileTypeAnalyzer fileTypeAnalyzer = (FileTypeAnalyzer) analyzer;
+            return fileTypeAnalyzer.accept(dependency.getActualFile());
        }

        return true;
--- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/Engine.java
+++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/Engine.java
@@ -47,6 +47,7 @@ import java.util.Iterator;
 import java.util.List;
 import java.util.Map;
 import java.util.Set;
+import java.util.concurrent.CancellationException;
 import java.util.concurrent.ExecutionException;
 import java.util.concurrent.ExecutorService;
 import java.util.concurrent.Executors;
@@ -396,20 +397,20 @@ public class Engine implements FileFilter {
        for (AnalysisPhase phase : AnalysisPhase.values()) {
            final List<Analyzer> analyzerList = analyzers.get(phase);

-            for (final Analyzer a : analyzerList) {
+            for (final Analyzer analyzer : analyzerList) {
                final long analyzerStart = System.currentTimeMillis();
                try {
-                    initializeAnalyzer(a);
+                    initializeAnalyzer(analyzer);
                } catch (InitializationException ex) {
                    exceptions.add(ex);
                    continue;
                }

-                executeAnalysisTasks(exceptions, a);
+                executeAnalysisTasks(analyzer, exceptions);

                final long analyzerDurationMillis = System.currentTimeMillis() - analyzerStart;
                final long analyzerDurationSeconds = TimeUnit.MILLISECONDS.toSeconds(analyzerDurationMillis);
-                LOGGER.info("Finished {}. Took {} secs.", a.getName(), analyzerDurationSeconds);
+                LOGGER.info("Finished {}. Took {} secs.", analyzer.getName(), analyzerDurationSeconds);
            }
        }
        for (AnalysisPhase phase : AnalysisPhase.values()) {
@@ -427,7 +428,7 @@ public class Engine implements FileFilter {
        }
    }

-    private void executeAnalysisTasks(List<Throwable> exceptions, Analyzer analyzer) throws ExceptionCollection {
+    void executeAnalysisTasks(Analyzer analyzer, List<Throwable> exceptions) throws ExceptionCollection {
        LOGGER.debug("Starting {}", analyzer.getName());
        final List<AnalysisTask> analysisTasks = getAnalysisTasks(analyzer, exceptions);
        final ExecutorService executorService = getExecutorService(analyzer);
@@ -441,14 +442,18 @@ public class Engine implements FileFilter {
                    result.get();
                } catch (ExecutionException e) {
                    throwFatalExceptionCollection("Analysis task failed with a fatal exception.", e, exceptions);
+                } catch (CancellationException e) {
+                    throwFatalExceptionCollection("Analysis task timed out.", e, exceptions);
                }
            }
        } catch (InterruptedException e) {
            throwFatalExceptionCollection("Analysis has been interrupted.", e, exceptions);
+        } finally {
+            executorService.shutdown();
        }
    }

-    private List<AnalysisTask> getAnalysisTasks(Analyzer analyzer, List<Throwable> exceptions) {
+    List<AnalysisTask> getAnalysisTasks(Analyzer analyzer, List<Throwable> exceptions) {
        final List<AnalysisTask> result = new ArrayList<AnalysisTask>();
        synchronized (dependencies) {
            for (final Dependency dependency : dependencies) {
@@ -459,7 +464,7 @@ public class Engine implements FileFilter {
        return result;
    }

-    private ExecutorService getExecutorService(Analyzer analyzer) {
+    ExecutorService getExecutorService(Analyzer analyzer) {
        if (analyzer.supportsParallelProcessing()) {
            // just a fair trade-off that should be reasonable for all analyzer types
            int maximumNumberOfThreads = 4 * Runtime.getRuntime().availableProcessors();
--- a/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/DependencyBundlingAnalyzer.java
+++ b/dependency-check-core/src/main/java/org/owasp/dependencycheck/analyzer/DependencyBundlingAnalyzer.java
@@ -58,10 +58,11 @@ public class DependencyBundlingAnalyzer extends AbstractAnalyzer {
     * A pattern for obtaining the first part of a filename.
     */
    private static final Pattern STARTING_TEXT_PATTERN = Pattern.compile("^[a-zA-Z0-9]*");
+
    /**
     * a flag indicating if this analyzer has run. This analyzer only runs once.
     */
-    private boolean analyzed = false;
+    boolean analyzed = false;
    //</editor-fold>
    //<editor-fold defaultstate="collapsed" desc="All standard implementation details of Analyzer">
    /**
@@ -95,7 +96,7 @@ public class DependencyBundlingAnalyzer extends AbstractAnalyzer {
    //</editor-fold>

    /**
-     * Does not support parallel processing as it both modifies and iterates over the engine's list of dependencies.
+     * Does not support parallel processing as it only runs once and then operates on <em>all</em> dependencies.
     *
     * @see #analyze(Dependency, Engine)
     */
--- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/AnalysisTaskTest.java
+++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/AnalysisTaskTest.java
@@ -0,0 +1,100 @@
+package org.owasp.dependencycheck;
+
+import mockit.Expectations;
+import mockit.Mocked;
+import mockit.Verifications;
+import org.junit.Test;
+import org.owasp.dependencycheck.analyzer.FileTypeAnalyzer;
+import org.owasp.dependencycheck.analyzer.HintAnalyzer;
+import org.owasp.dependencycheck.dependency.Dependency;
+
+import java.io.File;
+
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
+public class AnalysisTaskTest {
+
+    @Mocked
+    FileTypeAnalyzer fileTypeAnalyzer;
+
+    @Mocked
+    Dependency dependency;
+
+    @Mocked
+    Engine engine;
+
+
+    @Test
+    public void shouldAnalyzeReturnsTrueForNonFileTypeAnalyzers() {
+        AnalysisTask instance = new AnalysisTask(new HintAnalyzer(), null, null, null);
+        boolean shouldAnalyze = instance.shouldAnalyze();
+        assertTrue(shouldAnalyze);
+    }
+
+    @Test
+    public void shouldAnalyzeReturnsTrueIfTheFileTypeAnalyzersAcceptsTheDependency() {
+        final File dependencyFile = new File("");
+        new Expectations() {{
+            dependency.getActualFile();
+            result = dependencyFile;
+
+            fileTypeAnalyzer.accept(dependencyFile);
+            result = true;
+        }};
+
+        AnalysisTask analysisTask = new AnalysisTask(fileTypeAnalyzer, dependency, null, null);
+
+        boolean shouldAnalyze = analysisTask.shouldAnalyze();
+        assertTrue(shouldAnalyze);
+    }
+
+    @Test
+    public void shouldAnalyzeReturnsFalseIfTheFileTypeAnalyzerDoesNotAcceptTheDependency() {
+        final File dependencyFile = new File("");
+        new Expectations() {{
+            dependency.getActualFile();
+            result = dependencyFile;
+
+            fileTypeAnalyzer.accept(dependencyFile);
+            result = false;
+        }};
+
+        AnalysisTask analysisTask = new AnalysisTask(fileTypeAnalyzer, dependency, null, null);
+
+        boolean shouldAnalyze = analysisTask.shouldAnalyze();
+        assertFalse(shouldAnalyze);
+    }
+
+    @Test
+    public void taskAnalyzes() throws Exception {
+        final AnalysisTask analysisTask = new AnalysisTask(fileTypeAnalyzer, dependency, engine, null);
+        new Expectations(analysisTask) {{
+            analysisTask.shouldAnalyze();
+            result = true;
+        }};
+
+        analysisTask.call();
+
+        new Verifications() {{
+            fileTypeAnalyzer.analyze(dependency, engine);
+            times = 1;
+        }};
+    }
+
+    @Test
+    public void taskDoesNothingIfItShouldNotAnalyze() throws Exception {
+        final AnalysisTask analysisTask = new AnalysisTask(fileTypeAnalyzer, dependency, engine, null);
+        new Expectations(analysisTask) {{
+            analysisTask.shouldAnalyze();
+            result = false;
+        }};
+
+        analysisTask.call();
+
+        new Verifications() {{
+            fileTypeAnalyzer.analyze(dependency, engine);
+            times = 0;
+        }};
+    }
+}
--- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/EngineTest.java
+++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/EngineTest.java
@@ -17,19 +17,36 @@
 */
 package org.owasp.dependencycheck;

-import java.io.File;
+import mockit.Expectations;
+import mockit.Mocked;
 import org.junit.Test;
-import static org.junit.Assert.*;
+import org.owasp.dependencycheck.analyzer.Analyzer;
 import org.owasp.dependencycheck.analyzer.JarAnalyzer;
 import org.owasp.dependencycheck.data.nvdcve.DatabaseException;
 import org.owasp.dependencycheck.dependency.Dependency;
+import org.owasp.dependencycheck.exception.ExceptionCollection;
+
+import java.io.File;
+import java.util.ArrayList;
+import java.util.List;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
+
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertTrue;

 /**
- *
 * @author Jeremy Long
 */
 public class EngineTest extends BaseDBTestCase {

+    @Mocked
+    Analyzer analyzer;
+
+    @Mocked
+    AnalysisTask analysisTask;
+
+
    /**
     * Test of scanFile method, of class Engine.
     */
@@ -40,14 +57,40 @@ public class EngineTest extends BaseDBTestCase {
        File file = BaseTest.getResourceAsFile(this, "dwr.jar");
        Dependency dwr = instance.scanFile(file);
        file = BaseTest.getResourceAsFile(this, "org.mortbay.jmx.jar");
-        Dependency jmx = instance.scanFile(file);
+        instance.scanFile(file);
        assertEquals(2, instance.getDependencies().size());
-        
+
        file = BaseTest.getResourceAsFile(this, "dwr.jar");
        Dependency secondDwr = instance.scanFile(file);
-        
+
        assertEquals(2, instance.getDependencies().size());
        assertTrue(dwr == secondDwr);
-        
+    }
+
+    @Test(expected = ExceptionCollection.class)
+    public void exceptionDuringAnalysisTaskExecutionIsFatal() throws DatabaseException, ExceptionCollection {
+        final ExecutorService executorService = Executors.newFixedThreadPool(3);
+        final Engine instance = new Engine();
+        final List<Throwable> exceptions = new ArrayList<Throwable>();
+
+        new Expectations() {{
+            analysisTask.call();
+            result = new IllegalStateException("Analysis task execution threw an exception");
+        }};
+
+        final List<AnalysisTask> failingAnalysisTask = new ArrayList<AnalysisTask>();
+        failingAnalysisTask.add(analysisTask);
+
+        new Expectations(instance) {{
+            instance.getExecutorService(analyzer);
+            result = executorService;
+
+            instance.getAnalysisTasks(analyzer, exceptions);
+            result = failingAnalysisTask;
+        }};
+
+        instance.executeAnalysisTasks(analyzer, exceptions);
+
+        assertTrue(executorService.isShutdown());
    }
 }
--- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CMakeAnalyzerTest.java
+++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/CMakeAnalyzerTest.java
@@ -17,23 +17,31 @@
 */
 package org.owasp.dependencycheck.analyzer;

+import mockit.Mock;
+import mockit.MockUp;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.owasp.dependencycheck.BaseDBTestCase;
 import org.owasp.dependencycheck.BaseTest;
 import org.owasp.dependencycheck.Engine;
 import org.owasp.dependencycheck.analyzer.exception.AnalysisException;
 import org.owasp.dependencycheck.data.nvdcve.DatabaseException;
 import org.owasp.dependencycheck.dependency.Dependency;
+import org.owasp.dependencycheck.exception.InitializationException;

 import java.io.File;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;
 import java.util.List;
 import java.util.regex.Pattern;

 import static org.hamcrest.CoreMatchers.equalTo;
 import static org.hamcrest.CoreMatchers.is;
-import static org.junit.Assert.*;
-import org.owasp.dependencycheck.BaseDBTestCase;
+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertThat;
+import static org.junit.Assert.assertTrue;

 /**
 * Unit tests for CmakeAnalyzer.
@@ -150,4 +158,21 @@ public class CMakeAnalyzerTest extends BaseDBTestCase {
        assertTrue("Expected version evidence to contain \"" + version + "\".",
                result.getVersionEvidence().toString().contains(version));
    }
+
+    @Test(expected = InitializationException.class)
+    public void analyzerIsDisabledInCaseOfMissingMessageDigest() throws InitializationException {
+        new MockUp<MessageDigest>() {
+            @Mock
+            MessageDigest getInstance(String ignore) throws NoSuchAlgorithmException {
+                throw new NoSuchAlgorithmException();
+            }
+        };
+
+        analyzer = new CMakeAnalyzer();
+        analyzer.setFilesMatched(true);
+        assertTrue(analyzer.isEnabled());
+        analyzer.initialize();
+
+        assertFalse(analyzer.isEnabled());
+    }
 }
--- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/ComposerLockAnalyzerTest.java
+++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/ComposerLockAnalyzerTest.java
@@ -17,19 +17,25 @@
 */
 package org.owasp.dependencycheck.analyzer;

+import mockit.Mock;
+import mockit.MockUp;
 import org.junit.After;
 import org.junit.Before;
 import org.junit.Test;
+import org.owasp.dependencycheck.BaseDBTestCase;
 import org.owasp.dependencycheck.BaseTest;
 import org.owasp.dependencycheck.Engine;
 import org.owasp.dependencycheck.analyzer.exception.AnalysisException;
 import org.owasp.dependencycheck.dependency.Dependency;
+import org.owasp.dependencycheck.exception.InitializationException;

 import java.io.File;
+import java.security.MessageDigest;
+import java.security.NoSuchAlgorithmException;

 import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
 import static org.junit.Assert.assertTrue;
-import org.owasp.dependencycheck.BaseDBTestCase;

 /**
 * Unit tests for NodePackageAnalyzer.
@@ -96,4 +102,22 @@ public class ComposerLockAnalyzerTest extends BaseDBTestCase {
                "composer.lock"));
        analyzer.analyze(result, engine);
    }
+
+
+    @Test(expected = InitializationException.class)
+    public void analyzerIsDisabledInCaseOfMissingMessageDigest() throws InitializationException {
+        new MockUp<MessageDigest>() {
+            @Mock
+            MessageDigest getInstance(String ignore) throws NoSuchAlgorithmException {
+                throw new NoSuchAlgorithmException();
+            }
+        };
+
+        analyzer = new ComposerLockAnalyzer();
+        analyzer.setFilesMatched(true);
+        assertTrue(analyzer.isEnabled());
+        analyzer.initialize();
+
+        assertFalse(analyzer.isEnabled());
+    }
 }
--- a/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/DependencyBundlingAnalyzerTest.java
+++ b/dependency-check-core/src/test/java/org/owasp/dependencycheck/analyzer/DependencyBundlingAnalyzerTest.java
@@ -17,17 +17,25 @@
 */
 package org.owasp.dependencycheck.analyzer;

-import static org.junit.Assert.assertEquals;
+import mockit.Mocked;
+import mockit.Verifications;
 import org.junit.Test;
 import org.owasp.dependencycheck.BaseTest;
+import org.owasp.dependencycheck.Engine;
 import org.owasp.dependencycheck.dependency.Dependency;

+import static org.junit.Assert.assertEquals;
+import static org.junit.Assert.assertFalse;
+import static org.junit.Assert.assertTrue;
+
 /**
- *
 * @author Jeremy Long
 */
 public class DependencyBundlingAnalyzerTest extends BaseTest {

+    @Mocked
+    Engine engineMock;
+
    /**
     * Test of getName method, of class DependencyBundlingAnalyzer.
     */
@@ -52,15 +60,27 @@ public class DependencyBundlingAnalyzerTest extends BaseTest {

    /**
     * Test of analyze method, of class DependencyBundlingAnalyzer.
+     * The actually passed dependency does not matter. The analyzer only runs once.
     */
    @Test
    public void testAnalyze() throws Exception {
-//        Dependency ignore = null;
-//        Engine engine = null;
-//        DependencyBundlingAnalyzer instance = new DependencyBundlingAnalyzer();
-//        instance.analyze(ignore, engine);
-//        // TODO review the generated test code and remove the default call to fail.
-//        fail("The test case is a prototype.");
+        DependencyBundlingAnalyzer instance = new DependencyBundlingAnalyzer();
+
+        // the actual dependency does not matter
+        assertFalse(instance.analyzed);
+        instance.analyze(null, engineMock);
+
+        // the second runs basically does nothing
+        assertTrue(instance.analyzed);
+        instance.analyze(null, engineMock);
+        instance.analyze(null, engineMock);
+        instance.analyze(null, engineMock);
+        assertTrue(instance.analyzed);
+
+        new Verifications() {{
+            engineMock.getDependencies();
+            times = 2;
+        }};
    }

    /**
@@ -119,7 +139,5 @@ public class DependencyBundlingAnalyzerTest extends BaseTest {
        expResult = true;
        result = instance.firstPathIsShortest(left, right);
        assertEquals(expResult, result);
-
    }
-
 }