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.za...@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
