From 72217d61b8c3d010670e619fb9181b27841a6725 Mon Sep 17 00:00:00 2001 From: Niklas Date: Wed, 31 Jul 2024 15:45:00 +0200 Subject: [PATCH] Replace manual transaction commits with `callInTransaction` (#815) --- .../persistence/MetricsQueryManager.java | 15 ++-- .../persistence/NotificationQueryManager.java | 22 +++--- .../persistence/ProjectQueryManager.java | 31 ++++---- .../persistence/QueryManager.java | 72 +++++-------------- .../VulnerabilityQueryManager.java | 21 +++--- .../resources/v1/ProjectResource.java | 2 +- .../resources/v1/TeamResource.java | 2 +- .../v1/VulnerabilityPolicyResource.java | 2 +- .../tasks/BomUploadProcessingTask.java | 4 +- .../tasks/WorkflowStateCleanupTask.java | 4 +- src/main/resources/logback.xml | 2 + 11 files changed, 66 insertions(+), 111 deletions(-) diff --git a/src/main/java/org/dependencytrack/persistence/MetricsQueryManager.java b/src/main/java/org/dependencytrack/persistence/MetricsQueryManager.java index bc496ce75..ea96a9618 100644 --- a/src/main/java/org/dependencytrack/persistence/MetricsQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/MetricsQueryManager.java @@ -192,17 +192,10 @@ public List getDependencyMetricsSince(Component component, Da * Synchronizes VulnerabilityMetrics. */ public void synchronizeVulnerabilityMetrics(List metrics) { - pm.currentTransaction().begin(); - // No need for complex updating, just replace the existing ~400 rows with new ones - // Unless we have a contract with clients that the ID of metric records cannot change? - - final Query delete = pm.newQuery("DELETE FROM org.dependencytrack.model.VulnerabilityMetrics"); - delete.execute(); - - // This still does ~400 queries, probably because not all databases can do bulk insert with autogenerated PKs - // Or because Datanucleus is trying to be smart as it wants to cache all these instances - pm.makePersistentAll(metrics); - pm.currentTransaction().commit(); + runInTransaction(() -> { + pm.newQuery("DELETE FROM org.dependencytrack.model.VulnerabilityMetrics").execute(); + pm.makePersistentAll(metrics); + }); } /** diff --git a/src/main/java/org/dependencytrack/persistence/NotificationQueryManager.java b/src/main/java/org/dependencytrack/persistence/NotificationQueryManager.java index 78a6c0f22..ee53a2ee0 100644 --- a/src/main/java/org/dependencytrack/persistence/NotificationQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/NotificationQueryManager.java @@ -171,18 +171,16 @@ public NotificationPublisher getDefaultNotificationPublisherByName(final String public NotificationPublisher createNotificationPublisher(final String name, final String description, final String publisherClass, final String templateContent, final String templateMimeType, final boolean defaultPublisher) { - pm.currentTransaction().begin(); - final NotificationPublisher publisher = new NotificationPublisher(); - publisher.setName(name); - publisher.setDescription(description); - publisher.setPublisherClass(publisherClass); - publisher.setTemplate(templateContent); - publisher.setTemplateMimeType(templateMimeType); - publisher.setDefaultPublisher(defaultPublisher); - pm.makePersistent(publisher); - pm.currentTransaction().commit(); - pm.getFetchPlan().addGroup(NotificationPublisher.FetchGroup.ALL.name()); - return getObjectById(NotificationPublisher.class, publisher.getId()); + return callInTransaction(() -> { + final NotificationPublisher publisher = new NotificationPublisher(); + publisher.setName(name); + publisher.setDescription(description); + publisher.setPublisherClass(publisherClass); + publisher.setTemplate(templateContent); + publisher.setTemplateMimeType(templateMimeType); + publisher.setDefaultPublisher(defaultPublisher); + return pm.makePersistent(publisher); + }); } /** diff --git a/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java b/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java index cc9a1e4e0..b631195cc 100644 --- a/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java @@ -775,25 +775,26 @@ public List getProjectProperties(final Project project) { * @param project a Project object * @param tags a List of Tag objects */ - @SuppressWarnings("unchecked") @Override public void bind(Project project, List tags) { - final Query query = pm.newQuery(Tag.class, "projects.contains(:project)"); - final List currentProjectTags = (List) query.execute(project); - pm.currentTransaction().begin(); - for (final Tag tag : currentProjectTags) { - if (!tags.contains(tag)) { - tag.getProjects().remove(project); + runInTransaction(() -> { + final Query query = pm.newQuery(Tag.class, "projects.contains(:project)"); + query.setParameters(project); + final List currentProjectTags = executeAndCloseList(query); + + for (final Tag tag : currentProjectTags) { + if (!tags.contains(tag)) { + tag.getProjects().remove(project); + } } - } - project.setTags(tags); - for (final Tag tag : tags) { - final List projects = tag.getProjects(); - if (!projects.contains(project)) { - projects.add(project); + project.setTags(tags); + for (final Tag tag : tags) { + final List projects = tag.getProjects(); + if (!projects.contains(project)) { + projects.add(project); + } } - } - pm.currentTransaction().commit(); + }); } /** diff --git a/src/main/java/org/dependencytrack/persistence/QueryManager.java b/src/main/java/org/dependencytrack/persistence/QueryManager.java index ca0de2d2b..314ee5dc0 100644 --- a/src/main/java/org/dependencytrack/persistence/QueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/QueryManager.java @@ -30,6 +30,7 @@ import alpine.persistence.AlpineQueryManager; import alpine.persistence.OrderDirection; import alpine.persistence.PaginatedResult; +import alpine.persistence.ScopedCustomization; import alpine.resources.AlpineRequest; import alpine.server.util.DbUtil; import com.github.packageurl.PackageURL; @@ -117,9 +118,10 @@ import java.util.Map; import java.util.Set; import java.util.UUID; +import java.util.concurrent.Callable; import java.util.function.Predicate; -import java.util.function.Supplier; +import static org.datanucleus.PropertyNames.PROPERTY_QUERY_SQL_ALLOWALL; import static org.dependencytrack.proto.vulnanalysis.v1.ScanStatus.SCAN_STATUS_FAILED; /** @@ -1483,70 +1485,28 @@ public T getObjectByUuid(final Class clazz, final UUID uuid, final List - * Eventually, this may be moved to {@link alpine.persistence.AbstractAlpineQueryManager}. - * - * @param runnable The {@link Runnable} to execute - * @since 4.6.0 - */ - public void runInTransaction(final Runnable runnable) { - final Transaction trx = pm.currentTransaction(); - try { - trx.begin(); - runnable.run(); - trx.commit(); - } finally { - if (trx.isActive()) { - trx.rollback(); - } - } - } - - /** - * Convenience method to execute a given {@link Supplier} within the context of a {@link Transaction}. - * - * @param supplier The {@link Supplier} to execute - * @param Type of the result of {@code supplier} - * @return The result of the execution of {@code supplier} - */ - public T runInTransaction(final Supplier supplier) { - final Transaction trx = pm.currentTransaction(); - try { - trx.begin(); - final T result = supplier.get(); - trx.commit(); - return result; - } finally { - if (trx.isActive()) { - trx.rollback(); - } - } - } - - public T runInRetryableTransaction(final Supplier supplier, final Predicate retryOn) { + public T runInRetryableTransaction(final Callable supplier, final Predicate retryOn) { final var retryConfig = RetryConfig.custom() .retryOnException(retryOn) .maxAttempts(3) .build(); return Retry.of("runInRetryableTransaction", retryConfig) - .executeSupplier(() -> runInTransaction(supplier)); + .executeSupplier(() -> callInTransaction(supplier)); } public void recursivelyDeleteTeam(Team team) { - pm.setProperty("datanucleus.query.sql.allowAll", true); - final Transaction trx = pm.currentTransaction(); - pm.currentTransaction().begin(); - pm.deletePersistentAll(team.getApiKeys()); - String aclDeleteQuery = """ - DELETE FROM "PROJECT_ACCESS_TEAMS" WHERE "TEAM_ID" = ? - """; - final Query query = pm.newQuery(JDOQuery.SQL_QUERY_LANGUAGE, aclDeleteQuery); - query.executeWithArray(team.getId()); - pm.deletePersistent(team); - pm.currentTransaction().commit(); + runInTransaction(() -> { + pm.deletePersistentAll(team.getApiKeys()); + + try (var ignored = new ScopedCustomization(pm).withProperty(PROPERTY_QUERY_SQL_ALLOWALL, "true")) { + final Query aclDeleteQuery = pm.newQuery(JDOQuery.SQL_QUERY_LANGUAGE, """ + DELETE FROM "PROJECT_ACCESS_TEAMS" WHERE "PROJECT_ACCESS_TEAMS"."TEAM_ID" = ?"""); + executeAndCloseWithArray(aclDeleteQuery, team.getId()); + } + + pm.deletePersistent(team); + }); } /** diff --git a/src/main/java/org/dependencytrack/persistence/VulnerabilityQueryManager.java b/src/main/java/org/dependencytrack/persistence/VulnerabilityQueryManager.java index 7ccbe5540..8684de1eb 100644 --- a/src/main/java/org/dependencytrack/persistence/VulnerabilityQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/VulnerabilityQueryManager.java @@ -246,15 +246,16 @@ public void addVulnerability(Vulnerability vulnerability, Component component, A * @param component the component unaffected by the vulnerabiity */ public void removeVulnerability(Vulnerability vulnerability, Component component) { - if (contains(vulnerability, component)) { - pm.currentTransaction().begin(); - component.removeVulnerability(vulnerability); - pm.currentTransaction().commit(); - } - final FindingAttribution fa = getFindingAttribution(vulnerability, component); - if (fa != null) { - delete(fa); - } + runInTransaction(() -> { + if (contains(vulnerability, component)) { + component.removeVulnerability(vulnerability); + } + + final FindingAttribution fa = getFindingAttribution(vulnerability, component); + if (fa != null) { + delete(fa); + } + }); } /** @@ -640,7 +641,7 @@ SELECT COUNT(DISTINCT this.project.id) } public synchronized VulnerabilityAlias synchronizeVulnerabilityAlias(final VulnerabilityAlias alias) { - return runInTransaction(() -> { + return callInTransaction(() -> { // Query existing aliases that match AT LEAST ONE identifier of the given alias. // // For each data source, we want to know the existing aliases where the respective identifier either: diff --git a/src/main/java/org/dependencytrack/resources/v1/ProjectResource.java b/src/main/java/org/dependencytrack/resources/v1/ProjectResource.java index d551f7973..8cf412017 100644 --- a/src/main/java/org/dependencytrack/resources/v1/ProjectResource.java +++ b/src/main/java/org/dependencytrack/resources/v1/ProjectResource.java @@ -671,7 +671,7 @@ public Response cloneProject(CloneProjectRequest jsonRequest) { } LOGGER.info("Project " + sourceProject + " is being cloned by " + super.getPrincipal().getName()); CloneProjectEvent event = new CloneProjectEvent(jsonRequest); - final Response response = qm.runInTransaction(() -> { + final Response response = qm.callInTransaction(() -> { WorkflowState workflowState = qm.getWorkflowStateByTokenAndStep(event.getChainIdentifier(), WorkflowStep.PROJECT_CLONE); if (workflowState != null) { if (isEventBeingProcessed(event.getChainIdentifier()) || !workflowState.getStatus().isTerminal()) { diff --git a/src/main/java/org/dependencytrack/resources/v1/TeamResource.java b/src/main/java/org/dependencytrack/resources/v1/TeamResource.java index b57ef944d..86b732b8e 100644 --- a/src/main/java/org/dependencytrack/resources/v1/TeamResource.java +++ b/src/main/java/org/dependencytrack/resources/v1/TeamResource.java @@ -273,7 +273,7 @@ public Response updateApiKeyComment(@PathParam("apikey") final String apikey, try (final var qm = new QueryManager()) { qm.getPersistenceManager().setProperty(PROPERTY_RETAIN_VALUES, "true"); - return qm.runInTransaction(() -> { + return qm.callInTransaction(() -> { final ApiKey apiKey = qm.getApiKey(apikey); if (apiKey == null) { return Response diff --git a/src/main/java/org/dependencytrack/resources/v1/VulnerabilityPolicyResource.java b/src/main/java/org/dependencytrack/resources/v1/VulnerabilityPolicyResource.java index d05b13e49..27ae34199 100644 --- a/src/main/java/org/dependencytrack/resources/v1/VulnerabilityPolicyResource.java +++ b/src/main/java/org/dependencytrack/resources/v1/VulnerabilityPolicyResource.java @@ -123,7 +123,7 @@ public Response triggerVulnerabilityPolicyBundleSync() { qm.getPersistenceManager().currentTransaction().setSerializeRead(true); final UUID token = VulnerabilityPolicyFetchEvent.CHAIN_IDENTIFIER; - final Response response = qm.runInTransaction(() -> { + final Response response = qm.callInTransaction(() -> { WorkflowState workflowState = qm.getWorkflowStateByTokenAndStep(token, WorkflowStep.POLICY_BUNDLE_SYNC); if (workflowState != null) { if (isEventBeingProcessed(token) || !workflowState.getStatus().isTerminal()) { diff --git a/src/main/java/org/dependencytrack/tasks/BomUploadProcessingTask.java b/src/main/java/org/dependencytrack/tasks/BomUploadProcessingTask.java index 66d310711..287a5466f 100644 --- a/src/main/java/org/dependencytrack/tasks/BomUploadProcessingTask.java +++ b/src/main/java/org/dependencytrack/tasks/BomUploadProcessingTask.java @@ -346,7 +346,7 @@ private ProcessedBom processBom(final Context ctx, final ConsumedBom bom) { // See https://www.datanucleus.org/products/accessplatform_6_0/jdo/persistence.html#lifecycle qm.getPersistenceManager().setProperty(PROPERTY_RETAIN_VALUES, "true"); - return qm.runInTransaction(() -> { + return qm.callInTransaction(() -> { final Project persistentProject = processProject(ctx, qm, bom.project(), bom.projectMetadata()); LOGGER.info("Processing %d components".formatted(bom.components().size())); @@ -1031,7 +1031,7 @@ private static List createRepoMetaAnalysis continue; } - final boolean shouldFetchIntegrityData = qm.runInTransaction(() -> prepareIntegrityMetaComponent(qm, component)); + final boolean shouldFetchIntegrityData = qm.callInTransaction(() -> prepareIntegrityMetaComponent(qm, component)); if (shouldFetchIntegrityData) { events.add(new ComponentRepositoryMetaAnalysisEvent( component.getUuid(), diff --git a/src/main/java/org/dependencytrack/tasks/WorkflowStateCleanupTask.java b/src/main/java/org/dependencytrack/tasks/WorkflowStateCleanupTask.java index f36ab97cb..87eff5a87 100644 --- a/src/main/java/org/dependencytrack/tasks/WorkflowStateCleanupTask.java +++ b/src/main/java/org/dependencytrack/tasks/WorkflowStateCleanupTask.java @@ -142,7 +142,7 @@ private static void transitionTimedOutStepsToFailed(final QueryManager qm, final int stepsCancelled = 0; try { for (final WorkflowState state : failedQuery.executeList()) { - stepsCancelled += qm.runInTransaction(() -> { + stepsCancelled += qm.callInTransaction(() -> { final Date now = new Date(); state.setStatus(WorkflowStatus.FAILED); state.setFailureReason("Timed out"); @@ -207,7 +207,7 @@ HAVING max(updatedAt) < :cutoff ).isEmpty() """); try { - stepsDeleted += qm.runInTransaction(() -> (long) workflowDeleteQuery.executeWithMap(Map.of( + stepsDeleted += qm.callInTransaction(() -> (long) workflowDeleteQuery.executeWithMap(Map.of( "tokens", tokenBatch, "nonTerminalStatuses", Set.of(WorkflowStatus.PENDING, WorkflowStatus.TIMED_OUT) ))); diff --git a/src/main/resources/logback.xml b/src/main/resources/logback.xml index 491d67174..00e4660a0 100644 --- a/src/main/resources/logback.xml +++ b/src/main/resources/logback.xml @@ -1,5 +1,7 @@ + + %date %level [%logger{0}] %msg%replace( [%mdc{}]){' \[\]', ''}%n