[TCK] JTA EntityManager issue
Hi community, I'm currently working on a TCK failure. com.sun.ts.tests.jpa.core.enums.Client#setgetFlushModeEntityManagerTest_from_stateless3 This particular method https://github.com/eclipse-ee4j/jakartaee-tck/blob/master/src/com/sun/ts/tests/jpa/core/enums/Client.java#L955 In essence, it gets an entity manager from the container. It's a Stateless session bean, with Bean managed transactions. There is no transaction started. Then, it calls setFlushMode and right after getFlushMode to see if the change got applied. The issue with our implementation (or not) is that we do https://github.com/apache/tomee/blob/master/container/openejb-core/src/main/java/org/apache/openejb/persistence/JtaEntityManager.java#L258 So we get the current entity manager and then we delegate the call. The issue is that we call our JtaEntityManagerRegistry to get the current EntityManager if any or create a new one. https://github.com/apache/tomee/blob/master/container/openejb-core/src/main/java/org/apache/openejb/persistence/JtaEntityManagerRegistry.java#L85 As we are not in a transaction and we are not using an Extended Persistence Context, we create a new EntityManager every time, which means, the one used for set is not the one used for the get :-( The spec does not seem to cover this case https://jakarta.ee/specifications/persistence/3.0/jakarta-persistence-spec-3.0.html#container-responsibilities I quickly checked and both glassfish and wildfy have a new "invocation scoped" entity manager which is not part of the spec. One way would be to start a transaction at the beginning of the method so we have the guarantee to use the same underlying entity manager. -- Jean-Louis Monteiro http://twitter.com/jlouismonteiro http://www.tomitribe.com
Re: JSON Pointer/Patch issue with /-
Le mar. 1 déc. 2020 à 14:40, Jonathan Gallimore < jonathan.gallim...@gmail.com> a écrit : > I'll address a few points inline below, but at a high level, what are we > looking to achieve from a spec/tck challenge? > > I can see a case for some clarification and updates to the Javadoc. > > The assertions that /- will return an error (as that references an index to > append to after the *end* of an array - i.e. array.length) are tested in > the TCK, and other implementations must be passing that TCK. It's hard to > see a spec change happening, as there is no spec document beyond the RFCs > that I can find. A TCK change that would enable Johnzon to pass, and > require other currently passing implementations to make a change seems > unlikely. Jakarta EE 8's TCK has been around a while and has > implementations that pass. The Jakarta EE 9 TCK is basically "done" and is > essentially the same as EE8, bar the namespace change. I guess adding a > test exclude is possible, but serves to make this more vague and vendor > dependent (and non-portable) which feels like it defeats the purpose - > surely having it better defined and tested is the way to go. > Well, here the fact is that it does not impact other vendors since it is a johnzon vendor specific feature we put in a shadow of the (javax/jakarta) spec handling in a custom fashion an error case. Typically the case where we can exclude the TCK since it is irrelevant for our impl but I understand also it is not perfect. > > I appreciate that this introduces a backwards incompatible change, and that > there may be other consumers of the library that would have an issue if > this just changed. This seems like a fairly straightforward case that could > be easily and quickly solved with a feature switch, and passing the TCK is > a worthwhile goal, both for Johnzon and TomEE. I suspect the TCK challenge > will take a bit of time, and we'll likely end up back at the feature switch > anyway. > Issue is we dont have a Json.createPointerFactory(mapWithToggle) so it is a global flag which means it breaks some deployments anyway - at least at tomee level - when > 1 app is deployed (or >= 1 app + 1 extension). > > On Fri, Nov 27, 2020 at 10:44 AM Romain Manni-Bucau > > wrote: > > > Hi JL, > > > > As discussed together - but sharing for others - we must take into > account > > some points: > > > > 1. reading both spec, JSON-Patch enables to handle /- as your first did > (ie > > consider it is last element). JSON-Patch uses JSON-Pointer but nowhere it > > is written it behaves as JSON-Pointer in all cases and it is typically > > "integration" definition which can extend an underlying spec (otherwise > > most of EE wouldn't be right? ;)) > > > > I think the idea is that it references a non-existent element, *after* the > last element in an array. So if you have an array [0, 1, 2, 3, 4], then > "/-" would reference element _5_ (assuming you start your numbering at 0), > and not the last element in the array (index 4). > This is the jsonpointer spec right, but JSONPatch never requires to not handle the case as we do, it is just not written (and why we used it also). Issue on jsonpointer side being we can't have another character which means "last element". > > > > 2. On johnzon point of view we can't break this feature which was > requested > > by user and transitive users (ie user of products built with johnzon) > > without at least a clear migration path so if we want to break we should > do > > a 1.3 (dont think we need a 1.2 maintenance branch, we can do it lazily), > > document how to migrate from current behavior to new one (i'll detail it > > after) and communicate on it on our website properly (index.html ref and > > dedicated page I guess with the release annoucement). Alternative is to > > challenge the TCK, it is a failure case so it is typically the kind of > case > > we can plug custom/vendor behavior (we do in other parts of the JSON-B > spec > > for ex). Overall idea is to not let users on the road because some TCK > > exist (functional and users over procedural work). > > > > I'd be interested in the history, it helps to be mindful of it when making > changes. > Goal is to be able to work on the last element, there is nothing in specs about this one but it is very common to need that (see it as "length" operator). Indeed we can enrich jsonlogic module to cover that case but most users just bring jsonp+jsonb and not johnzon-jsonlogic. > > > > > > On strict TCK side, we can also do a johnzon-tck module where we wrap the > > provider to handle that case and pass the TCK, this is purely technical > to > > be compliant but would avoid to break anything. > > Now if we really want to be strict in our implementation we must still > > enable this last element case. One option not far from what we have is to > > use our json-logic module and add some jsonpatch operators. Combining > > multiple operators we can manage to fulfill this common patching need - > but
Re: JSON Pointer/Patch issue with /-
I'll address a few points inline below, but at a high level, what are we looking to achieve from a spec/tck challenge? I can see a case for some clarification and updates to the Javadoc. The assertions that /- will return an error (as that references an index to append to after the *end* of an array - i.e. array.length) are tested in the TCK, and other implementations must be passing that TCK. It's hard to see a spec change happening, as there is no spec document beyond the RFCs that I can find. A TCK change that would enable Johnzon to pass, and require other currently passing implementations to make a change seems unlikely. Jakarta EE 8's TCK has been around a while and has implementations that pass. The Jakarta EE 9 TCK is basically "done" and is essentially the same as EE8, bar the namespace change. I guess adding a test exclude is possible, but serves to make this more vague and vendor dependent (and non-portable) which feels like it defeats the purpose - surely having it better defined and tested is the way to go. I appreciate that this introduces a backwards incompatible change, and that there may be other consumers of the library that would have an issue if this just changed. This seems like a fairly straightforward case that could be easily and quickly solved with a feature switch, and passing the TCK is a worthwhile goal, both for Johnzon and TomEE. I suspect the TCK challenge will take a bit of time, and we'll likely end up back at the feature switch anyway. On Fri, Nov 27, 2020 at 10:44 AM Romain Manni-Bucau wrote: > Hi JL, > > As discussed together - but sharing for others - we must take into account > some points: > > 1. reading both spec, JSON-Patch enables to handle /- as your first did (ie > consider it is last element). JSON-Patch uses JSON-Pointer but nowhere it > is written it behaves as JSON-Pointer in all cases and it is typically > "integration" definition which can extend an underlying spec (otherwise > most of EE wouldn't be right? ;)) > I think the idea is that it references a non-existent element, *after* the last element in an array. So if you have an array [0, 1, 2, 3, 4], then "/-" would reference element _5_ (assuming you start your numbering at 0), and not the last element in the array (index 4). > 2. On johnzon point of view we can't break this feature which was requested > by user and transitive users (ie user of products built with johnzon) > without at least a clear migration path so if we want to break we should do > a 1.3 (dont think we need a 1.2 maintenance branch, we can do it lazily), > document how to migrate from current behavior to new one (i'll detail it > after) and communicate on it on our website properly (index.html ref and > dedicated page I guess with the release annoucement). Alternative is to > challenge the TCK, it is a failure case so it is typically the kind of case > we can plug custom/vendor behavior (we do in other parts of the JSON-B spec > for ex). Overall idea is to not let users on the road because some TCK > exist (functional and users over procedural work). > I'd be interested in the history, it helps to be mindful of it when making changes. > > On strict TCK side, we can also do a johnzon-tck module where we wrap the > provider to handle that case and pass the TCK, this is purely technical to > be compliant but would avoid to break anything. > Now if we really want to be strict in our implementation we must still > enable this last element case. One option not far from what we have is to > use our json-logic module and add some jsonpatch operators. Combining > multiple operators we can manage to fulfill this common patching need - but > we break the overall API + require a new module to be added to apps). > > Lastly I would note that JSON Pointer *enables* our impl: > > > Note that the use of the "-" character to index an array will always > >result in such an error condition because by definition it refers to >a nonexistent array element. Thus, applications of JSON Pointer need >to specify how that character is to be handled, if it is to be >useful. > > > > For example, some applications might stop pointer processing upon an > >error, while others may attempt to recover from missing values by >inserting default ones. > > > Literally means "this is a case we consider as an error but your > application can recover from it" and we do ;). > Sort of. "applications of JSON Pointer need to specify how that character is to be handled". What's the definition of "application of JSON pointer"? In the case of TomEE, I'd suggest the "application" is Jakarta EE, which has specified that an error should be thrown. In a standalone case, is the application whatever is consuming Johnzon, or Johnzon itself? > Since it is an error case I would start by challenging the TCK to make it > vendor dependent and exclude it from the passing list for now. > If really blocking we can go with plan B and try to have a migration
Re: CI job for tomee-7.0.x
Hi, forgot to send an eMail to the list. The related PRs are available: https://github.com/apache/tomee/pull/720https://github.com/apache/tomee/pull/722 BestRichard Z Am Freitag, den 27.11.2020, 13:09 + schrieb Zowalla, Richard: > Will also check the third one :) - will create a related JIRA. > > Best, > Richard > > > > > Am Freitag, den 27.11.2020, 08:24 + schrieb Zowalla, Richard: > > Hi Cesar, > > i will give > > > The second failing test trowed a: [Fatal Error] :8:23: Invalid > > > byte 2 of2-byte UTF-8 sequence. error. > > > https://ci-builds.apache.org/job/Tomee/job/tomee-7.0.x/org.superbiz$moviefun-functional-test/3/testReport/junit/org.superbiz.moviefun/MoviesArquillianHtmlUnitTest/org_superbiz_moviefun_MoviesArquillianHtmlUnitTest/ > > > > a try and create a related JIRA. > > > > Best, > > Richard > > Am Donnerstag, den 26.11.2020, 20:35 -0600 schrieb Cesar Hernandez: > > > Hi All, > > > Today I set up a CI job for tomee 7.0.x.I initially set up Java 7 > > > but I got the "Unsupported major.minor version52.0" error so the > > > job currently uses Java 8 and maven 3.3.9.The current branch > > > status is here > > > https://ci-builds.apache.org/job/Tomee/job/tomee-7.0.x/3/ > > > > > > One of the failing test is because one of the examples requires > > > the usageof 7.0.8-SNAPSHOT:zip:plus. > > > https://github.com/apache/tomee/blob/tomee-7.0.x/examples/connector-ear/connector-sample-functional-tests/src/test/java/org/superbiz/moviefun/DeployInWebAppsDirectoryTest.java#L108 > > > I'll open a JIRA and a Patch for that test. > > > The second failing test trowed a: [Fatal Error] :8:23: Invalid > > > byte 2 of2-byte UTF-8 sequence. error. > > > https://ci-builds.apache.org/job/Tomee/job/tomee-7.0.x/org.superbiz$moviefun-functional-test/3/testReport/junit/org.superbiz.moviefun/MoviesArquillianHtmlUnitTest/org_superbiz_moviefun_MoviesArquillianHtmlUnitTest/ > > > If someone wants to take a swing on this, feel free to pick it > > > up. Rememberto create a JIRA and notify the mailing list to avoid > > > duplication of work. > > > The thrid failing test: javax.ws.rs.NotSupportedException: HTTP > > > 415Unsupported Media Type > > > https://ci-builds.apache.org/job/Tomee/job/tomee-7.0.x/org.superbiz$tomee-webprofile-embedded/3/testReport/junit/org.superbiz.movie/MovieServiceTest/addMovie/ > > > If someone wants to take a swing on this, feel free to pick it > > > up. Rememberto create a JIRA and notify the mailing list to avoid > > > duplication of work. smime.p7s Description: S/MIME cryptographic signature
