Hi David... I will commit Prasad's patch of his iTests today.
On 6/2/07, David Blevins <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[EMAIL PROTECTED]> 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 <[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 >> > > >> >> > >> >> > > >> >> > >> >> > > >> >> > >> >> > > >> >> > > >> > > >> >> > >> > > >> >> > >> > > >> >> >> > > >> > >> > > >> >> > > >> >> > > > >> > > >> > > >> > >> >
-- Thanks - Mohammad Nour
