Support CDI interceptor annotations on `PanacheEntity` instance methods?

[neon-dev]

Describe the bug

Currently we can define interceptor annotations on PanacheEntity static methods like so:

@Entity
public class Something extends PanacheEntity {

  @Transactional
  public static void deleteTransactionalById(Long id) {
    deleteById(id);
  }
}

but we cannot define interceptors on PanacheEntity instance methods, because they are not "beans", so calling this will throw TransactionRequiredException:

  @Transactional
  public void deleteTransactional() {
    delete();
  }

If you are using your entities from a REST endpoint, you can easily annotate the endpoint, but if you're invoking entity methods from code such as jobs, this becomes more complicated and you have to resort to QuarkusTransaction.

This may be fine, but perhaps we need to consider supporting CDI interceptors on entities?

[quarkus-bot[bot]]

/cc @FroMage (panache), @loicmathieu (panache)

[FroMage]

Transactions are working with CDI interceptors and entities are not beans, so they can't be intercepted. Is there any reason why you can't do your transactions in the static methods?

[neon-dev]

No, but it would be weird to delete an entity via Entity.deleteTransactionalById(entity.getId()) or Entity.deleteTransactional(entity). It's also not always very elegant to define intermediate transactional methods in the calling class. Do you have a suggestion how to refactor this sample code without creating an annotated helper method in SomeClass or using QuarkusTransaction?

class SomeClass {

  void doStuff() {
    Entity entity = getEntity();
    if (tryToDoSomethingWith(entity)) {
      entity.delete();
    }
  }
}

[FroMage]

Yes, OK. But usually all entity modifications are done by a method annotated with @Transactional, so where are you calling these entity methods?

[neon-dev]

Here's one of my real world use cases:

@Scheduled(every = "2s", concurrentExecution = Scheduled.ConcurrentExecution.SKIP)
void maintainTriggers() {
  ExportTrigger.<ExportTrigger>listAll().forEach(trigger -> {
    PricelistConfig config = PricelistConfig.findById(trigger.customerName);
    if (config == null) {
      trigger.delete();
    } else if (config.getChannel() == Config.exporterChannel) {
      try {
        JobKey jobKey = new JobKey(trigger.customerName);
        JobDataMap jobDataMap = new JobDataMap();
        jobDataMap.put("customerName", trigger.customerName);
        jobDataMap.put("trigger", trigger);
        scheduler.addJob(JobBuilder.newJob(ExportJob.class).withIdentity(jobKey).storeDurably().usingJobData(jobDataMap).build(), true);
        scheduler.triggerJob(jobKey);
      } catch (SchedulerException e) {
        throw new RuntimeException(e);
      } finally {
        trigger.delete();
      }
    }
  });
}

trigger.delete() is the only method that needs to be transactional here. If I annotate the whole method or consumer with @Transactional, the lastChange field of config gets updated, which I don't want.

[FroMage]

Ah, I see. You can probably do it this way:

@Scheduled(every = "2s", concurrentExecution = Scheduled.ConcurrentExecution.SKIP)
void maintainTriggers() {
  ExportTrigger.<ExportTrigger>listAll().forEach(trigger -> {
    PricelistConfig config = PricelistConfig.findById(trigger.customerName);
    if (config == null) {
      delete(trigger);
    } else if (config.getChannel() == Config.exporterChannel) {
      try {
        JobKey jobKey = new JobKey(trigger.customerName);
        JobDataMap jobDataMap = new JobDataMap();
        jobDataMap.put("customerName", trigger.customerName);
        jobDataMap.put("trigger", trigger);
        scheduler.addJob(JobBuilder.newJob(ExportJob.class).withIdentity(jobKey).storeDurably().usingJobData(jobDataMap).build(), true);
        scheduler.triggerJob(jobKey);
      } catch (SchedulerException e) {
        throw new RuntimeException(e);
      } finally {
        delete(trigger);
      }
    }
  });
}

@Transactional
void delete(ExportTrigger trigger){
 trigger.delete();
}

I guess we could add support for this too:

@Scheduled(every = "2s", concurrentExecution = Scheduled.ConcurrentExecution.SKIP)
void maintainTriggers() {
  ExportTrigger.<ExportTrigger>listAll().forEach(trigger -> {
    PricelistConfig config = PricelistConfig.findById(trigger.customerName);
    if (config == null) {
        Panache.inTransaction(t -> trigger.delete());
    } else if (config.getChannel() == Config.exporterChannel) {
      try {
        JobKey jobKey = new JobKey(trigger.customerName);
        JobDataMap jobDataMap = new JobDataMap();
        jobDataMap.put("customerName", trigger.customerName);
        jobDataMap.put("trigger", trigger);
        scheduler.addJob(JobBuilder.newJob(ExportJob.class).withIdentity(jobKey).storeDurably().usingJobData(jobDataMap).build(), true);
        scheduler.triggerJob(jobKey);
      } catch (SchedulerException e) {
        throw new RuntimeException(e);
      } finally {
        Panache.inTransaction(t -> trigger.delete());
      }
    }
  });
}

WDYT?

[neon-dev]

Yes, this should work. I was just looking for a way to make the delete method transactional by default and ended up doing this in the ExportTrigger class, instead of annotating it with @Transactional:

@Override
public void delete() {
  QuarkusTransaction.joiningExisting().run(super::delete);
}

PS: Wouldn't Panache.inTransaction(t -> trigger.delete()) be the same as QuarkusTransaction.requiringNew().run(() -> trigger.delete())?

[FroMage]

Ah yeah, you're right. This is better.

If you don't mind, I'll rephrase your issue title to "Support CDI interceptor annotations on PanacheEntity instance methods?" and leave it open to see if other people want this or not, because so far you're the first person to ask.

[neon-dev]

Sure, thank you very much!