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/6b9377fe05b7887d681491c3e9e53821 [2] https://gist.github.com/SvetlinZarev/db3b59404198cd494b45b23db7129edd 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 > >
