Use CDI_FULL test group to exclude @Interceptors tests

[starksm64]

Breaking up the sets of tests into separate categories, first @interceptors based tests need to added to the CDI_FULL test group.

[manovotn]

Note that you might need to annotate more than just tests with @Interceptors - for instance tests declaring around invoke methods directly on target classes and possibly more.

[starksm64]

I have found tests that are using the interceptors element of the beans.xml that I need to add.

What is the issue with around invoke? What is the full set of features from the interceptors spec we are not supporting in lite?

[manovotn]

What is the issue with around invoke? What is the full set of features from the interceptors spec we are not supporting in lite?

It is easier to define the other way around. What we do support is:

• Interception of all lifecycle methods defined via interceptor class and an interceptor binding
• Enablement of interceptors is only possible through @Priority
• We allow @PreDestroy and @PostConstruct defined on target classes (in beans)

Everything else is CDI Full only.

@Ladicek /@mkouba correct me if I am wrong :)

[Emily-Jiang]

Why do we move interceptors to CDI Full? I recall interceptor support is required in CDI Lite.

[manovotn]

@Emily-Jiang we are not moving them all, please read the issue carefully - interceptors are supported in Lite but not in all forms. For instance @Interceptors annotation won't be supported but the classic way via bindings will be working just fine.

[Emily-Jiang]

@Emily-Jiang we are not moving them all, please read the issue carefully - interceptors are supported in Lite but not in all forms. For instance @Interceptors annotation won't be supported but the classic way via bindings will be working just fine.

ah, my apologies. I missed some context. For @Interceptors, I think this is used by EJB and managed beans. Should we move these tests to EJB instead since we are discussing to move some EJB TCKs in CDI to EJB TCK? Maybe, how about grouping them differently e.g. EJB-TCK and move them over the fence?

[Ladicek]

I don't think we have ever specified it in detail. Which we should, IMHO.

So there's a couple of interceptor kinds:

• lifecycle callbacks
  • @AroundConstruct
  • @PostConstruct
  • @PreDestroy
• method-level interceptors
  • business method interceptors (@AroundInvoke)
  • timeout interceptors (@AroundTimeout) [EJB specific, so can ignore here]
• constructor-level interceptors
  • isn't that basically the same thing as @AroundConstruct?

Interceptors may be declared:

• as interceptor methods on the target class
• as interceptor methods on the interceptor class
• by implementing the Interceptor interface [CDI specific]

Interceptor classes can be associated with target classes using:

• interceptor binding annotations
• the @Interceptors annotation
• InterceptionFactory, for producer methods [CDI specific]

Interceptors may be enabled using:

• the @Priority annotation, for interceptors that are associated using interceptor bindings
• the @Interceptors annotation, for interceptors that are associated using that annotation
• the beans.xml descriptor [CDI specific]

Interceptors may be disabled using:

• the @ExcludeClassInterceptors annotation, for interceptors that are associated using @Interceptors
• the @ExcludeDefaultInterceptors annotation, for "default interceptors"

Did I miss anything? This seems to be a 5-dimensional matrix, so hard to convert to a 2D format :-) I'll try anyway.

✔️ means "part of CDI Lite"
❌ means "not part of CDI Lite, only CDI Full"
❔ means "I don't know, please help" :-)
☣️ means "can't happen"

	@AroundConstruct	@PostConstruct, @PreDestroy	@AroundInvoke
On target class	☣️	✔️	❔
On interceptor class, binding annotation + @Priority	✔️	✔️	✔️
On interceptor class, @Interceptors	❌	❌	❌
On interceptor class, enabled using beans.xml	❌	❌	❌
Implementing Interceptor	❌	❌	❌
Using InterceptionFactory	☣️	☣️	❌

I don't know what to think of @ExcludeDefaultInterceptors.

[starksm64]

Well, it seems @ExcludeDefaultInterceptors and @ExcludeClassInterceptors should be an X in terms of Lite since they are related to @interceptors.

[Ladicek]

@ExcludeClassInterceptors for sure, that's explicitly specified to apply to @Interceptors (so I didn't feel compelled to add an ❌ explicitly, because there's already an ❌ for @Interceptors).

I don't see that stated for @ExcludeDefaultInterceptors, but then, I don't even know what specifications define "default interceptors" (CDI doesn't, AFAIK), so maybe it's a non-issue.

[manovotn]

Had to look it up. We don't even seem to use ExcludeDefaultInterceptors in the TCK and the the only notion of it in Weld tests was this one.

Spec says the following about default interceptors:

Default interceptors are interceptors that apply to a set of target classes. An extension specification may support the use of a deployment descriptor or annotations to define default interceptors and their relative ordering.

[manovotn]

@Ladicek to answer your question marks:

• @AroundConstruct on a target class is ❌ - this is invalid according to the specification; the javadoc of the annotation literally forbids this and you can only declare it in an interceptor class
• Lifecycle methods in interceptor classes are ✔️ - here I think we can easily support all of them, so @AroundConstruct, @AroundInvoke, @PostConstruct, @PreDestroy
• @AroundInvoke on target class is IMO ❌

[Emily-Jiang]

• @AroundInvoke on target class is IMO ❌

According to the spec, AroundInvoke methods may be declared in interceptor classes, in the superclasses of interceptor classes, in the target class, and/or in superclasses of the target class. . It needs to be consistent between specifying on an interceptor class or target class. Why applying on an interceptor class is in Lite while applying on a target class is in FULL.

By the way, all of the xInterceptors are related to EJB. I think they should be moved to EJB spec.

[Ladicek]

Thanks @manovotn, I wasn't thinking, obviously @AroundConstruct can't be present on a target class, there's no way how that could possibly work (unless the interceptor method was static :-) ). I marked it as ☣️, prohibited by the spec.

I marked lifecycle callbacks on interceptor class as ✔️.

The single remaining item is @AroundInvoke on target class. While I don't see that as useful, perhaps we should demand its support just for conceptual clarity. I'll leave it as ❔, so that more people can voice their opinion.

@Emily-Jiang I wish @Interceptors was part of EJB from the start, because moving it from the Interceptors spec to EJB now would be a pretty big breaking change :-/

[mkouba]

Why applying on an interceptor class is in Lite while applying on a target class is in FULL.

The single remaining item is @AroundInvoke on target class. While I don't see that as useful, perhaps we should demand its support just for conceptual clarity. I'll leave it as grey_question, so that more people can voice their opinion.

I personally find this feature a bit useless and TBH I've only seen a very few usages during the many years of CDI development. And while the conceptual clarity is a nice thing it will add unnecessary complexity to the implementation.

If you need to "intercept" all/some methods of the target class it's probably better to use the regular language constructs instead of CDI interceptors, for example:

import java.util.concurrent.Callable;
import java.util.concurrent.TimeUnit;

public class Foo {

    String ping() {
        return monitor(() -> "foo");
    }

    void log() {
        monitor(() -> null);
    }

    <T> T monitor(Callable<T> action) {
        long start = System.nanoTime();
        try {
            return action.call();
        } catch (Exception e) {
            throw new RuntimeException(e);
        } finally {
            System.out.println("Took: " + TimeUnit.NANOSECONDS.toMillis(System.nanoTime() - start));
        }
    }

}

You'll definitely get better performance, type-safety and there's no need for reflection along the way.

And if you really need a dedicated CDI interceptor then nested classes seem to be an easy solution:

public class Foo {

    @Monitored
    String ping() {
        return "foo";
    }

    @Monitored
    void log() {
    }
   
    @Target({ TYPE, METHOD })
    @Retention(RUNTIME)
    @InterceptorBinding
    pullic @interface Monitored {}

    @Monitored
    @Priority(1)
    @Interceptor
    public static Monitor {
    
       @AroundInvoke
       Object monitor(InvocationContext ctx) throws Exception { 
          // ...
       }
    }
}

[manovotn]

@Ladicek @starksm64 since the PR was merged, can we consider this issue resolved? Or is there anything else to do?

[starksm64]

I think we are good to close this.

[graemerocher]

@manovotn @starksm64 @Ladicek what was the final decision on @AroundInvoke in the target class. I see definitions like:

https://github.com/eclipse-ee4j/cdi-tck/blob/master/impl/src/main/java/org/jboss/cdi/tck/interceptors/tests/contract/aroundInvoke/Bean3.java

Are included with the lite tests?

[manovotn]

@graemerocher you are right, this isn't supposed to be in Lite.
cdi-tck/impl/src/main/java/org/jboss/cdi/tck/interceptors/tests/ is where all interceptor specification TCK tests are. I think I reworked most interceptors tests to operate on interceptors with bindings and we didn't want to split interceptors TCK into different packages.

I must have missed this one (and possibly others?), it was lots and lots of tests...
Looking at the assertions, it seems to be testing this:

        <group>
            <text>Around-invoke methods can have |public|, |private|, |protected|, or |package| level access.</text>
            <assertion id="ca">
                <text>Test |public| level access.</text>
            </assertion>
            <assertion id="cb">
                <text>Test |private| level access.</text>
            </assertion>
            <assertion id="cc">
                <text>Test |protected| level access.</text>
            </assertion>
            <assertion id="cd">
                <text>Test |package| level access.</text>
            </assert

Which means this TCK test can be rewritten to use the standard interceptor with bindings.
But we should then keep a copy of this test (or verify there is other similar) for CDI Full.

[graemerocher]

@manovotn thanks for the clarification. There seem to be several cases, like this one as well:

https://github.com/eclipse-ee4j/cdi-tck/blob/master/impl/src/main/java/org/jboss/cdi/tck/interceptors/tests/order/aroundInvoke/Vehicle.java

[manovotn]

Yeah, most of the reworks I did were focused around @Interceptors and other stuff that was grep-able.
And the verification I could do was only with Weld which supports it all.

The one you mentioned will need to be marked Full and we'll need to make a similar version for Lite with only the supported bits (so that we don't coverage on this).

[manovotn]

Feel free to send PRs or, at the very least, please create new issue(s) for them so we keep track of all of them.