From 9cdccf427ea4a009a9c8fb99c0eb37547a670919 Mon Sep 17 00:00:00 2001 From: nscuro Date: Mon, 10 Jul 2023 14:35:09 +0200 Subject: [PATCH 1/3] Re-implement `recursivelyDelete` for `Component` to be more performant and transactional Signed-off-by: nscuro --- .../persistence/ComponentQueryManager.java | 58 +++++++--- .../persistence/QueryManager.java | 16 +++ .../ComponentQueryManagerTest.java | 109 ++++++++++++++++++ 3 files changed, 166 insertions(+), 17 deletions(-) create mode 100644 src/test/java/org/dependencytrack/persistence/ComponentQueryManagerTest.java diff --git a/src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java b/src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java index f57bebd8a..06e56b3d6 100644 --- a/src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java @@ -33,9 +33,9 @@ import org.dependencytrack.model.RepositoryMetaComponent; import org.dependencytrack.model.RepositoryType; -import javax.jdo.FetchPlan; import javax.jdo.PersistenceManager; import javax.jdo.Query; +import javax.jdo.Transaction; import javax.json.Json; import javax.json.JsonArray; import javax.json.JsonValue; @@ -389,26 +389,50 @@ protected void deleteComponents(Project project) { * @param commitIndex specifies if the search index should be committed (an expensive operation) */ public void recursivelyDelete(Component component, boolean commitIndex) { - if (component.getChildren() != null) { - for (final Component child: component.getChildren()) { + final Transaction trx = pm.currentTransaction(); + final boolean isJoiningExistingTrx = trx.isActive(); + try { + if (!isJoiningExistingTrx) { + trx.begin(); + } + + for (final Component child : component.getChildren()) { recursivelyDelete(child, false); + pm.flush(); } - } - pm.getFetchPlan().setDetachmentOptions(FetchPlan.DETACH_LOAD_FIELDS); - try { - // final Component result = pm.getObjectById(Component.class, component.getId()); - // Event.dispatch(new IndexEvent(IndexEvent.Action.DELETE, pm.detachCopy(result))); - deleteAnalysisTrail(component); - deleteViolationAnalysisTrail(component); - deleteMetrics(component); - deleteFindingAttributions(component); - deletePolicyViolations(component); - delete(component); + + // Use bulk DELETE queries to avoid having to fetch every single object from the database first. + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.AnalysisComment WHERE analysis.component == :component"), component); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.Analysis WHERE component == :component"), component); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.ViolationAnalysisComment WHERE violationAnalysis.component == :component"), component); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.ViolationAnalysis WHERE component == :component"), component); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.DependencyMetrics WHERE component == :component"), component); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.FindingAttribution WHERE component == :component"), component); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.PolicyViolation WHERE component == :component"), component); + + // Detach the component now; We'll need it to update the Lucene index post commit. + final Component detachedComponent = pm.detachCopy(component); + + // The component itself must be deleted via deletePersistentAll, otherwise relationships + // (e.g. with Vulnerability via COMPONENTS_VULNERABILITIES table) will not be cleaned up properly. + final Query componentQuery = pm.newQuery(Component.class, "this == :component"); + try { + componentQuery.deletePersistentAll(component); + } finally { + componentQuery.closeAll(); + } + + if (!isJoiningExistingTrx) { + trx.commit(); + } + + // Event.dispatch(new IndexEvent(IndexEvent.Action.DELETE, detachedComponent)); // commitSearchIndex(commitIndex, Component.class); - } catch (javax.jdo.JDOObjectNotFoundException | org.datanucleus.exceptions.NucleusObjectNotFoundException e) { - LOGGER.warn("Deletion of component failed because it didn't exist anymore."); + } finally { + if (!isJoiningExistingTrx && trx.isActive()) { + trx.rollback(); + } } - } /** diff --git a/src/main/java/org/dependencytrack/persistence/QueryManager.java b/src/main/java/org/dependencytrack/persistence/QueryManager.java index e13234c2d..07ceab35c 100644 --- a/src/main/java/org/dependencytrack/persistence/QueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/QueryManager.java @@ -1517,4 +1517,20 @@ public VulnerabilityScan recordVulnerabilityScanResult(final String scanToken) { public VulnerableSoftware getVulnerableSoftwareByPurlAndVersion(String purlType, String purlNamespace, String purlName, String version) { return getVulnerableSoftwareQueryManager().getVulnerableSoftwareByPurlAndVersion(purlType, purlNamespace, purlName, version); } + + /** + * Execute a give {@link Query} and ensure that resources associated with it are released post execution. + * + * @param query The {@link Query} to execute + * @param parameters The parameters of the query + * @return The result of the query + */ + public Object executeAndClose(final Query query, final Object... parameters) { + try { + return query.executeWithArray(parameters); + } finally { + query.closeAll(); + } + } + } diff --git a/src/test/java/org/dependencytrack/persistence/ComponentQueryManagerTest.java b/src/test/java/org/dependencytrack/persistence/ComponentQueryManagerTest.java new file mode 100644 index 000000000..17e3e9017 --- /dev/null +++ b/src/test/java/org/dependencytrack/persistence/ComponentQueryManagerTest.java @@ -0,0 +1,109 @@ +package org.dependencytrack.persistence; + +import org.dependencytrack.PersistenceCapableTest; +import org.dependencytrack.model.Analysis; +import org.dependencytrack.model.AnalysisJustification; +import org.dependencytrack.model.AnalysisResponse; +import org.dependencytrack.model.AnalysisState; +import org.dependencytrack.model.AnalyzerIdentity; +import org.dependencytrack.model.Component; +import org.dependencytrack.model.DependencyMetrics; +import org.dependencytrack.model.Policy; +import org.dependencytrack.model.PolicyCondition; +import org.dependencytrack.model.PolicyViolation; +import org.dependencytrack.model.Project; +import org.dependencytrack.model.ViolationAnalysis; +import org.dependencytrack.model.ViolationAnalysisState; +import org.dependencytrack.model.Vulnerability; +import org.junit.Test; + +import javax.jdo.JDOObjectNotFoundException; +import java.util.Date; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatNoException; + +public class ComponentQueryManagerTest extends PersistenceCapableTest { + + @Test + public void recursivelyDeleteTest() { + final var project = new Project(); + project.setName("acme-app"); + project.setVersion("1.0.0"); + qm.persist(project); + + final var component = new Component(); + component.setProject(project); + component.setName("acme-lib"); + component.setVersion("2.0.0"); + qm.persist(component); + + // Assign a vulnerability and an accompanying analysis with comments to component. + final var vuln = new Vulnerability(); + vuln.setVulnId("INT-123"); + vuln.setSource(Vulnerability.Source.INTERNAL); + qm.persist(vuln); + qm.addVulnerability(vuln, component, AnalyzerIdentity.INTERNAL_ANALYZER); + final Analysis analysis = qm.makeAnalysis(component, vuln, + AnalysisState.NOT_AFFECTED, + AnalysisJustification.CODE_NOT_REACHABLE, + AnalysisResponse.WORKAROUND_AVAILABLE, + "analysisDetails", false); + qm.makeAnalysisComment(analysis, "someComment", "someCommenter"); + + // Create a child component to validate that deletion is indeed recursive. + final var componentChild = new Component(); + componentChild.setProject(project); + componentChild.setParent(component); + componentChild.setName("acme-sub-lib"); + componentChild.setVersion("3.0.0"); + qm.persist(componentChild); + + // Assign a policy violation and an accompanying analysis with comments to componentChild. + final var policy = new Policy(); + policy.setName("Test Policy"); + policy.setViolationState(Policy.ViolationState.WARN); + policy.setOperator(Policy.Operator.ALL); + qm.persist(policy); + final var policyCondition = new PolicyCondition(); + policyCondition.setPolicy(policy); + policyCondition.setSubject(PolicyCondition.Subject.COORDINATES); + policyCondition.setOperator(PolicyCondition.Operator.MATCHES); + policyCondition.setValue("someValue"); + qm.persist(policyCondition); + final var policyViolation = new PolicyViolation(); + policyViolation.setPolicyCondition(policyCondition); + policyViolation.setComponent(componentChild); + policyViolation.setType(PolicyViolation.Type.OPERATIONAL); + policyViolation.setTimestamp(new Date()); + qm.persist(policyViolation); + final ViolationAnalysis violationAnalysis = qm.makeViolationAnalysis(componentChild, policyViolation, + ViolationAnalysisState.REJECTED, false); + qm.makeViolationAnalysisComment(violationAnalysis, "someComment", "someCommenter"); + + // Create metrics for component. + final var metrics = new DependencyMetrics(); + metrics.setProject(project); + metrics.setComponent(component); + metrics.setFirstOccurrence(new Date()); + metrics.setLastOccurrence(new Date()); + qm.persist(metrics); + + assertThatNoException() + .isThrownBy(() -> qm.recursivelyDelete(component, false)); + + // Ensure everything has been deleted as expected. + assertThat(qm.getAllComponents(project)).isEmpty(); + assertThatExceptionOfType(JDOObjectNotFoundException.class).isThrownBy(() -> qm.getObjectById(Component.class, component.getId())); + assertThatExceptionOfType(JDOObjectNotFoundException.class).isThrownBy(() -> qm.getObjectById(Component.class, componentChild.getId())); + assertThatExceptionOfType(JDOObjectNotFoundException.class).isThrownBy(() -> qm.getObjectById(DependencyMetrics.class, metrics.getId())); + + // Ensure associated objects were NOT deleted. + assertThatNoException().isThrownBy(() -> qm.getObjectById(Project.class, project.getId())); + assertThatNoException().isThrownBy(() -> qm.getObjectById(Vulnerability.class, vuln.getId())); + assertThatNoException().isThrownBy(() -> qm.getObjectById(PolicyCondition.class, policyCondition.getId())); + assertThatNoException().isThrownBy(() -> qm.getObjectById(Policy.class, policy.getId())); + } + +} \ No newline at end of file From cfd99628ced955d36def530efb45492be5b24e44 Mon Sep 17 00:00:00 2001 From: nscuro Date: Mon, 10 Jul 2023 15:55:36 +0200 Subject: [PATCH 2/3] Re-implement `recursivelyDelete` for `Project` to be more performant and transactional Signed-off-by: nscuro --- .../persistence/ComponentQueryManager.java | 6 +- .../persistence/NotificationQueryManager.java | 12 +- .../persistence/PolicyQueryManager.java | 15 +- .../persistence/ProjectQueryManager.java | 75 +++++--- .../persistence/QueryManager.java | 4 + .../ServiceComponentQueryManager.java | 8 +- .../persistence/ProjectQueryManagerTest.java | 162 ++++++++++++++++++ 7 files changed, 248 insertions(+), 34 deletions(-) create mode 100644 src/test/java/org/dependencytrack/persistence/ProjectQueryManagerTest.java diff --git a/src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java b/src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java index 06e56b3d6..d9181cd2c 100644 --- a/src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java @@ -380,7 +380,11 @@ public Component updateComponent(Component transientComponent, boolean commitInd */ protected void deleteComponents(Project project) { final Query query = pm.newQuery(Component.class, "project == :project"); - query.deletePersistentAll(project); + try { + query.deletePersistentAll(project); + } finally { + query.closeAll(); + } } /** diff --git a/src/main/java/org/dependencytrack/persistence/NotificationQueryManager.java b/src/main/java/org/dependencytrack/persistence/NotificationQueryManager.java index 1c2558e2c..35ce8ec7b 100644 --- a/src/main/java/org/dependencytrack/persistence/NotificationQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/NotificationQueryManager.java @@ -212,9 +212,15 @@ public NotificationPublisher updateNotificationPublisher(NotificationPublisher t @SuppressWarnings("unchecked") public void removeProjectFromNotificationRules(final Project project) { final Query query = pm.newQuery(NotificationRule.class, "projects.contains(:project)"); - for (final NotificationRule rule: (List) query.execute(project)) { - rule.getProjects().remove(project); - persist(rule); + try { + for (final NotificationRule rule : (List) query.execute(project)) { + rule.getProjects().remove(project); + if (!pm.currentTransaction().isActive()) { + persist(rule); + } + } + } finally { + query.closeAll(); } } diff --git a/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java b/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java index c7757be4e..f2af10162 100644 --- a/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/PolicyQueryManager.java @@ -599,11 +599,18 @@ public void deletePolicyCondition(PolicyCondition policyCondition) { */ public void removeProjectFromPolicies(final Project project) { final Query query = pm.newQuery(Policy.class, "projects.contains(:project)"); - query.setParameters(project); + try { + query.setParameters(project); + + for (final Policy policy : query.executeList()) { + policy.getProjects().remove(project); - for (final Policy policy: query.executeList()) { - policy.getProjects().remove(project); - persist(policy); + if (!pm.currentTransaction().isActive()) { + persist(policy); + } + } + } finally { + query.closeAll(); } } diff --git a/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java b/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java index c5fabf5a4..d2883187c 100644 --- a/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java @@ -38,7 +38,6 @@ import org.dependencytrack.model.FindingAttribution; import org.dependencytrack.model.Project; import org.dependencytrack.model.ProjectProperty; -import org.dependencytrack.model.ServiceComponent; import org.dependencytrack.model.Tag; import org.dependencytrack.model.Vulnerability; import org.dependencytrack.notification.NotificationConstants; @@ -46,9 +45,9 @@ import org.dependencytrack.notification.NotificationScope; import org.dependencytrack.util.NotificationUtil; -import javax.jdo.FetchPlan; import javax.jdo.PersistenceManager; import javax.jdo.Query; +import javax.jdo.Transaction; import java.security.Principal; import java.util.ArrayList; import java.util.Date; @@ -681,34 +680,62 @@ public Project clone(UUID from, String newVersion, boolean includeTags, boolean * @param commitIndex specifies if the search index should be committed (an expensive operation) */ public void recursivelyDelete(final Project project, final boolean commitIndex) { - if (project.getChildren() != null) { + final Transaction trx = pm.currentTransaction(); + final boolean isJoiningExistingTrx = trx.isActive(); + try { + if (!isJoiningExistingTrx) { + trx.begin(); + } + for (final Project child : project.getChildren()) { + // Note: This could be refactored such that each project is deleted + // in its own transaction. That would break semantics when it comes + // to joining an existing transaction though, so needs a bit more thought. recursivelyDelete(child, false); + pm.flush(); } - } - pm.getFetchPlan().setDetachmentOptions(FetchPlan.DETACH_LOAD_FIELDS); - // final Project result = pm.getObjectById(Project.class, project.getId()); - // Event.dispatch(new IndexEvent(IndexEvent.Action.DELETE, pm.detachCopy(result))); - // commitSearchIndex(commitIndex, Project.class); - deleteAnalysisTrail(project); - deleteViolationAnalysisTrail(project); - deleteMetrics(project); - deleteFindingAttributions(project); - deletePolicyViolations(project); - deleteComponents(project); + // Use bulk DELETE queries to avoid having to fetch every single object from the database first. + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.AnalysisComment WHERE analysis.project == :project"), project); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.Analysis WHERE project == :project"), project); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.ViolationAnalysisComment WHERE violationAnalysis.project == :project"), project); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.ViolationAnalysis WHERE project == :project"), project); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.DependencyMetrics WHERE project == :project"), project); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.ProjectMetrics WHERE project == :project"), project); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.FindingAttribution WHERE project == :project"), project); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.PolicyViolation WHERE project == :project"), project); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.Bom WHERE project == :project"), project); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.Vex WHERE project == :project"), project); + executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.ProjectProperty WHERE project == :project"), project); + + // Projects, Components, and ServiceComponents must be deleted via deletePersistentAll, otherwise relationships + // (e.g. with Vulnerability via COMPONENTS_VULNERABILITIES table) will not be cleaned up properly. + deleteComponents(project); + deleteServiceComponents(project); + removeProjectFromNotificationRules(project); + removeProjectFromPolicies(project); + + // Detach the project now; We'll need it to update the Lucene index post commit. + final Project detachedProject = pm.detachCopy(project); + + final Query projectQuery = pm.newQuery(Project.class, "this == :project"); + try { + projectQuery.deletePersistentAll(project); + } finally { + projectQuery.closeAll(); + } + + if (!isJoiningExistingTrx) { + trx.commit(); + } - for (final ServiceComponent s : getAllServiceComponents(project)) { - recursivelyDelete(s, false); + // Event.dispatch(new IndexEvent(IndexEvent.Action.DELETE, detachedProject)); + // commitSearchIndex(commitIndex, Project.class); + } finally { + if (!isJoiningExistingTrx && trx.isActive()) { + trx.rollback(); + } } - deleteBoms(project); - deleteVexs(project); - removeProjectFromNotificationRules(project); - removeProjectFromPolicies(project); - delete(project.getProperties()); - delete(getAllBoms(project)); - delete(project.getChildren()); - delete(project); } /** diff --git a/src/main/java/org/dependencytrack/persistence/QueryManager.java b/src/main/java/org/dependencytrack/persistence/QueryManager.java index 07ceab35c..6081e529d 100644 --- a/src/main/java/org/dependencytrack/persistence/QueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/QueryManager.java @@ -977,6 +977,10 @@ public ServiceComponent updateServiceComponent(ServiceComponent transientService return getServiceComponentQueryManager().updateServiceComponent(transientServiceComponent, commitIndex); } + public void deleteServiceComponents(final Project project) { + getServiceComponentQueryManager().deleteServiceComponents(project); + } + public void recursivelyDelete(ServiceComponent service, boolean commitIndex) { getServiceComponentQueryManager().recursivelyDelete(service, commitIndex); } diff --git a/src/main/java/org/dependencytrack/persistence/ServiceComponentQueryManager.java b/src/main/java/org/dependencytrack/persistence/ServiceComponentQueryManager.java index 8105ac119..cd5dfcd3a 100644 --- a/src/main/java/org/dependencytrack/persistence/ServiceComponentQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/ServiceComponentQueryManager.java @@ -233,9 +233,13 @@ public ServiceComponent updateServiceComponent(ServiceComponent transientService * Deletes all services for the specified Project. * @param project the Project to delete services of */ - private void deleteServiceComponents(Project project) { + public void deleteServiceComponents(Project project) { final Query query = pm.newQuery(ServiceComponent.class, "project == :project"); - query.deletePersistentAll(project); + try { + query.deletePersistentAll(project); + } finally { + query.closeAll(); + } } /** diff --git a/src/test/java/org/dependencytrack/persistence/ProjectQueryManagerTest.java b/src/test/java/org/dependencytrack/persistence/ProjectQueryManagerTest.java new file mode 100644 index 000000000..aec6071b1 --- /dev/null +++ b/src/test/java/org/dependencytrack/persistence/ProjectQueryManagerTest.java @@ -0,0 +1,162 @@ +package org.dependencytrack.persistence; + +import alpine.notification.NotificationLevel; +import org.dependencytrack.PersistenceCapableTest; +import org.dependencytrack.model.Analysis; +import org.dependencytrack.model.AnalysisJustification; +import org.dependencytrack.model.AnalysisResponse; +import org.dependencytrack.model.AnalysisState; +import org.dependencytrack.model.AnalyzerIdentity; +import org.dependencytrack.model.Bom; +import org.dependencytrack.model.Component; +import org.dependencytrack.model.DependencyMetrics; +import org.dependencytrack.model.NotificationPublisher; +import org.dependencytrack.model.NotificationRule; +import org.dependencytrack.model.Policy; +import org.dependencytrack.model.PolicyCondition; +import org.dependencytrack.model.PolicyViolation; +import org.dependencytrack.model.Project; +import org.dependencytrack.model.ProjectMetrics; +import org.dependencytrack.model.Vex; +import org.dependencytrack.model.ViolationAnalysis; +import org.dependencytrack.model.ViolationAnalysisState; +import org.dependencytrack.model.Vulnerability; +import org.dependencytrack.notification.NotificationScope; +import org.junit.Test; + +import javax.jdo.JDOObjectNotFoundException; +import java.util.Date; +import java.util.List; +import java.util.UUID; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatNoException; + +public class ProjectQueryManagerTest extends PersistenceCapableTest { + + @Test + public void recursivelyDeleteTest() { + final var project = new Project(); + project.setName("acme-app"); + project.setVersion("1.0.0"); + qm.persist(project); + + final var component = new Component(); + component.setProject(project); + component.setName("acme-lib"); + component.setVersion("2.0.0"); + qm.persist(component); + + // Assign a vulnerability and an accompanying analysis with comments to component. + final var vuln = new Vulnerability(); + vuln.setVulnId("INT-123"); + vuln.setSource(Vulnerability.Source.INTERNAL); + qm.persist(vuln); + qm.addVulnerability(vuln, component, AnalyzerIdentity.INTERNAL_ANALYZER); + final Analysis analysis = qm.makeAnalysis(component, vuln, + AnalysisState.NOT_AFFECTED, + AnalysisJustification.CODE_NOT_REACHABLE, + AnalysisResponse.WORKAROUND_AVAILABLE, + "analysisDetails", false); + qm.makeAnalysisComment(analysis, "someComment", "someCommenter"); + + // Create a child component to validate that deletion is indeed recursive. + final var componentChild = new Component(); + componentChild.setProject(project); + componentChild.setParent(component); + componentChild.setName("acme-sub-lib"); + componentChild.setVersion("3.0.0"); + qm.persist(componentChild); + + // Assign a policy violation and an accompanying analysis with comments to componentChild. + final var policy = new Policy(); + policy.setName("Test Policy"); + policy.setViolationState(Policy.ViolationState.WARN); + policy.setOperator(Policy.Operator.ALL); + policy.setProjects(List.of(project)); + qm.persist(policy); + final var policyCondition = new PolicyCondition(); + policyCondition.setPolicy(policy); + policyCondition.setSubject(PolicyCondition.Subject.COORDINATES); + policyCondition.setOperator(PolicyCondition.Operator.MATCHES); + policyCondition.setValue("someValue"); + qm.persist(policyCondition); + final var policyViolation = new PolicyViolation(); + policyViolation.setPolicyCondition(policyCondition); + policyViolation.setComponent(componentChild); + policyViolation.setType(PolicyViolation.Type.OPERATIONAL); + policyViolation.setTimestamp(new Date()); + qm.persist(policyViolation); + final ViolationAnalysis violationAnalysis = qm.makeViolationAnalysis(componentChild, policyViolation, + ViolationAnalysisState.REJECTED, false); + qm.makeViolationAnalysisComment(violationAnalysis, "someComment", "someCommenter"); + + // Create metrics for component. + final var componentMetrics = new DependencyMetrics(); + componentMetrics.setProject(project); + componentMetrics.setComponent(component); + componentMetrics.setFirstOccurrence(new Date()); + componentMetrics.setLastOccurrence(new Date()); + qm.persist(componentMetrics); + + // Create metrics for project. + final var projectMetrics = new ProjectMetrics(); + projectMetrics.setProject(project); + projectMetrics.setFirstOccurrence(new Date()); + projectMetrics.setLastOccurrence(new Date()); + qm.persist(projectMetrics); + + // Create a BOM. + final Bom bom = qm.createBom(project, new Date(), Bom.Format.CYCLONEDX, "1.4", 1, "serialNumber", UUID.randomUUID()); + + // Create a child project with an accompanying component. + final var projectChild = new Project(); + projectChild.setParent(project); + projectChild.setName("acme-sub-app"); + projectChild.setVersion("1.1.0"); + qm.persist(projectChild); + final var projectChildComponent = new Component(); + projectChildComponent.setProject(projectChild); + projectChildComponent.setName("acme-lib-x"); + projectChildComponent.setVersion("4.0.0"); + qm.persist(projectChildComponent); + + // Create a VEX for projectChild. + final Vex vex = qm.createVex(projectChild, new Date(), Vex.Format.CYCLONEDX, "1.3", 1, "serialNumber"); + + // Create a notification rule and associate projectChild with it. + final NotificationPublisher notificationPublisher = qm.createNotificationPublisher("name", "description", "publisherClass", "templateContent", "templateMimeType", true); + final NotificationRule notificationRule = qm.createNotificationRule("name", NotificationScope.PORTFOLIO, NotificationLevel.WARNING, notificationPublisher); + notificationRule.getProjects().add(projectChild); + qm.persist(notificationRule); + + assertThatNoException() + .isThrownBy(() -> qm.recursivelyDelete(project, false)); + + // Ensure everything has been deleted as expected. + assertThatExceptionOfType(JDOObjectNotFoundException.class).isThrownBy(() -> qm.getObjectById(Project.class, project.getId())); + assertThatExceptionOfType(JDOObjectNotFoundException.class).isThrownBy(() -> qm.getObjectById(Project.class, projectChild.getId())); + assertThatExceptionOfType(JDOObjectNotFoundException.class).isThrownBy(() -> qm.getObjectById(Component.class, component.getId())); + assertThatExceptionOfType(JDOObjectNotFoundException.class).isThrownBy(() -> qm.getObjectById(Component.class, componentChild.getId())); + assertThatExceptionOfType(JDOObjectNotFoundException.class).isThrownBy(() -> qm.getObjectById(Component.class, projectChildComponent.getId())); + assertThatExceptionOfType(JDOObjectNotFoundException.class).isThrownBy(() -> qm.getObjectById(ProjectMetrics.class, projectMetrics.getId())); + assertThatExceptionOfType(JDOObjectNotFoundException.class).isThrownBy(() -> qm.getObjectById(DependencyMetrics.class, componentMetrics.getId())); + assertThatExceptionOfType(JDOObjectNotFoundException.class).isThrownBy(() -> qm.getObjectById(Bom.class, bom.getId())); + assertThatExceptionOfType(JDOObjectNotFoundException.class).isThrownBy(() -> qm.getObjectById(Vex.class, vex.getId())); + + // Ensure associated objects were NOT deleted. + assertThatNoException().isThrownBy(() -> qm.getObjectById(Vulnerability.class, vuln.getId())); + assertThatNoException().isThrownBy(() -> qm.getObjectById(PolicyCondition.class, policyCondition.getId())); + assertThatNoException().isThrownBy(() -> qm.getObjectById(Policy.class, policy.getId())); + assertThatNoException().isThrownBy(() -> qm.getObjectById(NotificationRule.class, notificationRule.getId())); + assertThatNoException().isThrownBy(() -> qm.getObjectById(NotificationPublisher.class, notificationPublisher.getId())); + + // Ensure that associations have been cleaned up. + qm.getPersistenceManager().refresh(notificationRule); + assertThat(notificationRule.getProjects()).isEmpty(); + qm.getPersistenceManager().refresh(policy); + assertThat(policy.getProjects()).isEmpty(); + } + +} \ No newline at end of file From 6ddfd087b43ffb0ec7a10a957ac561952fcc2447 Mon Sep 17 00:00:00 2001 From: nscuro Date: Mon, 17 Jul 2023 15:56:18 +0200 Subject: [PATCH 3/3] Re-implement `recursivelyDelete` for `ServiceComponent` to be more performant and transactional Signed-off-by: nscuro --- .../persistence/ComponentQueryManager.java | 3 -- .../persistence/ProjectQueryManager.java | 3 -- .../ServiceComponentQueryManager.java | 50 +++++++++++++------ 3 files changed, 36 insertions(+), 20 deletions(-) diff --git a/src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java b/src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java index d9181cd2c..7e43e23a1 100644 --- a/src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/ComponentQueryManager.java @@ -414,9 +414,6 @@ public void recursivelyDelete(Component component, boolean commitIndex) { executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.FindingAttribution WHERE component == :component"), component); executeAndClose(pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.PolicyViolation WHERE component == :component"), component); - // Detach the component now; We'll need it to update the Lucene index post commit. - final Component detachedComponent = pm.detachCopy(component); - // The component itself must be deleted via deletePersistentAll, otherwise relationships // (e.g. with Vulnerability via COMPONENTS_VULNERABILITIES table) will not be cleaned up properly. final Query componentQuery = pm.newQuery(Component.class, "this == :component"); diff --git a/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java b/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java index d2883187c..fb98da74c 100644 --- a/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/ProjectQueryManager.java @@ -715,9 +715,6 @@ public void recursivelyDelete(final Project project, final boolean commitIndex) removeProjectFromNotificationRules(project); removeProjectFromPolicies(project); - // Detach the project now; We'll need it to update the Lucene index post commit. - final Project detachedProject = pm.detachCopy(project); - final Query projectQuery = pm.newQuery(Project.class, "this == :project"); try { projectQuery.deletePersistentAll(project); diff --git a/src/main/java/org/dependencytrack/persistence/ServiceComponentQueryManager.java b/src/main/java/org/dependencytrack/persistence/ServiceComponentQueryManager.java index cd5dfcd3a..ac0375be1 100644 --- a/src/main/java/org/dependencytrack/persistence/ServiceComponentQueryManager.java +++ b/src/main/java/org/dependencytrack/persistence/ServiceComponentQueryManager.java @@ -24,9 +24,9 @@ import org.dependencytrack.model.Project; import org.dependencytrack.model.ServiceComponent; -import javax.jdo.FetchPlan; import javax.jdo.PersistenceManager; import javax.jdo.Query; +import javax.jdo.Transaction; import java.util.ArrayList; import java.util.List; @@ -248,21 +248,43 @@ public void deleteServiceComponents(Project project) { * @param commitIndex specifies if the search index should be committed (an expensive operation) */ public void recursivelyDelete(ServiceComponent service, boolean commitIndex) { - if (service.getChildren() != null) { - for (final ServiceComponent child: service.getChildren()) { + final Transaction trx = pm.currentTransaction(); + final boolean isJoiningExistingTrx = trx.isActive(); + try { + if (!isJoiningExistingTrx) { + trx.begin(); + } + + for (final ServiceComponent child : service.getChildren()) { recursivelyDelete(child, false); + pm.flush(); + } + + // TODO: Add these in when these features are supported by service components + //deleteAnalysisTrail(service); + //deleteViolationAnalysisTrail(service); + //deleteMetrics(service); + //deleteFindingAttributions(service); + //deletePolicyViolations(service); + + final Query query = pm.newQuery(ServiceComponent.class); + query.setFilter("this == :service"); + try { + query.deletePersistentAll(service); + } finally { + query.closeAll(); + } + + if (!isJoiningExistingTrx) { + trx.commit(); + } + + // Event.dispatch(new IndexEvent(IndexEvent.Action.DELETE, detachedService)); + // commitSearchIndex(commitIndex, ServiceComponent.class); + } finally { + if (!isJoiningExistingTrx && trx.isActive()) { + trx.rollback(); } } - pm.getFetchPlan().setDetachmentOptions(FetchPlan.DETACH_LOAD_FIELDS); - // final ServiceComponent result = pm.getObjectById(ServiceComponent.class, service.getId()); - // Event.dispatch(new IndexEvent(IndexEvent.Action.DELETE, pm.detachCopy(result))); - // TODO: Add these in when these features are supported by service components - //deleteAnalysisTrail(service); - //deleteViolationAnalysisTrail(service); - //deleteMetrics(service); - //deleteFindingAttributions(service); - //deletePolicyViolations(service); - delete(service); - // commitSearchIndex(commitIndex, ServiceComponent.class); } }