fix(db): correct use of Transactional annotation (#415)

compose/cryostat.yml

@@ -26,7 +26,7 @@ services:
       io.cryostat.jmxPort: "0"
       io.cryostat.jmxUrl: "service:jmx:rmi:///jndi/rmi://localhost:0/jmxrmi"
     environment:
-      QUARKUS_LOG_LEVEL: TRACE
+      QUARKUS_LOG_LEVEL: ALL
       QUARKUS_HTTP_HOST: "cryostat"
       QUARKUS_HTTP_PORT: ${CRYOSTAT_HTTP_PORT}
       QUARKUS_HIBERNATE_ORM_LOG_SQL: "true"

compose/db-viewer.yml

@@ -4,7 +4,7 @@ services:
     depends_on:
       db:
         condition: service_healthy
-    image: ${DB_VIEWER_IMAGE:-docker.io/dpage/pgadmin4:7}
+    image: ${DB_VIEWER_IMAGE:-docker.io/dpage/pgadmin4:8}
     hostname: db-viewer
     ports:
       - "8989:8989"

compose/sample-apps.yml

@@ -103,7 +103,18 @@ services:
     expose:
       - "9977"
     environment:
-      JAVA_OPTS_APPEND: "-Dquarkus.http.host=0.0.0.0 -Djava.util.logging.manager=org.jboss.logmanager.LogManager -javaagent:/deployments/app/cryostat-agent.jar"
+      JAVA_OPTS_APPEND: >-
+        -Dquarkus.http.host=0.0.0.0
+        -Djava.util.logging.manager=org.jboss.logmanager.LogManager
+        -javaagent:/deployments/app/cryostat-agent.jar
+        -Dcom.sun.management.jmxremote.autodiscovery=false
+        -Dcom.sun.management.jmxremote
+        -Dcom.sun.management.jmxremote.port=22222
+        -Dcom.sun.management.jmxremote.rmi.port=22222
+        -Djava.rmi.server.hostname=quarkus-test-agent
+        -Dcom.sun.management.jmxremote.authenticate=false
+        -Dcom.sun.management.jmxremote.ssl=false
+        -Dcom.sun.management.jmxremote.local.only=false
       QUARKUS_HTTP_PORT: 10010
       ORG_ACME_CRYOSTATSERVICE_ENABLED: "false"
       CRYOSTAT_AGENT_APP_NAME: quarkus-test-agent

src/main/java/io/cryostat/credentials/Credential.java

@@ -15,13 +15,16 @@
  */
 package io.cryostat.credentials;
 
+import io.cryostat.discovery.DiscoveryPlugin;
 import io.cryostat.expressions.MatchExpression;
 import io.cryostat.ws.MessagingServer;
 import io.cryostat.ws.Notification;
 
+import com.fasterxml.jackson.annotation.JsonIgnore;
 import com.fasterxml.jackson.annotation.JsonProperty;
 import io.quarkus.hibernate.orm.panache.PanacheEntity;
 import io.vertx.mutiny.core.eventbus.EventBus;
+import jakarta.annotation.Nullable;
 import jakarta.enterprise.context.ApplicationScoped;
 import jakarta.inject.Inject;
 import jakarta.persistence.CascadeType;
@@ -68,6 +71,16 @@ public class Credential extends PanacheEntity {
     @JsonProperty(access = JsonProperty.Access.WRITE_ONLY)
     public String password;
 
+    @OneToOne(
+            optional = true,
+            fetch = FetchType.LAZY,
+            mappedBy = "credential",
+            cascade = CascadeType.REMOVE)
+    @JoinColumn(name = "discoveryPlugin_id")
+    @JsonIgnore
+    @Nullable
+    public DiscoveryPlugin discoveryPlugin;
+
     @ApplicationScoped
     static class Listener {
         @Inject EventBus bus;

src/main/java/io/cryostat/credentials/Credentials.java

@@ -98,8 +98,7 @@ public RestResponse<Void> create(
     @RolesAllowed("write")
     @Path("/{id}")
     public void delete(@RestPath long id) {
-        Credential credential = Credential.find("id", id).singleResult();
-        credential.delete();
+        Credential.find("id", id).singleResult().delete();
     }
 
     static Map<String, Object> notificationResult(Credential credential) throws ScriptException {

src/main/java/io/cryostat/discovery/CustomDiscovery.java

@@ -34,8 +34,8 @@
 import io.cryostat.targets.Target.Annotations;
 import io.cryostat.targets.TargetConnectionManager;
 
+import io.quarkus.narayana.jta.QuarkusTransaction;
 import io.quarkus.runtime.StartupEvent;
-import io.smallrye.common.annotation.Blocking;
 import io.vertx.mutiny.core.eventbus.EventBus;
 import jakarta.annotation.security.RolesAllowed;
 import jakarta.enterprise.context.ApplicationScoped;
@@ -128,13 +128,15 @@ public Response create(
         return doV2Create(target, Optional.ofNullable(credential), dryrun, storeCredentials);
     }
 
-    @Transactional
-    @Blocking
     Response doV2Create(
             Target target,
             Optional<Credential> credential,
             boolean dryrun,
             boolean storeCredentials) {
+        var beginTx = !QuarkusTransaction.isActive();
+        if (beginTx) {
+            QuarkusTransaction.begin();
+        }
         try {
             target.connectUrl = sanitizeConnectUrl(target.connectUrl.toString());
 
@@ -178,10 +180,17 @@ Response doV2Create(
             node.persist();
             realm.persist();
 
+            if (beginTx) {
+                QuarkusTransaction.commit();
+            }
+
             return Response.created(URI.create("/api/v3/targets/" + target.id))
                     .entity(V2Response.json(Response.Status.CREATED, target))
                     .build();
         } catch (Exception e) {
+            // roll back regardless of whether we initiated this database transaction or a caller
+            // did
+            QuarkusTransaction.rollback();
             if (ExceptionUtils.indexOfType(e, ConstraintViolationException.class) >= 0) {
                 logger.warn("Invalid target definition", e);
                 return Response.status(Response.Status.BAD_REQUEST.getStatusCode())

src/main/java/io/cryostat/discovery/Discovery.java

@@ -30,11 +30,9 @@
 import java.util.List;
 import java.util.Map;
 import java.util.Objects;
-import java.util.Optional;
 import java.util.Set;
 import java.util.UUID;
 
-import io.cryostat.credentials.Credential;
 import io.cryostat.discovery.DiscoveryPlugin.PluginCallback;
 import io.cryostat.targets.TargetConnectionManager;
 
@@ -43,6 +41,7 @@
 import com.nimbusds.jose.JOSEException;
 import com.nimbusds.jwt.proc.BadJWTException;
 import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+import io.quarkus.narayana.jta.QuarkusTransaction;
 import io.quarkus.runtime.ShutdownEvent;
 import io.quarkus.runtime.StartupEvent;
 import io.vertx.core.json.JsonObject;
@@ -64,6 +63,7 @@
 import jakarta.ws.rs.core.Context;
 import jakarta.ws.rs.core.MediaType;
 import jakarta.ws.rs.core.Response;
+import jakarta.ws.rs.core.UriBuilder;
 import org.apache.commons.lang3.StringUtils;
 import org.eclipse.microprofile.config.inject.ConfigProperty;
 import org.jboss.logging.Logger;
@@ -180,6 +180,7 @@ public RestResponse<Void> checkRegistration(
     @Produces(MediaType.APPLICATION_JSON)
     @Consumes(MediaType.APPLICATION_JSON)
     @RolesAllowed("write")
+    @SuppressFBWarnings("DLS_DEAD_LOCAL_STORE")
     public Response register(@Context RoutingContext ctx, JsonObject body)
             throws URISyntaxException,
                     JOSEException,
@@ -192,6 +193,7 @@ public Response register(@Context RoutingContext ctx, JsonObject body)
         String priorToken = body.getString("token");
         String realmName = body.getString("realm");
         URI callbackUri = new URI(body.getString("callback"));
+        URI unauthCallback = UriBuilder.fromUri(callbackUri).userInfo(null).build();
 
         // TODO apply URI range validation to the remote address
         InetAddress remoteAddress = getRemoteAddress(ctx);
@@ -205,22 +207,46 @@ public Response register(@Context RoutingContext ctx, JsonObject body)
             if (!Objects.equals(plugin.realm.name, realmName)) {
                 throw new ForbiddenException();
             }
-            if (!Objects.equals(plugin.callback, callbackUri)) {
+            if (!Objects.equals(plugin.callback, unauthCallback)) {
                 throw new BadRequestException();
             }
             location = jwtFactory.getPluginLocation(plugin);
             jwtFactory.parseDiscoveryPluginJwt(plugin, priorToken, location, remoteAddress, false);
         } else {
+            // check if a plugin record with the same callback already exists. If it does, ping it:
+            // if it's still there reject this request as a duplicate, otherwise delete the previous
+            // record and accept this new one as a replacement
+            DiscoveryPlugin.<DiscoveryPlugin>find("callback", unauthCallback)
+                    .singleResultOptional()
+                    .ifPresent(
+                            p -> {
+                                try {
+                                    var cb = PluginCallback.create(p);
+                                    cb.ping();
+                                    throw new IllegalArgumentException(
+                                            String.format(
+                                                    "Plugin with callback %s already exists and is"
+                                                            + " still reachable",
+                                                    unauthCallback));
+                                } catch (Exception e) {
+                                    logger.error(e);
+                                    p.delete();
+                                }
+                            });
+
             // new plugin registration
             plugin = new DiscoveryPlugin();
             plugin.callback = callbackUri;
             plugin.realm =
                     DiscoveryNode.environment(
                             requireNonBlank(realmName, "realm"), BaseNodeType.REALM);
             plugin.builtin = false;
-            plugin.persist();
 
-            DiscoveryNode.getUniverse().children.add(plugin.realm);
+            var universe = DiscoveryNode.getUniverse();
+            plugin.realm.parent = universe;
+            plugin.persist();
+            universe.children.add(plugin.realm);
+            universe.persist();
 
             location = jwtFactory.getPluginLocation(plugin);
 
@@ -293,15 +319,15 @@ public Map<String, Map<String, String>> publish(
         DiscoveryPlugin plugin = DiscoveryPlugin.find("id", id).singleResult();
         jwtValidator.validateJwt(ctx, plugin, token, true);
         plugin.realm.children.clear();
-        plugin.persist();
         plugin.realm.children.addAll(body);
-        body.forEach(
-                b -> {
-                    if (b.target != null) {
-                        b.target.discoveryNode = b;
-                    }
-                    b.persist();
-                });
+        for (var b : body) {
+            if (b.target != null) {
+                b.target.discoveryNode = b;
+                b.target.discoveryNode.parent = plugin.realm;
+                b.parent = plugin.realm;
+            }
+            b.persist();
+        }
         plugin.persist();
 
         return Map.of(
@@ -338,10 +364,7 @@ public Map<String, Map<String, String>> deregister(
             scheduler.deleteJob(key);
         }
 
-        plugin.realm.delete();
         plugin.delete();
-        getStoredCredential(plugin).ifPresent(Credential::delete);
-        DiscoveryNode.getUniverse().children.remove(plugin.realm);
         return Map.of(
                 "meta",
                         Map.of(
@@ -374,17 +397,11 @@ public DiscoveryPlugin getPlugin(@RestPath UUID id) throws JsonProcessingExcepti
         return DiscoveryPlugin.find("id", id).singleResult();
     }
 
-    Optional<Credential> getStoredCredential(DiscoveryPlugin plugin) {
-        return new DiscoveryPlugin.PluginCallback.DiscoveryPluginAuthorizationHeaderFactory(plugin)
-                .getCredential();
-    }
-
+    @SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_OF_NULL_VALUE")
     static class RefreshPluginJob implements Job {
         @Inject Logger logger;
 
         @Override
-        @Transactional
-        @SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_OF_NULL_VALUE")
         public void execute(JobExecutionContext context) throws JobExecutionException {
             DiscoveryPlugin plugin = null;
             try {
@@ -407,14 +424,12 @@ public void execute(JobExecutionContext context) throws JobExecutionException {
                 if (plugin != null) {
                     logger.debugv(
                             e, "Pruned discovery plugin: {0} @ {1}", plugin.realm, plugin.callback);
-                    plugin.realm.delete();
-                    plugin.delete();
-                    new DiscoveryPlugin.PluginCallback.DiscoveryPluginAuthorizationHeaderFactory(
-                                    plugin)
-                            .getCredential()
-                            .ifPresent(Credential::delete);
+                    QuarkusTransaction.requiringNew().run(plugin::delete);
+                } else {
+                    var ex = new JobExecutionException(e);
+                    ex.setUnscheduleFiringTrigger(true);
+                    throw ex;
                 }
-                throw new JobExecutionException(e);
             }
         }
     }

src/main/java/io/cryostat/discovery/DiscoveryNode.java

@@ -30,6 +30,7 @@
 import com.fasterxml.jackson.annotation.JsonInclude.Include;
 import com.fasterxml.jackson.annotation.JsonView;
 import io.quarkus.hibernate.orm.panache.PanacheEntity;
+import io.quarkus.narayana.jta.QuarkusTransaction;
 import io.vertx.mutiny.core.eventbus.EventBus;
 import jakarta.annotation.Nullable;
 import jakarta.enterprise.context.ApplicationScoped;
@@ -130,25 +131,33 @@ public static List<DiscoveryNode> findAllByNodeType(NodeType nodeType) {
     }
 
     public static DiscoveryNode environment(String name, NodeType nodeType) {
-        DiscoveryNode node = new DiscoveryNode();
-        node.name = name;
-        node.nodeType = nodeType.getKind();
-        node.labels = new HashMap<>();
-        node.children = new ArrayList<>();
-        node.target = null;
-        node.persist();
-        return node;
+        return QuarkusTransaction.joiningExisting()
+                .call(
+                        () -> {
+                            DiscoveryNode node = new DiscoveryNode();
+                            node.name = name;
+                            node.nodeType = nodeType.getKind();
+                            node.labels = new HashMap<>();
+                            node.children = new ArrayList<>();
+                            node.target = null;
+                            node.persist();
+                            return node;
+                        });
     }
 
     public static DiscoveryNode target(Target target, NodeType nodeType) {
-        DiscoveryNode node = new DiscoveryNode();
-        node.name = target.connectUrl.toString();
-        node.nodeType = nodeType.getKind();
-        node.labels = new HashMap<>(target.labels);
-        node.children = null;
-        node.target = target;
-        node.persist();
-        return node;
+        return QuarkusTransaction.joiningExisting()
+                .call(
+                        () -> {
+                            DiscoveryNode node = new DiscoveryNode();
+                            node.name = target.connectUrl.toString();
+                            node.nodeType = nodeType.getKind();
+                            node.labels = new HashMap<>(target.labels);
+                            node.children = null;
+                            node.target = target;
+                            node.persist();
+                            return node;
+                        });
     }
 
     @Override