Merge pull request #501 from DependencyTrack/jdbi-rowmapper-threadsafety

Address thread safety issues with `CelPolicy*RowMapper`s

src/main/java/org/dependencytrack/policy/cel/persistence/CelPolicyComponentRowMapper.java

@@ -12,19 +12,9 @@
 
 public class CelPolicyComponentRowMapper implements RowMapper<Component> {
 
-    private final Component.Builder builder;
-
-    @SuppressWarnings("unused") // Used by JDBI
-    public CelPolicyComponentRowMapper() {
-        this(Component.newBuilder());
-    }
-
-    CelPolicyComponentRowMapper(final Component.Builder builder) {
-        this.builder = builder;
-    }
-
     @Override
     public Component map(final ResultSet rs, final StatementContext ctx) throws SQLException {
+        final Component.Builder builder = Component.newBuilder();
         maybeSet(rs, "uuid", ResultSet::getString, builder::setUuid);
         maybeSet(rs, "group", ResultSet::getString, builder::setGroup);
         maybeSet(rs, "name", ResultSet::getString, builder::setName);

src/main/java/org/dependencytrack/policy/cel/persistence/CelPolicyProjectRowMapper.java

@@ -22,19 +22,9 @@
 
 public class CelPolicyProjectRowMapper implements RowMapper<Project> {
 
-    private final Project.Builder builder;
-
-    @SuppressWarnings("unused") // Used by JDBI
-    public CelPolicyProjectRowMapper() {
-        this(Project.newBuilder());
-    }
-
-    public CelPolicyProjectRowMapper(final Project.Builder builder) {
-        this.builder = builder;
-    }
-
     @Override
     public Project map(final ResultSet rs, final StatementContext ctx) throws SQLException {
+        final Project.Builder builder = Project.newBuilder();
         maybeSet(rs, "uuid", ResultSet::getString, builder::setUuid);
         maybeSet(rs, "group", ResultSet::getString, builder::setGroup);
         maybeSet(rs, "name", ResultSet::getString, builder::setName);

src/main/java/org/dependencytrack/policy/cel/persistence/CelPolicyVulnerabilityRowMapper.java

@@ -24,19 +24,9 @@ public class CelPolicyVulnerabilityRowMapper implements RowMapper<Vulnerability>
     private static final TypeReference<List<VulnerabilityAlias>> VULNERABILITY_ALIASES_TYPE_REF = new TypeReference<>() {
     };
 
-    private final Vulnerability.Builder builder;
-
-    @SuppressWarnings("unused") // Used by JDBI
-    public CelPolicyVulnerabilityRowMapper() {
-        this(Vulnerability.newBuilder());
-    }
-
-    public CelPolicyVulnerabilityRowMapper(final Vulnerability.Builder builder) {
-        this.builder = builder;
-    }
-
     @Override
     public Vulnerability map(final ResultSet rs, final StatementContext ctx) throws SQLException {
+        final Vulnerability.Builder builder = Vulnerability.newBuilder();
         maybeSet(rs, "uuid", ResultSet::getString, builder::setUuid);
         maybeSet(rs, "id", ResultSet::getString, builder::setId);
         maybeSet(rs, "source", ResultSet::getString, builder::setSource);

src/test/java/org/dependencytrack/policy/cel/CelVulnerabilityPolicyEvaluatorTest.java

@@ -16,6 +16,10 @@
 import java.util.List;
 import java.util.Map;
 import java.util.UUID;
+import java.util.concurrent.ConcurrentLinkedQueue;
+import java.util.concurrent.CountDownLatch;
+import java.util.concurrent.ExecutorService;
+import java.util.concurrent.Executors;
 import java.util.concurrent.TimeUnit;
 
 import static org.assertj.core.api.Assertions.assertThat;
@@ -130,7 +134,7 @@ public void testEvaluateWithMultipleMatchingPolicies() {
     }
 
     @Test
-    public void testEvaluateWithAdditionalRequiredFields() {
+    public void testEvaluateWithAdditionalRequiredFields() throws Exception {
         final var persistentProject = new org.dependencytrack.model.Project();
         persistentProject.setName("acme-app");
         persistentProject.setVersion("1.0.0");
@@ -183,8 +187,38 @@ public void testEvaluateWithAdditionalRequiredFields() {
         doReturn(List.of(policy))
                 .when(policyProviderMock).getApplicablePolicies(any(Project.class));
 
-        assertThat(policyEvaluator.evaluate(List.of(vuln), component, project))
-                .containsOnly(Map.entry(persistentVuln.getUuid(), policy));
+        // We want evaluation as a whole to be thread safe!
+        final var startLatch = new CountDownLatch(1);
+        final ExecutorService executor = Executors.newFixedThreadPool(10);
+        final var exceptionsThrown = new ConcurrentLinkedQueue<Exception>();
+        try {
+            for (int i = 0; i < 1000; i++) {
+                executor.submit(() -> {
+                    try {
+                        startLatch.await();
+                    } catch (InterruptedException e) {
+                        throw new RuntimeException(e);
+                    }
+
+                    try {
+                        assertThat(policyEvaluator.evaluate(List.of(vuln), component, project))
+                                .containsOnly(Map.entry(persistentVuln.getUuid(), policy));
+                    } catch (Exception e) {
+                        // If we throw here, the exception won't bubble up to the test.
+                        // Collect all encountered exceptions here and assert on all of them later.
+                        exceptionsThrown.add(e);
+                    }
+                });
+            }
+
+            // Open the flood gates :)
+            startLatch.countDown();
+        } finally {
+            executor.shutdown();
+            assertThat(executor.awaitTermination(90, TimeUnit.SECONDS)).isTrue();
+        }
+
+        assertThat(exceptionsThrown).isEmpty();
     }
 
     private static final class TestCacheManager extends AbstractCacheManager {