hibernate-orm icon indicating copy to clipboard operation
hibernate-orm copied to clipboard

HHH-17743 Allow updates outside transaction

Open subijayb opened this issue 1 year ago • 1 comments

Please check https://discourse.hibernate.org/t/jakarta-persistence-transactionrequiredexception-on-query-modifying-and-transactional-methods/9042

https://hibernate.atlassian.net/browse/HHH-17743

subijayb avatar Feb 20 '24 11:02 subijayb

Please also add a test that reproduces the problem to the test class org.hibernate.orm.test.flush.NonTransactionalDataAccessTest.

beikov avatar Feb 20 '24 16:02 beikov

Thanks for your pull request!

This pull request appears to follow the contribution rules.

› This message was automatically generated.

Hi @beikov , could you please review the test case ? I don't know why I cannot assign a reviewer

subijayb avatar Feb 22 '24 04:02 subijayb

Please rebase and squash to a single commit

beikov avatar Feb 22 '24 13:02 beikov

Hi @beikov, could you please check now and let me know the next step

subijayb avatar Feb 22 '24 17:02 subijayb

Hi @beikov , could you please review and let me know the next steps?

subijayb avatar Feb 26 '24 12:02 subijayb

Hi @beikov. This is a showstopper bug when working with the latest version of Spring Boot and JPA.

miles-po avatar Mar 14 '24 20:03 miles-po

Doing things outside a transation is never a show stopper ;)

Also, why is this targetting 6.4 and not main? Does this not happen on main?

sebersole avatar Mar 14 '24 20:03 sebersole

Hi @sebersole ,

This bug impacts all 6.x branch. I pushed it to 6.4 hoping that 6.4.5 will come out soon and we can use this. I will also merge it to main branch. Could you please review and merge this change?

subijayb avatar Mar 15 '24 07:03 subijayb

@sebersole Yes, my comment was intentionally provocative due to lack of response on what looks like a viable patch for three weeks. For reference, this shows up even when trying to execute queries from the EntityManager, even native queries. With or without @Transactional. The default deleteById works, but any other deleteBy variant hits this code path. em.getTransaction() doesn't work either because it's a managed em. It appears like our only option is to completely bypass the ORM and make queries on a raw DataSource. That option is unappealing considering a very simple patch already exists here upstream.

miles-po avatar Mar 15 '24 14:03 miles-po

Sorry this takes so long, but please understand that the ORM team has it's own priorities and is not that huge to respond to every request so fast. We're all trying our best, but if you need guaranteed fast response times, consider becoming a Red Hat customer and create support tickets, because such tasks have priority over everything else that we do.

beikov avatar Mar 15 '24 15:03 beikov

Understood. Thank you for your efforts.

miles-po avatar Mar 15 '24 15:03 miles-po

Thanks @beikov for merging. Could you please let know how to push it to main branch? I have to raise a new PR?

subijayb avatar Mar 15 '24 17:03 subijayb

2 PRs is fine. Or (future) send a PR against main and we will port it to maintained branches generally

sebersole avatar Mar 15 '24 17:03 sebersole

@beikov instead of removing getSession().prepareForQueryExecution(true)

should not be something like getSession().prepareForQueryExecution(false)

or

getSession().prepareForQueryExecution(
				requiresTxn( getQueryOptions().getLockOptions().findGreatestLockMode() ) )

dreab8 avatar Mar 16 '24 12:03 dreab8

I already merged it on main.

@dreab8 the change of this PR is fine IMO, because doExecuteUpdate() is only ever called from executeUpdate() which already does the following:

getSession().checkTransactionNeededForUpdateOperation( "Executing an update/delete query" );

beikov avatar Mar 18 '24 10:03 beikov