Skip to content

Commit

Permalink
Replace manual transaction commits with callInTransaction
Browse files Browse the repository at this point in the history
This is to properly support nested transactions as introduced in stevespringett/Alpine#552.

Signed-off-by: nscuro <nscuro@protonmail.com>
  • Loading branch information
nscuro authored and MM-msr committed Jun 18, 2024
1 parent 2b63328 commit 8963ef9
Show file tree
Hide file tree
Showing 6 changed files with 64 additions and 80 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -18,21 +18,19 @@
*/
package org.dependencytrack.persistence;

import java.util.Date;
import java.util.List;

import javax.jdo.PersistenceManager;
import javax.jdo.Query;

import alpine.persistence.PaginatedResult;
import alpine.resources.AlpineRequest;
import org.dependencytrack.model.Component;
import org.dependencytrack.model.DependencyMetrics;
import org.dependencytrack.model.PortfolioMetrics;
import org.dependencytrack.model.Project;
import org.dependencytrack.model.ProjectMetrics;
import org.dependencytrack.model.VulnerabilityMetrics;

import alpine.persistence.PaginatedResult;
import alpine.resources.AlpineRequest;
import javax.jdo.PersistenceManager;
import javax.jdo.Query;
import java.util.Date;
import java.util.List;

public class MetricsQueryManager extends QueryManager implements IQueryManager {

Expand Down Expand Up @@ -164,17 +162,14 @@ public List<DependencyMetrics> getDependencyMetricsSince(Component component, Da
}

public void synchronizeVulnerabilityMetrics(List<VulnerabilityMetrics> 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<VulnerabilityMetrics> 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(() -> {
// No need for complex updating, just replace the existing ~400 rows with new ones.
pm.newQuery(Query.JDOQL, "DELETE FROM org.dependencytrack.model.VulnerabilityMetrics").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);
});
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -252,19 +252,17 @@ private NotificationPublisher getDefaultNotificationPublisher(final String publi
public NotificationPublisher createNotificationPublisher(final String name, final String description,
final Class<? extends Publisher> publisherClass, final String templateContent,
final String templateMimeType, final boolean defaultPublisher, final boolean publishScheduled) {
pm.currentTransaction().begin();
final NotificationPublisher publisher = new NotificationPublisher();
publisher.setName(name);
publisher.setDescription(description);
publisher.setPublisherClass(publisherClass.getName());
publisher.setTemplate(templateContent);
publisher.setTemplateMimeType(templateMimeType);
publisher.setDefaultPublisher(defaultPublisher);
publisher.setPublishScheduled(publishScheduled);
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.getName());
publisher.setTemplate(templateContent);
publisher.setTemplateMimeType(templateMimeType);
publisher.setDefaultPublisher(defaultPublisher);
publisher.setPublishScheduled(publishScheduled);
return pm.makePersistent(publisher);
});
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -816,25 +816,26 @@ public List<ProjectProperty> 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<Tag> tags) {
final Query<Tag> query = pm.newQuery(Tag.class, "projects.contains(:project)");
final List<Tag> currentProjectTags = (List<Tag>)query.execute(project);
pm.currentTransaction().begin();
for (final Tag tag: currentProjectTags) {
if (!tags.contains(tag)) {
tag.getProjects().remove(project);
runInTransaction(() -> {
final Query<Tag> query = pm.newQuery(Tag.class, "projects.contains(:project)");
query.setParameters(project);
final List<Tag> 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<Project> projects = tag.getProjects();
if (!projects.contains(project)) {
projects.add(project);
project.setTags(tags);
for (final Tag tag : tags) {
final List<Project> projects = tag.getProjects();
if (!projects.contains(project)) {
projects.add(project);
}
}
}
pm.currentTransaction().commit();
});
}

/**
Expand Down
21 changes: 10 additions & 11 deletions src/main/java/org/dependencytrack/persistence/QueryManager.java
Original file line number Diff line number Diff line change
Expand Up @@ -1469,17 +1469,16 @@ public void ensureNoActiveTransaction() {
}

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 \"PROJECT_ACCESS_TEAMS\".\"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());

final Query<?> aclDeleteQuery = pm.newQuery(JDOQuery.SQL_QUERY_LANGUAGE, """
DELETE FROM "PROJECT_ACCESS_TEAMS" WHERE "PROJECT_ACCESS_TEAMS"."TEAM_ID" = ?""");
aclDeleteQuery.setParameters(team.getId());
executeAndClose(aclDeleteQuery);

pm.deletePersistent(team);
});
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,9 +46,9 @@
import java.util.Comparator;
import java.util.Date;
import java.util.HashMap;
import java.util.UUID;
import java.util.List;
import java.util.Map;
import java.util.UUID;
import java.util.stream.Collectors;

final class VulnerabilityQueryManager extends QueryManager implements IQueryManager {
Expand Down Expand Up @@ -277,15 +277,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);
}
});
}

/**
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,6 @@

import javax.jdo.PersistenceManager;
import javax.jdo.Query;
import javax.jdo.Transaction;
import java.time.Duration;
import java.time.Instant;
import java.util.List;
Expand Down Expand Up @@ -95,20 +94,11 @@ private void analyze() throws Exception {
}

if (component.isInternal() != internal) {
final Transaction trx = pm.currentTransaction();
try {
trx.begin();
component.setInternal(internal);
trx.commit();
} finally {
if (trx.isActive()) {
trx.rollback();
}
}
qm.runInTransaction(() -> component.setInternal(internal));
}
}

final long lastId = components.get(components.size() - 1).getId();
final long lastId = components.getLast().getId();
components = fetchNextComponentsPage(pm, lastId);
}
}
Expand Down

0 comments on commit 8963ef9

Please sign in to comment.