I've added Arquilian test in arquillian-tomee-webprofile-tests. It passes on my machine :) but I'm pretty sure it runs only with OpenJPA. How can I force it to run with eclipselink ?
https://github.com/apache/tomee/pull/70 2017-06-12 12:04 GMT+03:00 Romain Manni-Bucau <[email protected]>: > 2017-06-12 11:02 GMT+02:00 Svetlin Zarev <[email protected] > >: > > > OK, I'll update my PR. > > > > I want to write a test as well. In which project > > should I add it, so it's executed with both > > OpenJPA & Eclipse link ? > > > > > you have a few options here: > > 1. openejb-core > 2. add an example in examples/ (we use them as itest sometimes) > 3. arquillian/*tests > > The easiest is the example option probably but take the approach making you > comfortable. Only constraints are 1. don't break the whole build ;), 2. > ensure it is covered :) > > > > > > 2017-06-12 11:43 GMT+03:00 Romain Manni-Bucau <[email protected]>: > > > > > 2017-06-12 10:42 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail. > > com > > > >: > > > > > > > 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. > > > > > > > > > > Hmm, this is true...and if openejb removes SystemInstance too so guess > it > > > is better to decrease the reflection. Also don't forget the test would > > > break if it changes > > > and we would fix it ;). > > > > > > Side note: we are committer on this geronimo component too so see it as > > > part of tomee in term of risk. > > > > > > > > > > > > > > But I can modify my PR to use txn if you insist. > > > > > > > > 2017-06-12 11:36 GMT+03:00 Romain Manni-Bucau <[email protected] > >: > > > > > > > > > 2017-06-12 10:29 GMT+02:00 Svetlin Zarev > > <svetlin.angelov.zarev@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 registerInterposedSynchronizat > ion > > > > 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 < > > [email protected] > > > >: > > > > > > > > > > > > > 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 > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > > >
