Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Allow Propagation.NOT_SUPPORTED with @TransactionalEventListener #31907

Closed
xak2000 opened this issue Dec 25, 2023 · 2 comments
Closed

Allow Propagation.NOT_SUPPORTED with @TransactionalEventListener #31907

xak2000 opened this issue Dec 25, 2023 · 2 comments
Assignees
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement
Milestone

Comments

@xak2000
Copy link
Contributor

xak2000 commented Dec 25, 2023

After #30679 and #31414 this doesn't work anymore:

@TransactionalEventListener(fallbackExecution = true)
@Transactional(propagation = Propagation.NOT_SUPPORTED)
public void onUserDeleted(UserDeletedEvent userDeletedEvent) {
	someOtherService.anyTransactionalOrNonTransactionalMethod();
}

I think, NOT_SUPPORTED should be allowed here, as any other Propagation that implies suspending of the current transaction (that is in an undefined state, as @odrotbohm mentioned).

The whole purpose of restriction it to support only REQUIRES_NEW was to catch invalid usages of @TransactionalEventListener, right?

But NOT_SUPPORTED is also totally valid usage, as the method will never be executed in a current transaction (that was active at the moment of calling the listener). Current transaction will be suspended in exactly the same way as it is supsended with REQUIRES_NEW.

Probably, Propagation.NEVER is also valid here, as it will never execute the method if a transaction exists. But this has much less practical use: why do you need Propagation.NEVER on a @TransactionalEventListener in the first place? The transaction will always be active at the moment of calling such listener, except when fallbackExecution = true and listener was actually called on an event, published from non-transactional method. So, while I doubt it will be practial case, it is still valid in my opinion and should not prevent Application context to start.

Anyway, most importantly Propagation.NOT_SUPPORTED is totally valid and perfectly practical usage of @TransactionalEventListener. I would even argue that this is the best combination as Propagation.NOT_SUPPORTED serves as a failsafe way to prevent execution of the listener in an old transaction (that is in a "bad" state at the moment of execution) and the decision to start a new transaction or not for the code inside of the listener ifself is usually out of the scope of the listner and more in the scope of the business-method, that the listener calls.

@TransactionalEventListener + @Transactional(propagation = Propagation.NOT_SUPPORTED) means that someOtherService.anyTransactionalOrNonTransactionalMethod() will be executed correctly if it is annotated with @Transactional (with any propagation ) or if it is not annotated at all.

P.S. Also, there is phase attribute of TransactionalEventListener and with @TransactionalEventListener(phase = TransactionPhase.BEFORE_COMMIT) some other transaction Propagations will be valid too. E.g. Propagation.MANDATORY is semantically correct for such listener, isn't it? Probably, it doesn't have a practical value, as you can just remove @Transactional annotation in this case, but it is still valid. So, I think RestrictedTransactionalEventListenerFactory is too restrictive. Especially considering the fact that there is no easy opt-out if needed.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Dec 25, 2023
@jhoeller jhoeller added in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Dec 26, 2023
@jhoeller jhoeller self-assigned this Dec 26, 2023
@jhoeller jhoeller added this to the 6.1.3 milestone Dec 26, 2023
@jhoeller
Copy link
Contributor

I agree about NOT_SUPPORTED but I'd prefer to leave it at that then. It's a fine line to walk between rejecting invalid/pointless configuration upfront as opposed to tolerating it, and since @TransactionalEventListener implies subtle semantics that are not very obvious already, we'll rather try to provide as much guidance as possible here.

@xak2000
Copy link
Contributor Author

xak2000 commented Dec 26, 2023

I partially agree. Maybe other combinations are really invalid/pointless, or maybe we just not thought about something that is really valid and usable. This is the risk of restricting something that previously was possible.

But anyway, this task was primarily about NOT_SUPPORTED, so thank you for implementing this! It is really a blocker right now for our production code to be updated to use Spring Framework 6.1. I'm looking forward to the release. 👍

P.S. Found an error in the javadoc. Added a comment to the commit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: data Issues in data modules (jdbc, orm, oxm, tx) type: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

3 participants