Re: Wrong interceptor chaining order

Thu, 22 Mar 2007 12:49:40 -0800 

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 <[EMAIL PROTECTED]> wrote:

On 3/22/07, David Blevins <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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
> >>
> >>
> >>
> >
>
>

Reply via email to