You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@tomee.apache.org by Prasad Kashyap <go...@gmail.com> on 2007/05/21 19:28:34 UTC
Re: Wrong interceptor chaining order
JIRA to track the bug -
https://issues.apache.org/jira/browse/OPENEJB-584
Cheers
Prasad
On 3/24/07, Prasad Kashyap <go...@gmail.com> wrote:
> https://issues.apache.org/jira/secure/attachment/12354131/Interceptor-v4.patch
>
> https://issues.apache.org/jira/browse/OPENEJB-201
>
> Cheers
> Prasad
>
>
>
> On 3/23/07, Prasad Kashyap <go...@gmail.com> wrote:
> > This still does not seem to work for method-level interceptors.
> >
> > I have 2 method-level interceptors; 1 defined using annotation and 1
> > specified in the DD.
> >
> > As per the fix we applied recently, I am able to verify that the
> > binding from the annotation is added to the list before the binding
> > from the DD.
> >
> > Yet, the interceptor from the DD gets invoked before the interceptor
> > from the annotation !
> >
> > Cheers
> > Prasad
> >
> > On 3/22/07, David Blevins <da...@visi.com> wrote:
> > >
> > > On Mar 22, 2007, at 6:44 PM, Prasad Kashyap wrote:
> > >
> > > > Thanx..
> > > >
> > > > https://issues.apache.org/jira/secure/attachment/12354016/
> > > > AnnotationDeployer.patch
> > > >
> > >
> > > Applied! Thank you very much!
> > >
> > > -David
> > >
> > > >
> > > > On 3/22/07, David Blevins <da...@visi.com> wrote:
> > > >> Your patch got line wrapped.
> > > >>
> > > >> Here, I've reopened this issue, go ahead and throw in in there:
> > > >>
> > > >> http://issues.apache.org/jira/browse/OPENEJB-87
> > > >>
> > > >> -David
> > > >>
> > > >> On Mar 22, 2007, at 1:21 PM, Prasad Kashyap wrote:
> > > >>
> > > >> > Index: container/openejb-core/src/main/java/org/apache/openejb/
> > > >> > config/AnnotationDeployer.java
> > > >> > ===================================================================
> > > >> > --- container/openejb-core/src/main/java/org/apache/openejb/config/
> > > >> > AnnotationDeployer.java (revision
> > > >> > 521254)
> > > >> > +++ container/openejb-core/src/main/java/org/apache/openejb/config/
> > > >> > AnnotationDeployer.java (working
> > > >> > copy)
> > > >> > @@ -63,6 +63,7 @@
> > > >> > import java.net.URL;
> > > >> > import java.util.ArrayList;
> > > >> > import java.util.Arrays;
> > > >> > +import java.util.Collection;
> > > >> > import java.util.List;
> > > >> > import java.util.Map;
> > > >> >
> > > >> > @@ -307,14 +308,15 @@
> > > >> >
> > > >> > Interceptors interceptors =
> > > >> > clazz.getAnnotation(Interceptors.class);
> > > >> > if (interceptors != null){
> > > >> > - EjbJar ejbJar = ejbModule.getEjbJar();
> > > >> > + EjbJar ejbJar = ejbModule.getEjbJar();
> > > >> > for (Class interceptor : interceptors.value
> > > >> ()) {
> > > >> > if
> > > >> > (ejbJar.getInterceptor(interceptor.getName()) == null){
> > > >> > ejbJar.addInterceptor(new
> > > >> > Interceptor(interceptor.getName()));
> > > >> > }
> > > >> > }
> > > >> >
> > > >> > - InterceptorBinding binding =
> > > >> > assemblyDescriptor.addInterceptorBinding(new InterceptorBinding());
> > > >> > + InterceptorBinding binding = new
> > > >> > InterceptorBinding();
> > > >> > + assemblyDescriptor.getInterceptorBinding().add
> > > >> > (0, binding);
> > > >> > binding.setEjbName(bean.getEjbName());
> > > >> >
> > > >> > for (Class interceptor : interceptors.value
> > > >> ()) {
> > > >> > @@ -332,7 +334,8 @@
> > > >> > }
> > > >> > }
> > > >> >
> > > >> > - InterceptorBinding binding =
> > > >> > assemblyDescriptor.addInterceptorBinding(new InterceptorBinding());
> > > >> > + InterceptorBinding binding = new
> > > >> > InterceptorBinding();
> > > >> > +
> > > >> > assemblyDescriptor.getInterceptorBinding().add(0, binding);
> > > >> > binding.setEjbName(bean.getEjbName());
> > > >> >
> > > >> > for (Class interceptor : interceptors.value
> > > >> > ()) {
> > > >> >
> > > >> >
> > > >> >
> > > >> > On 3/22/07, Prasad Kashyap <go...@gmail.com> wrote:
> > > >> >> On 3/22/07, David Blevins <da...@visi.com> wrote:
> > > >> >> >
> > > >> >> > On Mar 22, 2007, at 8:35 AM, Prasad Kashyap wrote:
> > > >> >> >
> > > >> >> > > Are you sure we can't just change the spec instead ?
> > > >> >> >
> > > >> >> > Maybe last year at this time :)
> > > >> >> >
> > > >> >> > > This is much complicated than the "simple" change you
> > > >> >> mentioned. It
> > > >> >> > > affects both lines 317 (class-level interceptor) and 335
> > > >> >> (method-level
> > > >> >> > > interceptor).
> > > >> >> > >
> > > >> >> > > InterceptorBinding binding =
> > > >> >> > > assemblyDescriptor.addInterceptorBinding(new
> > > >> InterceptorBinding
> > > >> >> ());
> > > >> >> > >
> > > >> >> > > addInterceptorBinding() gets the existing binding list and
> > > >> >> adds one to
> > > >> >> > > the bottom.
> > > >> >> > >
> > > >> >> > > Now if you simply modify this to add one to the top of the
> > > >> >> list, then
> > > >> >> > > it would reverse the specification order as defined by the the
> > > >> >> > > annotations in the class. That's a no-no.
> > > >> >> > >
> > > >> >> > > The right way to do this would be to
> > > >> >> > > 1. go over the collection of @Interceptor annotatations in the
> > > >> >> > > reverse order
> > > >> >> > > 2. insert the interceptor-binding to the top of the
> > > >> >> bindingsList in
> > > >> >> > > the assembly-descriptor.
> > > >> >> > >
> > > >> >> >
> > > >> >> > I mean at line 317 only (class-level interceptor) make a call
> > > >> like
> > > >> >> > this instead:
> > > >> >> >
> > > >> >> > IntercpetorBinding binding = new IntercpetorBinding()
> > > >> >> > assemblyDescriptor.getInterceptorBinding().add(0, binding);
> > > >> >> >
> > > >> >> > This would put only the annotated class-level binding before all
> > > >> >> the
> > > >> >> > other class-level bindings for that bean. It technically
> > > >> puts it
> > > >> >> > before all bindings, but they get resorted into package,
> > > >> class, and
> > > >> >> > method order anyway, so it works out fine.
> > > >> >> >
> > > >> >> > We wouldn't need to go over the @Interceptor annotation in
> > > >> reverse
> > > >> >> > order as a class can only have one @Interceptor annotation,
> > > >> so we
> > > >> >> > should be good as the loop will only ever execute once.
> > > >> >>
> > > >> >>
> > > >> >> Oh shucks. I knew that. While reading the code, I had the
> > > >> >> @Interceptors confused with the interceptors.value(). I don't
> > > >> know
> > > >> >> how that confusion crept in. Thanks for patiently clearing
> > > >> that up
> > > >> >> for me.
> > > >> >>
> > > >> >> Adding the binding to the top of the list does work.
> > > >> >>
> > > >> >> I think we should do the same for the method-level bindings too
> > > >> >> (line 335).
> > > >> >>
> > > >> >> Cheers
> > > >> >> Prasad
> > > >> >>
> > > >> >> >
> > > >> >> > Thoughts?
> > > >> >> >
> > > >> >> > -David
> > > >> >> >
> > > >> >> >
> > > >> >> > >
> > > >> >> > > On 3/21/07, David Blevins <da...@visi.com> wrote:
> > > >> >> > >> Wow, you're going to be the interceptor king pretty soon
> > > >> here :)
> > > >> >> > >> This is an extremely sharp observation. I certainly never
> > > >> >> saw it.
> > > >> >> > >>
> > > >> >> > >> Comments below...
> > > >> >> > >>
> > > >> >> > >> On Mar 21, 2007, at 12:37 PM, Prasad Kashyap wrote:
> > > >> >> > >>
> > > >> >> > >> > When an interceptor is declared in the DD and bound
> > > >> >> specifically
> > > >> >> > >> to a
> > > >> >> > >> > bean (not using the wildcard *), it becomes yet another
> > > >> class
> > > >> >> > >> > interceptor for the bean. In such a scenario, Section
> > > >> >> 12.8.2 of the
> > > >> >> > >> > spec says,
> > > >> >> > >> >
> > > >> >> > >> > "The binding of interceptors to classes is additive. If
> > > >> >> > >> interceptors
> > > >> >> > >> > are bound at the class-level and/or default-level as well
> > > >> >> as at the
> > > >> >> > >> > method-level, both class-level and/or default-level as
> > > >> well as
> > > >> >> > >> > method-level interceptors will apply. The deployment
> > > >> >> descriptor
> > > >> >> > >> may be
> > > >> >> > >> > used to augment the interceptors and interceptor methods
> > > >> >> defined by
> > > >> >> > >> > means of annotations.
> > > >> >> > >> >
> > > >> >> > >> > <emphasis>When the deployment descriptor is used to augment
> > > >> >> the
> > > >> >> > >> > interceptors specified in annotations, the interceptor
> > > >> methods
> > > >> >> > >> > specified in the deployment descriptor will be invoked
> > > >> >> after those
> > > >> >> > >> > specified in annotations, </emphasis>
> > > >> >> > >> >
> > > >> >> > >> > according to the ordering specified in sections 12.3.1 and
> > > >> >> > >> 12.4.1. The
> > > >> >> > >> > interceptor-order deployment descriptor element may be
> > > >> used to
> > > >> >> > >> > override this ordering."
> > > >> >> > >> >
> > > >> >> > >> > Expected outcome:
> > > >> >> > >> > -----------------------------
> > > >> >> > >> > As per the statement in <emphasis></emphasis> above, the
> > > >> >> > >> interceptor
> > > >> >> > >> > declared in the DD and bound to the bean should be invoked
> > > >> >> after
> > > >> >> > >> all
> > > >> >> > >> > the other class level interceptors specified by
> > > >> annotations.
> > > >> >> > >> This is
> > > >> >> > >> > assuming that the order is not modified by the
> > > >> <interceptor-
> > > >> >> order>
> > > >> >> > >> > element.
> > > >> >> > >> >
> > > >> >> > >> > Actual result:
> > > >> >> > >> > -------------------
> > > >> >> > >> > The actual result I'm seeing is the class level interceptor
> > > >> >> > >> bound in
> > > >> >> > >> > the DD gets invoked by before those specified by the
> > > >> >> @Interceptor
> > > >> >> > >> > annotation.
> > > >> >> > >>
> > > >> >> > >> This is easy to fix, we just need to tweak the
> > > >> >> AnnotationDeployer to
> > > >> >> > >> put the interceptor declarations found via the @Interceptors
> > > >> >> > >> annotation at the *beginning* of the list rather than at the
> > > >> >> end.
> > > >> >> > >>
> > > >> >> > >> http://fisheye6.cenqua.com/browse/openejb/trunk/openejb3/
> > > >> >> container/
> > > >> >> > >> openejb-core/src/main/java/org/apache/openejb/config/
> > > >> >> > >> AnnotationDeployer.java?r=519454#l335
> > > >> >> > >>
> > > >> >> > >> So right there on line 335 (love fisheye) we'd need to put
> > > >> that
> > > >> >> > >> binding *before* the bindings already declared in the list.
> > > >> >> > >>
> > > >> >> > >> Feel free to hack on it. It's actually a fantastic place to
> > > >> >> starting
> > > >> >> > >> digging into the runtime code.
> > > >> >> > >>
> > > >> >> > >> -David
> > > >> >> > >>
> > > >> >> > >>
> > > >> >> > >>
> > > >> >> > >
> > > >> >> >
> > > >> >> >
> > > >> >>
> > > >> >
> > > >>
> > > >>
> > > >
> > >
> > >
> >
>
Re: Wrong interceptor chaining order
Posted by Mohammad Nour El-Din <no...@gmail.com>.
Hi David...
I will commit Prasad's patch of his iTests today.
On 6/2/07, David Blevins <da...@visi.com> wrote:
>
> I don't recall, is there a test case for this one somewhere?
>
> Did your other itests ever get applied?
>
> -David
>
> On May 21, 2007, at 10:28 AM, Prasad Kashyap wrote:
>
> > JIRA to track the bug -
> > https://issues.apache.org/jira/browse/OPENEJB-584
> >
> > Cheers
> > Prasad
> >
> > On 3/24/07, Prasad Kashyap <go...@gmail.com> wrote:
> >> https://issues.apache.org/jira/secure/attachment/12354131/
> >> Interceptor-v4.patch
> >>
> >> https://issues.apache.org/jira/browse/OPENEJB-201
> >>
> >> Cheers
> >> Prasad
> >>
> >>
> >>
> >> On 3/23/07, Prasad Kashyap <go...@gmail.com> wrote:
> >> > This still does not seem to work for method-level interceptors.
> >> >
> >> > I have 2 method-level interceptors; 1 defined using annotation
> >> and 1
> >> > specified in the DD.
> >> >
> >> > As per the fix we applied recently, I am able to verify that the
> >> > binding from the annotation is added to the list before the binding
> >> > from the DD.
> >> >
> >> > Yet, the interceptor from the DD gets invoked before the
> >> interceptor
> >> > from the annotation !
> >> >
> >> > Cheers
> >> > Prasad
> >> >
> >> > On 3/22/07, David Blevins <da...@visi.com> wrote:
> >> > >
> >> > > On Mar 22, 2007, at 6:44 PM, Prasad Kashyap wrote:
> >> > >
> >> > > > Thanx..
> >> > > >
> >> > > > https://issues.apache.org/jira/secure/attachment/12354016/
> >> > > > AnnotationDeployer.patch
> >> > > >
> >> > >
> >> > > Applied! Thank you very much!
> >> > >
> >> > > -David
> >> > >
> >> > > >
> >> > > > On 3/22/07, David Blevins <da...@visi.com> wrote:
> >> > > >> Your patch got line wrapped.
> >> > > >>
> >> > > >> Here, I've reopened this issue, go ahead and throw in in
> >> there:
> >> > > >>
> >> > > >> http://issues.apache.org/jira/browse/OPENEJB-87
> >> > > >>
> >> > > >> -David
> >> > > >>
> >> > > >> On Mar 22, 2007, at 1:21 PM, Prasad Kashyap wrote:
> >> > > >>
> >> > > >> > Index: container/openejb-core/src/main/java/org/apache/
> >> openejb/
> >> > > >> > config/AnnotationDeployer.java
> >> > > >> >
> >> ===================================================================
> >> > > >> > --- container/openejb-core/src/main/java/org/apache/
> >> openejb/config/
> >> > > >> > AnnotationDeployer.java (revision
> >> > > >> > 521254)
> >> > > >> > +++ container/openejb-core/src/main/java/org/apache/
> >> openejb/config/
> >> > > >> > AnnotationDeployer.java (working
> >> > > >> > copy)
> >> > > >> > @@ -63,6 +63,7 @@
> >> > > >> > import java.net.URL;
> >> > > >> > import java.util.ArrayList;
> >> > > >> > import java.util.Arrays;
> >> > > >> > +import java.util.Collection;
> >> > > >> > import java.util.List;
> >> > > >> > import java.util.Map;
> >> > > >> >
> >> > > >> > @@ -307,14 +308,15 @@
> >> > > >> >
> >> > > >> > Interceptors interceptors =
> >> > > >> > clazz.getAnnotation(Interceptors.class);
> >> > > >> > if (interceptors != null){
> >> > > >> > - EjbJar ejbJar = ejbModule.getEjbJar();
> >> > > >> > + EjbJar ejbJar = ejbModule.getEjbJar();
> >> > > >> > for (Class interceptor :
> >> interceptors.value
> >> > > >> ()) {
> >> > > >> > if
> >> > > >> > (ejbJar.getInterceptor(interceptor.getName()) == null){
> >> > > >> > ejbJar.addInterceptor(new
> >> > > >> > Interceptor(interceptor.getName()));
> >> > > >> > }
> >> > > >> > }
> >> > > >> >
> >> > > >> > - InterceptorBinding binding =
> >> > > >> > assemblyDescriptor.addInterceptorBinding(new
> >> InterceptorBinding());
> >> > > >> > + InterceptorBinding binding = new
> >> > > >> > InterceptorBinding();
> >> > > >> > +
> >> assemblyDescriptor.getInterceptorBinding().add
> >> > > >> > (0, binding);
> >> > > >> > binding.setEjbName(bean.getEjbName());
> >> > > >> >
> >> > > >> > for (Class interceptor :
> >> interceptors.value
> >> > > >> ()) {
> >> > > >> > @@ -332,7 +334,8 @@
> >> > > >> > }
> >> > > >> > }
> >> > > >> >
> >> > > >> > - InterceptorBinding binding =
> >> > > >> > assemblyDescriptor.addInterceptorBinding(new
> >> InterceptorBinding());
> >> > > >> > + InterceptorBinding binding = new
> >> > > >> > InterceptorBinding();
> >> > > >> > +
> >> > > >> > assemblyDescriptor.getInterceptorBinding().add(0, binding);
> >> > > >> > binding.setEjbName(bean.getEjbName
> >> ());
> >> > > >> >
> >> > > >> > for (Class interceptor :
> >> interceptors.value
> >> > > >> > ()) {
> >> > > >> >
> >> > > >> >
> >> > > >> >
> >> > > >> > On 3/22/07, Prasad Kashyap <go...@gmail.com>
> >> wrote:
> >> > > >> >> On 3/22/07, David Blevins <da...@visi.com> wrote:
> >> > > >> >> >
> >> > > >> >> > On Mar 22, 2007, at 8:35 AM, Prasad Kashyap wrote:
> >> > > >> >> >
> >> > > >> >> > > Are you sure we can't just change the spec instead ?
> >> > > >> >> >
> >> > > >> >> > Maybe last year at this time :)
> >> > > >> >> >
> >> > > >> >> > > This is much complicated than the "simple" change you
> >> > > >> >> mentioned. It
> >> > > >> >> > > affects both lines 317 (class-level interceptor) and
> >> 335
> >> > > >> >> (method-level
> >> > > >> >> > > interceptor).
> >> > > >> >> > >
> >> > > >> >> > > InterceptorBinding binding =
> >> > > >> >> > > assemblyDescriptor.addInterceptorBinding(new
> >> > > >> InterceptorBinding
> >> > > >> >> ());
> >> > > >> >> > >
> >> > > >> >> > > addInterceptorBinding() gets the existing binding
> >> list and
> >> > > >> >> adds one to
> >> > > >> >> > > the bottom.
> >> > > >> >> > >
> >> > > >> >> > > Now if you simply modify this to add one to the top
> >> of the
> >> > > >> >> list, then
> >> > > >> >> > > it would reverse the specification order as defined
> >> by the the
> >> > > >> >> > > annotations in the class. That's a no-no.
> >> > > >> >> > >
> >> > > >> >> > > The right way to do this would be to
> >> > > >> >> > > 1. go over the collection of @Interceptor
> >> annotatations in the
> >> > > >> >> > > reverse order
> >> > > >> >> > > 2. insert the interceptor-binding to the top of the
> >> > > >> >> bindingsList in
> >> > > >> >> > > the assembly-descriptor.
> >> > > >> >> > >
> >> > > >> >> >
> >> > > >> >> > I mean at line 317 only (class-level interceptor) make
> >> a call
> >> > > >> like
> >> > > >> >> > this instead:
> >> > > >> >> >
> >> > > >> >> > IntercpetorBinding binding = new IntercpetorBinding()
> >> > > >> >> > assemblyDescriptor.getInterceptorBinding().add(0,
> >> binding);
> >> > > >> >> >
> >> > > >> >> > This would put only the annotated class-level binding
> >> before all
> >> > > >> >> the
> >> > > >> >> > other class-level bindings for that bean. It technically
> >> > > >> puts it
> >> > > >> >> > before all bindings, but they get resorted into package,
> >> > > >> class, and
> >> > > >> >> > method order anyway, so it works out fine.
> >> > > >> >> >
> >> > > >> >> > We wouldn't need to go over the @Interceptor
> >> annotation in
> >> > > >> reverse
> >> > > >> >> > order as a class can only have one @Interceptor
> >> annotation,
> >> > > >> so we
> >> > > >> >> > should be good as the loop will only ever execute once.
> >> > > >> >>
> >> > > >> >>
> >> > > >> >> Oh shucks. I knew that. While reading the code, I had the
> >> > > >> >> @Interceptors confused with the interceptors.value(). I
> >> don't
> >> > > >> know
> >> > > >> >> how that confusion crept in. Thanks for patiently clearing
> >> > > >> that up
> >> > > >> >> for me.
> >> > > >> >>
> >> > > >> >> Adding the binding to the top of the list does work.
> >> > > >> >>
> >> > > >> >> I think we should do the same for the method-level
> >> bindings too
> >> > > >> >> (line 335).
> >> > > >> >>
> >> > > >> >> Cheers
> >> > > >> >> Prasad
> >> > > >> >>
> >> > > >> >> >
> >> > > >> >> > Thoughts?
> >> > > >> >> >
> >> > > >> >> > -David
> >> > > >> >> >
> >> > > >> >> >
> >> > > >> >> > >
> >> > > >> >> > > On 3/21/07, David Blevins <da...@visi.com>
> >> wrote:
> >> > > >> >> > >> Wow, you're going to be the interceptor king pretty
> >> soon
> >> > > >> here :)
> >> > > >> >> > >> This is an extremely sharp observation. I
> >> certainly never
> >> > > >> >> saw it.
> >> > > >> >> > >>
> >> > > >> >> > >> Comments below...
> >> > > >> >> > >>
> >> > > >> >> > >> On Mar 21, 2007, at 12:37 PM, Prasad Kashyap wrote:
> >> > > >> >> > >>
> >> > > >> >> > >> > When an interceptor is declared in the DD and bound
> >> > > >> >> specifically
> >> > > >> >> > >> to a
> >> > > >> >> > >> > bean (not using the wildcard *), it becomes yet
> >> another
> >> > > >> class
> >> > > >> >> > >> > interceptor for the bean. In such a scenario,
> >> Section
> >> > > >> >> 12.8.2 of the
> >> > > >> >> > >> > spec says,
> >> > > >> >> > >> >
> >> > > >> >> > >> > "The binding of interceptors to classes is
> >> additive. If
> >> > > >> >> > >> interceptors
> >> > > >> >> > >> > are bound at the class-level and/or default-level
> >> as well
> >> > > >> >> as at the
> >> > > >> >> > >> > method-level, both class-level and/or default-
> >> level as
> >> > > >> well as
> >> > > >> >> > >> > method-level interceptors will apply. The deployment
> >> > > >> >> descriptor
> >> > > >> >> > >> may be
> >> > > >> >> > >> > used to augment the interceptors and interceptor
> >> methods
> >> > > >> >> defined by
> >> > > >> >> > >> > means of annotations.
> >> > > >> >> > >> >
> >> > > >> >> > >> > <emphasis>When the deployment descriptor is used
> >> to augment
> >> > > >> >> the
> >> > > >> >> > >> > interceptors specified in annotations, the
> >> interceptor
> >> > > >> methods
> >> > > >> >> > >> > specified in the deployment descriptor will be
> >> invoked
> >> > > >> >> after those
> >> > > >> >> > >> > specified in annotations, </emphasis>
> >> > > >> >> > >> >
> >> > > >> >> > >> > according to the ordering specified in sections
> >> 12.3.1 and
> >> > > >> >> > >> 12.4.1. The
> >> > > >> >> > >> > interceptor-order deployment descriptor element
> >> may be
> >> > > >> used to
> >> > > >> >> > >> > override this ordering."
> >> > > >> >> > >> >
> >> > > >> >> > >> > Expected outcome:
> >> > > >> >> > >> > -----------------------------
> >> > > >> >> > >> > As per the statement in <emphasis></emphasis>
> >> above, the
> >> > > >> >> > >> interceptor
> >> > > >> >> > >> > declared in the DD and bound to the bean should
> >> be invoked
> >> > > >> >> after
> >> > > >> >> > >> all
> >> > > >> >> > >> > the other class level interceptors specified by
> >> > > >> annotations.
> >> > > >> >> > >> This is
> >> > > >> >> > >> > assuming that the order is not modified by the
> >> > > >> <interceptor-
> >> > > >> >> order>
> >> > > >> >> > >> > element.
> >> > > >> >> > >> >
> >> > > >> >> > >> > Actual result:
> >> > > >> >> > >> > -------------------
> >> > > >> >> > >> > The actual result I'm seeing is the class level
> >> interceptor
> >> > > >> >> > >> bound in
> >> > > >> >> > >> > the DD gets invoked by before those specified by the
> >> > > >> >> @Interceptor
> >> > > >> >> > >> > annotation.
> >> > > >> >> > >>
> >> > > >> >> > >> This is easy to fix, we just need to tweak the
> >> > > >> >> AnnotationDeployer to
> >> > > >> >> > >> put the interceptor declarations found via the
> >> @Interceptors
> >> > > >> >> > >> annotation at the *beginning* of the list rather
> >> than at the
> >> > > >> >> end.
> >> > > >> >> > >>
> >> > > >> >> > >> http://fisheye6.cenqua.com/browse/openejb/trunk/
> >> openejb3/
> >> > > >> >> container/
> >> > > >> >> > >> openejb-core/src/main/java/org/apache/openejb/config/
> >> > > >> >> > >> AnnotationDeployer.java?r=519454#l335
> >> > > >> >> > >>
> >> > > >> >> > >> So right there on line 335 (love fisheye) we'd need
> >> to put
> >> > > >> that
> >> > > >> >> > >> binding *before* the bindings already declared in
> >> the list.
> >> > > >> >> > >>
> >> > > >> >> > >> Feel free to hack on it. It's actually a fantastic
> >> place to
> >> > > >> >> starting
> >> > > >> >> > >> digging into the runtime code.
> >> > > >> >> > >>
> >> > > >> >> > >> -David
> >> > > >> >> > >>
> >> > > >> >> > >>
> >> > > >> >> > >>
> >> > > >> >> > >
> >> > > >> >> >
> >> > > >> >> >
> >> > > >> >>
> >> > > >> >
> >> > > >>
> >> > > >>
> >> > > >
> >> > >
> >> > >
> >> >
> >>
> >
>
>
--
Thanks
- Mohammad Nour
Re: Wrong interceptor chaining order
Posted by David Blevins <da...@visi.com>.
I don't recall, is there a test case for this one somewhere?
Did your other itests ever get applied?
-David
On May 21, 2007, at 10:28 AM, Prasad Kashyap wrote:
> JIRA to track the bug -
> https://issues.apache.org/jira/browse/OPENEJB-584
>
> Cheers
> Prasad
>
> On 3/24/07, Prasad Kashyap <go...@gmail.com> wrote:
>> https://issues.apache.org/jira/secure/attachment/12354131/
>> Interceptor-v4.patch
>>
>> https://issues.apache.org/jira/browse/OPENEJB-201
>>
>> Cheers
>> Prasad
>>
>>
>>
>> On 3/23/07, Prasad Kashyap <go...@gmail.com> wrote:
>> > This still does not seem to work for method-level interceptors.
>> >
>> > I have 2 method-level interceptors; 1 defined using annotation
>> and 1
>> > specified in the DD.
>> >
>> > As per the fix we applied recently, I am able to verify that the
>> > binding from the annotation is added to the list before the binding
>> > from the DD.
>> >
>> > Yet, the interceptor from the DD gets invoked before the
>> interceptor
>> > from the annotation !
>> >
>> > Cheers
>> > Prasad
>> >
>> > On 3/22/07, David Blevins <da...@visi.com> wrote:
>> > >
>> > > On Mar 22, 2007, at 6:44 PM, Prasad Kashyap wrote:
>> > >
>> > > > Thanx..
>> > > >
>> > > > https://issues.apache.org/jira/secure/attachment/12354016/
>> > > > AnnotationDeployer.patch
>> > > >
>> > >
>> > > Applied! Thank you very much!
>> > >
>> > > -David
>> > >
>> > > >
>> > > > On 3/22/07, David Blevins <da...@visi.com> wrote:
>> > > >> Your patch got line wrapped.
>> > > >>
>> > > >> Here, I've reopened this issue, go ahead and throw in in
>> there:
>> > > >>
>> > > >> http://issues.apache.org/jira/browse/OPENEJB-87
>> > > >>
>> > > >> -David
>> > > >>
>> > > >> On Mar 22, 2007, at 1:21 PM, Prasad Kashyap wrote:
>> > > >>
>> > > >> > Index: container/openejb-core/src/main/java/org/apache/
>> openejb/
>> > > >> > config/AnnotationDeployer.java
>> > > >> >
>> ===================================================================
>> > > >> > --- container/openejb-core/src/main/java/org/apache/
>> openejb/config/
>> > > >> > AnnotationDeployer.java (revision
>> > > >> > 521254)
>> > > >> > +++ container/openejb-core/src/main/java/org/apache/
>> openejb/config/
>> > > >> > AnnotationDeployer.java (working
>> > > >> > copy)
>> > > >> > @@ -63,6 +63,7 @@
>> > > >> > import java.net.URL;
>> > > >> > import java.util.ArrayList;
>> > > >> > import java.util.Arrays;
>> > > >> > +import java.util.Collection;
>> > > >> > import java.util.List;
>> > > >> > import java.util.Map;
>> > > >> >
>> > > >> > @@ -307,14 +308,15 @@
>> > > >> >
>> > > >> > Interceptors interceptors =
>> > > >> > clazz.getAnnotation(Interceptors.class);
>> > > >> > if (interceptors != null){
>> > > >> > - EjbJar ejbJar = ejbModule.getEjbJar();
>> > > >> > + EjbJar ejbJar = ejbModule.getEjbJar();
>> > > >> > for (Class interceptor :
>> interceptors.value
>> > > >> ()) {
>> > > >> > if
>> > > >> > (ejbJar.getInterceptor(interceptor.getName()) == null){
>> > > >> > ejbJar.addInterceptor(new
>> > > >> > Interceptor(interceptor.getName()));
>> > > >> > }
>> > > >> > }
>> > > >> >
>> > > >> > - InterceptorBinding binding =
>> > > >> > assemblyDescriptor.addInterceptorBinding(new
>> InterceptorBinding());
>> > > >> > + InterceptorBinding binding = new
>> > > >> > InterceptorBinding();
>> > > >> > +
>> assemblyDescriptor.getInterceptorBinding().add
>> > > >> > (0, binding);
>> > > >> > binding.setEjbName(bean.getEjbName());
>> > > >> >
>> > > >> > for (Class interceptor :
>> interceptors.value
>> > > >> ()) {
>> > > >> > @@ -332,7 +334,8 @@
>> > > >> > }
>> > > >> > }
>> > > >> >
>> > > >> > - InterceptorBinding binding =
>> > > >> > assemblyDescriptor.addInterceptorBinding(new
>> InterceptorBinding());
>> > > >> > + InterceptorBinding binding = new
>> > > >> > InterceptorBinding();
>> > > >> > +
>> > > >> > assemblyDescriptor.getInterceptorBinding().add(0, binding);
>> > > >> > binding.setEjbName(bean.getEjbName
>> ());
>> > > >> >
>> > > >> > for (Class interceptor :
>> interceptors.value
>> > > >> > ()) {
>> > > >> >
>> > > >> >
>> > > >> >
>> > > >> > On 3/22/07, Prasad Kashyap <go...@gmail.com>
>> wrote:
>> > > >> >> On 3/22/07, David Blevins <da...@visi.com> wrote:
>> > > >> >> >
>> > > >> >> > On Mar 22, 2007, at 8:35 AM, Prasad Kashyap wrote:
>> > > >> >> >
>> > > >> >> > > Are you sure we can't just change the spec instead ?
>> > > >> >> >
>> > > >> >> > Maybe last year at this time :)
>> > > >> >> >
>> > > >> >> > > This is much complicated than the "simple" change you
>> > > >> >> mentioned. It
>> > > >> >> > > affects both lines 317 (class-level interceptor) and
>> 335
>> > > >> >> (method-level
>> > > >> >> > > interceptor).
>> > > >> >> > >
>> > > >> >> > > InterceptorBinding binding =
>> > > >> >> > > assemblyDescriptor.addInterceptorBinding(new
>> > > >> InterceptorBinding
>> > > >> >> ());
>> > > >> >> > >
>> > > >> >> > > addInterceptorBinding() gets the existing binding
>> list and
>> > > >> >> adds one to
>> > > >> >> > > the bottom.
>> > > >> >> > >
>> > > >> >> > > Now if you simply modify this to add one to the top
>> of the
>> > > >> >> list, then
>> > > >> >> > > it would reverse the specification order as defined
>> by the the
>> > > >> >> > > annotations in the class. That's a no-no.
>> > > >> >> > >
>> > > >> >> > > The right way to do this would be to
>> > > >> >> > > 1. go over the collection of @Interceptor
>> annotatations in the
>> > > >> >> > > reverse order
>> > > >> >> > > 2. insert the interceptor-binding to the top of the
>> > > >> >> bindingsList in
>> > > >> >> > > the assembly-descriptor.
>> > > >> >> > >
>> > > >> >> >
>> > > >> >> > I mean at line 317 only (class-level interceptor) make
>> a call
>> > > >> like
>> > > >> >> > this instead:
>> > > >> >> >
>> > > >> >> > IntercpetorBinding binding = new IntercpetorBinding()
>> > > >> >> > assemblyDescriptor.getInterceptorBinding().add(0,
>> binding);
>> > > >> >> >
>> > > >> >> > This would put only the annotated class-level binding
>> before all
>> > > >> >> the
>> > > >> >> > other class-level bindings for that bean. It technically
>> > > >> puts it
>> > > >> >> > before all bindings, but they get resorted into package,
>> > > >> class, and
>> > > >> >> > method order anyway, so it works out fine.
>> > > >> >> >
>> > > >> >> > We wouldn't need to go over the @Interceptor
>> annotation in
>> > > >> reverse
>> > > >> >> > order as a class can only have one @Interceptor
>> annotation,
>> > > >> so we
>> > > >> >> > should be good as the loop will only ever execute once.
>> > > >> >>
>> > > >> >>
>> > > >> >> Oh shucks. I knew that. While reading the code, I had the
>> > > >> >> @Interceptors confused with the interceptors.value(). I
>> don't
>> > > >> know
>> > > >> >> how that confusion crept in. Thanks for patiently clearing
>> > > >> that up
>> > > >> >> for me.
>> > > >> >>
>> > > >> >> Adding the binding to the top of the list does work.
>> > > >> >>
>> > > >> >> I think we should do the same for the method-level
>> bindings too
>> > > >> >> (line 335).
>> > > >> >>
>> > > >> >> Cheers
>> > > >> >> Prasad
>> > > >> >>
>> > > >> >> >
>> > > >> >> > Thoughts?
>> > > >> >> >
>> > > >> >> > -David
>> > > >> >> >
>> > > >> >> >
>> > > >> >> > >
>> > > >> >> > > On 3/21/07, David Blevins <da...@visi.com>
>> wrote:
>> > > >> >> > >> Wow, you're going to be the interceptor king pretty
>> soon
>> > > >> here :)
>> > > >> >> > >> This is an extremely sharp observation. I
>> certainly never
>> > > >> >> saw it.
>> > > >> >> > >>
>> > > >> >> > >> Comments below...
>> > > >> >> > >>
>> > > >> >> > >> On Mar 21, 2007, at 12:37 PM, Prasad Kashyap wrote:
>> > > >> >> > >>
>> > > >> >> > >> > When an interceptor is declared in the DD and bound
>> > > >> >> specifically
>> > > >> >> > >> to a
>> > > >> >> > >> > bean (not using the wildcard *), it becomes yet
>> another
>> > > >> class
>> > > >> >> > >> > interceptor for the bean. In such a scenario,
>> Section
>> > > >> >> 12.8.2 of the
>> > > >> >> > >> > spec says,
>> > > >> >> > >> >
>> > > >> >> > >> > "The binding of interceptors to classes is
>> additive. If
>> > > >> >> > >> interceptors
>> > > >> >> > >> > are bound at the class-level and/or default-level
>> as well
>> > > >> >> as at the
>> > > >> >> > >> > method-level, both class-level and/or default-
>> level as
>> > > >> well as
>> > > >> >> > >> > method-level interceptors will apply. The deployment
>> > > >> >> descriptor
>> > > >> >> > >> may be
>> > > >> >> > >> > used to augment the interceptors and interceptor
>> methods
>> > > >> >> defined by
>> > > >> >> > >> > means of annotations.
>> > > >> >> > >> >
>> > > >> >> > >> > <emphasis>When the deployment descriptor is used
>> to augment
>> > > >> >> the
>> > > >> >> > >> > interceptors specified in annotations, the
>> interceptor
>> > > >> methods
>> > > >> >> > >> > specified in the deployment descriptor will be
>> invoked
>> > > >> >> after those
>> > > >> >> > >> > specified in annotations, </emphasis>
>> > > >> >> > >> >
>> > > >> >> > >> > according to the ordering specified in sections
>> 12.3.1 and
>> > > >> >> > >> 12.4.1. The
>> > > >> >> > >> > interceptor-order deployment descriptor element
>> may be
>> > > >> used to
>> > > >> >> > >> > override this ordering."
>> > > >> >> > >> >
>> > > >> >> > >> > Expected outcome:
>> > > >> >> > >> > -----------------------------
>> > > >> >> > >> > As per the statement in <emphasis></emphasis>
>> above, the
>> > > >> >> > >> interceptor
>> > > >> >> > >> > declared in the DD and bound to the bean should
>> be invoked
>> > > >> >> after
>> > > >> >> > >> all
>> > > >> >> > >> > the other class level interceptors specified by
>> > > >> annotations.
>> > > >> >> > >> This is
>> > > >> >> > >> > assuming that the order is not modified by the
>> > > >> <interceptor-
>> > > >> >> order>
>> > > >> >> > >> > element.
>> > > >> >> > >> >
>> > > >> >> > >> > Actual result:
>> > > >> >> > >> > -------------------
>> > > >> >> > >> > The actual result I'm seeing is the class level
>> interceptor
>> > > >> >> > >> bound in
>> > > >> >> > >> > the DD gets invoked by before those specified by the
>> > > >> >> @Interceptor
>> > > >> >> > >> > annotation.
>> > > >> >> > >>
>> > > >> >> > >> This is easy to fix, we just need to tweak the
>> > > >> >> AnnotationDeployer to
>> > > >> >> > >> put the interceptor declarations found via the
>> @Interceptors
>> > > >> >> > >> annotation at the *beginning* of the list rather
>> than at the
>> > > >> >> end.
>> > > >> >> > >>
>> > > >> >> > >> http://fisheye6.cenqua.com/browse/openejb/trunk/
>> openejb3/
>> > > >> >> container/
>> > > >> >> > >> openejb-core/src/main/java/org/apache/openejb/config/
>> > > >> >> > >> AnnotationDeployer.java?r=519454#l335
>> > > >> >> > >>
>> > > >> >> > >> So right there on line 335 (love fisheye) we'd need
>> to put
>> > > >> that
>> > > >> >> > >> binding *before* the bindings already declared in
>> the list.
>> > > >> >> > >>
>> > > >> >> > >> Feel free to hack on it. It's actually a fantastic
>> place to
>> > > >> >> starting
>> > > >> >> > >> digging into the runtime code.
>> > > >> >> > >>
>> > > >> >> > >> -David
>> > > >> >> > >>
>> > > >> >> > >>
>> > > >> >> > >>
>> > > >> >> > >
>> > > >> >> >
>> > > >> >> >
>> > > >> >>
>> > > >> >
>> > > >>
>> > > >>
>> > > >
>> > >
>> > >
>> >
>>
>