I didn't think of that, but still I prefer to do it via the proper interface, because javax.transaction.Transaction does not expose the interposed list. So it would be transaction-manager implementation dependent and it might stop working if geronimo changes its impl.
But I can modify my PR to use txn if you insist. 2017-06-12 11:36 GMT+03:00 Romain Manni-Bucau <rmannibu...@gmail.com>: > 2017-06-12 10:29 GMT+02:00 Svetlin Zarev <svetlin.angelov.za...@gmail.com > >: > > > > why not relying on the > > default? javax.transaction.Transaction.class.cast(txn). > > registerSynchronization(listener > > > > Because it registers the transaction in the "regular" synchronizations > > list. But > > if the synchronization is registered via the > > TransactionSynchronizationRegistry, > > the synchronization will be registered in the "interposed" > synchronization > > list. > > > > The synchronizations from the interposed list are always executed after > the > > synchronizations from the regular list. > > > > > Makes sense. Note you can just get registerInterposedSynchronization from > the txn directly, it would make less reflection and do exactly the same. > > > > > > OpenEJB's SessionSynchronizationCoordinator is registered into the > > interposed list, so if we want for @BeforeCompletion to work > > correctly with eclipselink , then either openejb's > > SessionSynchronizationCoordinator > > must be registered in the regular list, or eclipselink's synchronization > > must be registered in the interposed list. I think that the second > option > > is less invasive. > > > > Your patch (adapted with the previous reflection enhancement probably) > looks good, just miss a test to ensure it works (surely in openejb-core > with application composer) > > > > > > > > 2017-06-12 11:22 GMT+03:00 Romain Manni-Bucau <rmannibu...@gmail.com>: > > > > > Hi Svetlin, > > > > > > why not relying on the > > > default? javax.transaction.Transaction.class.cast(txn). > > > registerSynchronization(listener)? > > > This is what your code does except it goes through the registry to find > > the > > > current transaction instead of using the one already passed and bound > to > > > the entity manager - should lead to the same if the application doesnt > > > abuse of transactions. > > > > > > The reflection is here cause openejb-core depends on > > > openejb-jpa-integration so if you add openejb-core as a dependency it > > > wouldn't build. This is done cause jpa-integration is added to > > applications > > > for the ones providing the jpa provider instead of using the container > > one. > > > > > > Side note: would be good to ensure any PR has some test(s) when > possible > > > otherwise it is easy to break it before next release without even > > noticing > > > it. > > > > > > > > > > > > Romain Manni-Bucau > > > @rmannibucau <https://twitter.com/rmannibucau> | Blog > > > <https://blog-rmannibucau.rhcloud.com> | Old Blog > > > <http://rmannibucau.wordpress.com> | Github <https://github.com/ > > > rmannibucau> | > > > LinkedIn <https://www.linkedin.com/in/rmannibucau> | JavaEE Factory > > > <https://javaeefactory-rmannibucau.rhcloud.com> > > > > > > 2017-06-12 10:13 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail. > > com > > > >: > > > > > > > Hi Everyone, Romain, > > > > > > > > As JIRA is down, I'm writing to the mailing list. > > > > > > > > Recently I reported TOMEE-2057 -> modification to > > > > entities done in the @BeforeCompletion callback were > > > > not getting persisted. The root cause was that > > > > eclipselink's synchronization was executed before > > > > openejb's one. The same case works as expected > > > > with OpenJPA. > > > > > > > > Hence I want to propose the following fix for the > > > > eclipselink's integration with tomee: [1] OpenJPA > > > > does the same, but it's implemented inside OpenJPA. > > > > > > > > [1] https://github.com/apache/tomee/pull/70 > > > > > > > > PS: Is there really need for reflection ? Why don't we add > > > > dependency to OpenEJB-Core and remove the reflection ? > > > > > > > > Best regards, > > > > Svetlin > > > > > > > > > >
