Looks like we are all ok. I'll proceed and merge. Thanks Le 19 juil. 2017 00:56, "David Blevins" <david.blev...@gmail.com> a écrit :
I also say +1. I’d love to see the layers thinned out of the Arquillian test, but this could be done in another PR. Sveltin, if you’re up for it I can probably use this as an excuse to pull some Arquillian devs over for some fun hacking :) -- David Blevins http://twitter.com/dblevins http://www.tomitribe.com > On Jul 14, 2017, at 5:14 AM, Romain Manni-Bucau <rmannibu...@gmail.com> wrote: > > looks good to apply to me > > > 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-07-14 12:55 GMT+02:00 Svetlin Zarev <svetlin.angelov.za...@gmail.com >: > >> Hi, >> >> Do you have any comments ? Do you thing that something is missing or maybe >> that something additional is needed ? >> >> Kind regards, >> Svetlin >> >> 2017-07-11 15:07 GMT+03:00 Romain Manni-Bucau <rmannibu...@gmail.com>: >> >>> 2017-07-11 12:38 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail. >> com >>>> : >>> >>>> I've added several JUnit test cases in openejb-core that should verify >>>> IvmContext.list() behaviour, yet I'll feel safer if we keep the >>> arquillian >>>> test as well. >>>> >>> >>> +1, both are needed >>> >>> >>>> >>>> 2017-07-11 10:10 GMT+03:00 Romain Manni-Bucau <rmannibu...@gmail.com>: >>>> >>>>> side note: embedded (not tomcat based) testing is needed to ensure we >>>> don't >>>>> break but doesn't fully test the ivmcontext code because the >> federation >>>> is >>>>> different so guess starting with tomcat is not that bad. >>>>> >>>>> >>>>> 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-07-11 8:28 GMT+02:00 Svetlin Zarev <svetlin.angelov.zarev@gmail. >>> com >>>>> : >>>>> >>>>>> Hi David, >>>>>> >>>>>> Thank you for sparing some time to look into my PR ! >>>>>> >>>>>>> I’m not sure I can see how the test actually works >>>>>> >>>>>> The issue is that IvmContext.list() returns objects that are not >>> really >>>>>> bound into the listed context. For instance if you run the MCVE >>>> attached >>>>> to >>>>>> the jira ticket you'll see that it returns [1]. There you can see >>> that >>>>>> TestEJB is bound in "java:" (and even appears several times!) or >> that >>>>>> "java:global" is bound in "java:module". But if you try to look up >>>> those >>>>>> entries, the lookup fails with a NameNotFoundException, because all >>>> these >>>>>> references are not really bound there. So the test recursively >> lists >>>> all >>>>>> contexts in the JNDI tree and tries to lookup up every name-class >>> pair >>>>> that >>>>>> is returned. If the lookup fails, it means that list() has returned >>>>>> something that is not really there. You can compare [1] and [2] >>> 9after >>>>> the >>>>>> fix) to see the difference in list()'s behaviour >>>>>> >>>>>>> Is there a test for listBindings? It’s mentioned as also broken, >>>> but I >>>>>> didn’t see a test for it. >>>>>> >>>>>> IvmContext.listBindings() and IvmContext.list() use the very same >>>>>> IvmContext.MyNamingEnumeration abstract class and share the very >> same >>>>> logic >>>>>> to traverse the naming tree. I didn't write test for it because >> they >>>>> share >>>>>> the same code, but I can easily modify it to run aginst both >> methods. >>>>>> >>>>>>> What is the PrintWriter used for? It seems it could be useful to >>>>> assert >>>>>> it prints what we expect. Not sure if that is there and I am >> missing >>>> it. >>>>>> >>>>>> I thought it would be helpful to be able to see what went wrong if >>> the >>>>> test >>>>>> fails. The IvmContextTest class collects the output from the >>> servlet's >>>>>> output stream (the print writer) and if the test fails it prints it >>> in >>>>> the >>>>>> console. >>>>>> >>>>>>> There is an IvmContextTest, could we put the test there? >>>>>> >>>>>> That was my initial idea, but AppComposer's naming tree is very >>>> different >>>>>> that tomee's. For instance it does not have the "app", "global", >> etc >>>> top >>>>>> level contexts and my fix has special code for such top-level >>>> contexts. I >>>>>> also was not bale to bind any env-entries to my EJB (I'm not really >>>>>> familiar with how to write a proper appcomposer test, so I guess I >>>> didn't >>>>>> do something that I should have.). The env-entries are needed just >> to >>>>>> create a couple of branches to the tree in order to test if >>>>>> MyNamingEnumeration.isMyChild() works correctly >>>>>> >>>>>>> so we could potentially skip the plumbing of the >>> test->servlet->ejb. >>>>>> >>>>>> I'll look into it. I also have a few ideas for additioanl tests. >>>>>> >>>>>> [1] https://gist.github.com/SvetlinZarev/ >>>> 6b9377fe05b7887d681491c3e9e538 >>>>> 21 >>>>>> [2] https://gist.github.com/SvetlinZarev/ >>>> db3b59404198cd494b45b23db7129e >>>>> dd >>>>>> >>>>>> Bets regards, >>>>>> Svetlin >>>>>> >>>>>> 2017-07-11 2:28 GMT+03:00 David Blevins <david.blev...@gmail.com>: >>>>>> >>>>>>> Hi Svetlin! >>>>>>> >>>>>>> This is an awesome catch. Also, my apologies for the time you >> had >>> to >>>>>>> spend in that code. It literally hasn’t changed much since >>> 1999/2000 >>>>> and >>>>>>> it shows. :) >>>>>>> >>>>>>> Looking at the PR I’m not sure I can see how the test actually >>> works. >>>>>>> Here’s what I can follow, any gaps you can fill in are excellent: >>>>>>> >>>>>>> The call chain as I can see it: >>>>>>> >>>>>>> - IvmContextTest.testListContextTree >>>>>>> - IvmContextTest.validateTest("testListContextTree”) >>>>>>> [network call] >>>>>>> - IvmContextServlet.doGet # invokes itself via reflection, >>>> returns >>>>>>> “true” if no exception >>>>>>> [reflection call] >>>>>>> - IvmContextServlet.testListContextTree >>>>>>> - NamingBean.verifyListContext # throws exception if >> listContext >>>>>> returns >>>>>>> false >>>>>>> - NamingBean.listContext >>>>>>> >>>>>>> Looks like the essence of the test is in NamingBean.listContext. >>>>> Inside >>>>>>> it looks like the heart of it is that if we can’t lookup an item >>>>> listed, >>>>>> we >>>>>>> return false. >>>>>>> >>>>>>> Not sure if I got it perfectly, so definitely say so :) >>>>>>> >>>>>>> Couple questions: >>>>>>> >>>>>>> - Is there a test for listBindings? It’s mentioned as also >>> broken, >>>>> but >>>>>>> I didn’t see a test for it. >>>>>>> >>>>>>> - What is the PrintWriter used for? It seems it could be >> useful >>> to >>>>>>> assert it prints what we expect. Not sure if that is there and I >>> am >>>>>>> missing it. >>>>>>> >>>>>>> - There is an IvmContextTest, could we put the test there? >>>>>>> https://github.com/apache/tomee/blob/master/container/ >>>>>>> openejb-core/src/test/java/org/apache/openejb/ivm/naming/ >>>>>>> IvmContextTest.java >>>>>>> >>>>>>> On the last one I noted the patched code mentions Tomcat, so >> maybe >>> it >>>>>> does >>>>>>> have to be Arquillian based. If so, maybe we could still trim >> out >>>> some >>>>>> of >>>>>>> those layers. I think Arquillian has the ability to execute >> remote >>>>> code, >>>>>>> so we could potentially skip the plumbing of the >>> test->servlet->ejb. >>>>>>> >>>>>>> Regardless, looking IvmContext always makes my brain hurt so >>>> incredibly >>>>>>> well done surviving it. You’re clearly quite sharp. >>>>>>> >>>>>>> >>>>>>> -David >>>>>>> >>>>>>> >>>>>>>> On Jul 10, 2017, at 4:08 AM, Svetlin Zarev >>>>>> <svetlin.angelov.zarev@gmail. >>>>>>> com> wrote: >>>>>>>> >>>>>>>> Hi, >>>>>>>> >>>>>>>> I'd like to propose the following fix: [1]. It fixes >>>>>>>> IvmContext.list()/listBindings(). There are several issues >> that >>>> are >>>>>>> being >>>>>>>> addressed: >>>>>>>> >>>>>>>> * MyNamingEnumeration.gatherNodes() adds the wrong federated >>>> context >>>>>>>> entries in the result set. It should add the nodes belonging to >>> the >>>>>>>> "initiallyRequestedNode", otherwise in some cases we are adding >>>> nodes >>>>>>> from >>>>>>>> two different parents (in other words we are mixing two >> different >>>>>>>> sub-contexts), which is incorrect. >>>>>>>> >>>>>>>> * MyNamingEnumeration.isMyChild() considers entries that are >> NOT >>>>>>> children >>>>>>>> to the "parent" tree as such - the original code was using >>>>>> "parentTree", >>>>>>> to >>>>>>>> check if a given node is a child to another one, which is >> wrong. >>>> The >>>>>>>> "parentTree" relationship indicates only the physical layout of >>> the >>>>>> tree, >>>>>>>> and NOT the relationship between the sub-contexts and their >>>> entries. >>>>>>> Hence >>>>>>>> it considers for instance "java:global" to be a child of >>>>> "java:module" >>>>>>>> which is obviously incorrect. The relationship between a >> context >>>> and >>>>>> the >>>>>>>> bound entries is denoted by the "parent" node. So when listing >> a >>>>>> context >>>>>>>> isMyChild should rely only on the "parent" node. There is one >>>>>> exception - >>>>>>>> the top level contexts (app, global, etc) which do not have a >>>> parent. >>>>>>>> >>>>>>>> * Wrong parentNode is passed as argument to gatherNodes in case >>> we >>>>> are >>>>>>>> listing the context for any "IvmContext != this. When we call >>>>>>>> IvmContext.list(), it looks up the relevant IvmContext and >> tries >>> to >>>>>>> build a >>>>>>>> NamingEnumeration for its sub-tree. So far so good, but the >>> looked >>>> up >>>>>>>> context might be different than the context on which we called >>>>> list(). >>>>>> If >>>>>>>> that's the case, the wrong NameNode is passed as "parent" to >>>>>>> gatherNodes() >>>>>>>> and as a result some nodes are not listed. >>>>>>>> >>>>>>>> PS: This issue is relevant for tomee 1.7.x as well. i noticed >> it >>>> does >>>>>> not >>>>>>>> correctly list the naming tree as well. It also does not list >> the >>>>>>> federated >>>>>>>> contexts which was implemented in [3]. >>>>>>>> >>>>>>>> [1] https://github.com/apache/tomee/pull/88 >>>>>>>> [2] https://issues.apache.org/jira/browse/TOMEE-2087 >>>>>>>> [3] https://github.com/apache/tomee/pull/81 >>>>>>>> >>>>>>>> Best regards, >>>>>>>> Svetlin >>>>>>> >>>>>>> >>>>>> >>>>> >>>> >>> >>
