Clarify whether rollbackOn accepts Throwables and improve typesafety

[gavinking]

It's unclear from the spec and Javadoc whether an "exception" specified by rollbackOn or dontRollbackOn must descend from Exception, or whether it may be any sort of Throwable. This problem is made far worse because these annotations members are not type safe, and declared using raw types, allowing the user to write nonsensical things like @Transactional(rollbackOn=Object.class).

It should be clarified whether stuff like @Transactional(rollbackOn=Error.class) is allowed, and the annotation members should be fixed to have a correct upper bound on the Class type.

[gavinking]

Note that the Javadoc does say:

@return Class[] of Exceptions

But I have no clue how to interpret this cryptic statement. Is this supposed to imply that the Classes should extend Exception, of is that just a random upper-case letter. I note the lack of monospace font here.

[gavinking]

Actually, thinking through how this must be implemented, I suppose that it probably is only subtypes of Exception allowed here. Otherwise you would need to catch Trhowable in an interceptor, which sounds like a bad idea. If that's correct, it's even more important to make that requirement explicit.

[gavinking]

Otherwise you would need to catch Throwable in an interceptor, which sounds like a bad idea.

Well, I checked, and apparently this is exactly what Quarkus does. So apparently any sort of Throwable is allowed here.

If that's also true for other JTA implementations, then this passage of the spec is highly ambiguous:

By default checked exceptions do not result in the transactional interceptor marking the transaction for rollback and instances of RuntimeException and its subclasses do.

Subtypes of Error are not checked exceptions, but nor are they instances of RuntimeException.

I found this issue: #191, which proposes to standardize the behavior I just observed in Quarkus.

[gavinking]

Oh, wait, wait, wait. Perhaps I've been a bit dense here. Perhaps rollbackOn and dontRollbackOn are intended to accept marker interfaces as well as exception classes. If that's the case then:

• It's correct that there's no upper bound on the class type in the annotation, but

• the language of the spec is even more misleading/ambiguous than I thought, since it talks only about classes, and has no trace of an indication that an interface is allowed:

  When a class is specified for either of these elements, the designated behavior applies to subclasses of that class as well.

[tomjenkinson]

We have talked about this issue and I was very close to being convinced that it was appropriate that we could narrow this to Throwable classes and maintain binary backwards compatibility at least however, our conversation developed with idea of what if someone happens to have used a non-Exception inherited Interface in the dontRollbackOn attributed. Given it is not defined this is not allowed we have to consider how it would be possible to proceed with that approach, but if we do not proceed with that approach. We agreed that the Javadoc should therefore be updated to include a note that if an interface is provided, implementations must support that and the Javadoc should also make it clear that the behaviour at runtime when an subclass of error is throw is undefined.

[tomjenkinson]

A pull request with the wording change is a good way to go here due to interfaces possibly being provided and already working with current implementations so mandating in the spec as we would do with the Javadoc seems feasible