javaee / interceptors-spec

interceptors-spec~interceptors-spec-repository
Other
6 stars 3 forks source link

What is the reason to allow basically only one way of interceptor association to target class #33

Closed tremes closed 7 years ago

tremes commented 7 years ago

Latest update of spec document states: If an interceptor does not declare an Interceptor annotation, it can be bound to components using the Interceptors annotation. and also: The Interceptor annotation is ignored on interceptor classes bound using the Interceptors annotation. What is the reason for such new restriction? In Weld if the interceptor is associated by binding as well as by using @Interceptors then it is called twice. Additionally it's not very clear what means this ignore. Does it mean that CDI interceptor should be turned off in such case and only @Interceptors invocation counts ??

tremes commented 7 years ago

The code in question is : @Interceptors(MyInterceptor.class) @MyIntBinding class Foo and @MyIntBinding @Interceptor MyInterceptor

Will be the MyInterceptor (let's say around-invoke) invocation called twice for Foo class?

Emily-Jiang commented 7 years ago

@tremes I guess MyInterceptor is enabled via beans.xml in your case.

+1 on this issue. I think the spec needs to be clarified. I prefer the CDI interceptor is not ignored if they are enabled. The @Interceptor should only be ignored if the binding is not present or not enabled. Otherwise, this is behavior change.

manovotn commented 7 years ago

Adding my voice to this issue; I think this should be treated. The current change presents, at worst, an incompatible change or, at best, invites ambiguous interpretations. We should avoid that while we still can.

Also, ignoring the interceptor would be very costly (at least for CDI) because you would need to have all the classes scanned just to identify all classes mentioned in @Interceptors.

@Emily-Jiang I would say @tremes just forgot to add @Priority to the code sample :-)

tremes commented 7 years ago

Yes the MyInterceptor should be considered as enabled. No matter if in beans.xml or by using @Priority.

ldemichiel commented 7 years ago

The statement in question was intended solely in the context of processing the @Interceptors annotation, and not intended to mean that it would cause interceptor binding usages of the class to be ignored.

For the example @Interceptors(MyInterceptor.class) @MyIntBinding class Foo and @MyIntBinding @Interceptor MyInterceptor MyInterceptor should be called twice for Foo according to the rules in section 5.2.

ldemichiel commented 7 years ago

With regard to spec changes, I think the spec should be modified as follows. This text should be merged into the paragraph that currently precedes it: The Interceptor annotation is ignored during the processing of classes bound using the Interceptors annotation. (It will continue to be observed on such classes when used in the context of interceptor binding.)

Please let me know if you continue to see any issues with this.

tremes commented 7 years ago

Thanks for clarifiaction. OK makes sense then. I think this suggested update should be OK.

manovotn commented 7 years ago

+1 for suggested changes

Emily-Jiang commented 7 years ago

+1. I suggest to remove the parenthesis and make it part of the spec. "The Interceptor annotation is ignored during the processing of classes bound using the Interceptors annotation. It will continue to be observed on such classes when used in the context of interceptor binding."

Emily-Jiang commented 7 years ago

@ldemichiel Have you made the suggested changes as yet? If yes, please share the updated version. Thanks

ldemichiel commented 7 years ago

Clarified in Interceptors 1.2 rev A specification