You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomee.apache.org by Svetlin Zarev <sv...@gmail.com> on 2017/07/10 11:08:16 UTC
[PR] TOMEE-2087 IvmContext.list() does not correctly list the context content
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
Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the
context content
Posted by Svetlin Zarev <sv...@gmail.com>.
Thanks!
> 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 :)
I'm not that knowledgeable in Arquillian, but I'll help with whatever I can
:)
> 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
I removed the servlet, maybe I can remove the bean as well. It's not needed
as long as the test is executed against the IvmContext.
Kind regards,
Svetlin
2017-07-19 10:53 GMT+03:00 Jean-Louis Monteiro <jl...@tomitribe.com>:
> Looks like we are all ok. I'll proceed and merge.
> Thanks
>
> Le 19 juil. 2017 00:56, "David Blevins" <da...@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 <rm...@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.zarev@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 <rm...@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 <rmannibucau@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 <da...@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
> >>>>>>>
> >>>>>>>
> >>>>>>
> >>>>>
> >>>>
> >>>
> >>
>
Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the
context content
Posted by Jean-Louis Monteiro <jl...@tomitribe.com>.
Looks like we are all ok. I'll proceed and merge.
Thanks
Le 19 juil. 2017 00:56, "David Blevins" <da...@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 <rm...@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.zarev@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 <rm...@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 <rm...@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 <da...@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
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the
context content
Posted by David Blevins <da...@gmail.com>.
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 <rm...@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 <sv...@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 <rm...@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 <rm...@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 <da...@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
>>>>>>>
>>>>>>>
>>>>>>
>>>>>
>>>>
>>>
>>
Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the
context content
Posted by Romain Manni-Bucau <rm...@gmail.com>.
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 <sv...@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 <rm...@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 <rm...@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 <da...@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
> > > > > >
> > > > > >
> > > > >
> > > >
> > >
> >
>
Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the
context content
Posted by Svetlin Zarev <sv...@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 <rm...@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 <rm...@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 <da...@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
> > > > >
> > > > >
> > > >
> > >
> >
>
Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the
context content
Posted by Romain Manni-Bucau <rm...@gmail.com>.
2017-07-11 12:38 GMT+02:00 Svetlin Zarev <sv...@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 <rm...@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 <da...@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
> > > >
> > > >
> > >
> >
>
Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the
context content
Posted by Svetlin Zarev <sv...@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.
2017-07-11 10:10 GMT+03:00 Romain Manni-Bucau <rm...@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 <sv...@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 <da...@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
> > >
> > >
> >
>
Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the
context content
Posted by Romain Manni-Bucau <rm...@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 <sv...@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/6b9377fe05b7887d681491c3e9e53821
> [2] https://gist.github.com/SvetlinZarev/db3b59404198cd494b45b23db7129edd
>
> Bets regards,
> Svetlin
>
> 2017-07-11 2:28 GMT+03:00 David Blevins <da...@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
> >
> >
>
Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the
context content
Posted by Svetlin Zarev <sv...@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/6b9377fe05b7887d681491c3e9e53821
[2] https://gist.github.com/SvetlinZarev/db3b59404198cd494b45b23db7129edd
Bets regards,
Svetlin
2017-07-11 2:28 GMT+03:00 David Blevins <da...@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
>
>
Re: [PR] TOMEE-2087 IvmContext.list() does not correctly list the
context content
Posted by David Blevins <da...@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 <sv...@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