Quarkus: Repeated @InterceptorBinding are ignored by arc-processor

[gwenneg]

I struggled for a while today with a repeatable interceptor binding that was ignored by arc-processor until I found in debug mode why it is ignored.

Let's assume the interceptor binding looks like this:

@InterceptorBinding
@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.TYPE, ElementType.METHOD })
@Repeatable(Bar.class)
public @interface Foo {

    @Nonbinding
    String someValue() default "";

    @Retention(RetentionPolicy.RUNTIME)
    @Target(ElementType.METHOD)
    @interface Bar {
        Foo[] value();
    }
}

And is used that way:

@ApplicationScoped
public class EmptyService {
    @Foo(someValue = "one")
    @Foo(someValue = "two")
    public void doSomething() {
    }
}

Then, in the following code the repeated interceptor bindings will be ignored because beanDeployment doesn't know any interceptor binding named Foo$Bar:
https://github.com/quarkusio/quarkus/blob/dda2f83a1c0b1ca65884e4a0b65fc80070e57418/independent-projects/arc/processor/src/main/java/io/quarkus/arc/processor/Methods.java#L158-L160

Here's the value of methodAnnotations during the code execution:

[@Bar(value = [@Foo(someValue = "1"),@Foo(someValue = "2")])]

@mkouba Am I wrong trying to use repeatable interceptor bindings or is this a bug? It currently works when the bindings are added using an AnnotationsTransformer but I'd like it to work without the transformer as well.

[quarkus-bot[bot]]

/cc @manovotn, @mkouba

[manovotn]

This is a bug. Or to be more precise, we probably just didn't need it yet :-)

While interceptors spec doesn't specifically say it should work, it doesn't forbid it either.
And Weld seems to have tests for it - https://github.com/weld/core/blob/3.1/tests-arquillian/src/test/java/org/jboss/weld/tests/repeatable/IncrementingInterceptor.java

[gwenneg]

Thanks for the answer @manovotn.

I self-assigned this issue because I thought I had a clean fix, but it's not pretty at all in the end so I'm unassigning...

[mkouba]

Hm, I don't remember why only repeating qualifiers are standardized. It might be the inheritance rules where things can get really complicated. But the same applies to qualifiers. So I really don't know.

I think that a straightforward solution might be to (1) collect all interceptor bindings, (2) collect all container annotations, (3) use an annotation transformer to "extract" container annotation instances (container -> bindings). But we'll need more investigation...

[manovotn]

@mkouba @gwenneg I agree that specs aren't clear about this so I've create this issue in interceptors spec - https://github.com/eclipse-ee4j/interceptor-api/issues/81

[Ladicek]

I started looking into this and got it somewhat working, and then stumbled upon this part of the Interceptors spec:

If the set of interceptor bindings of a component class or interceptor, including bindings inherited from CDI stereotypes [3] and other interceptor bindings, has two instances of a certain interceptor binding type and the instances have different values of some annotation member, the container automatically detects the problem, treats it as a definition error, and causes deployment to fail.

If you interpret this to the letter, this says that repeatable annotations are not permitted (unless their members are @Nonbinding, in which case why bother?). Do I read that correctly? :-)

[gwenneg]

Thanks for taking care of this @Ladicek!

Having two instances of the same interceptor binding with @NonBinding members is exactly what made me create this issue.

Here's my use case:

@InterceptorBinding
@Retention(RetentionPolicy.RUNTIME)
@Target({ ElementType.TYPE, ElementType.METHOD })
@Repeatable(List.class)
public @interface CacheInvalidateAll {

    @Nonbinding
    String cacheName() default "";

    @Retention(RetentionPolicy.RUNTIME)
    @Target({ ElementType.TYPE, ElementType.METHOD })
    @interface List {
        CacheInvalidateAll[] value();
    }
}

@ApplicationScoped
public class MyService {
    @CacheInvalidateAll(cacheName = "cache-A")
    @CacheInvalidateAll(cacheName = "cache-B")
    public void invalidateBothCaches() {
    }
}

There are other ways to achieve the same result (two methods calls instead of one, each one with one binding) but this behavior has actually been supported by the Quarkus cache extension for a while because when the interceptor bindings are added using an AnnotationTransformer, repeating the same binding works with Arc.

I'm trying to get rid of the AnnotationTransformer usage in the extension to fix an open Quarkus issue, but when the bindings are written in the source code and not added with the transformer, Arc does not support repeated bindings.

[gwenneg]

If the repeated interceptor bindings hold different binding members values, I understand the spec like you do, it does not seem to be allowed.

Whether my use case should be allowed or not, I think Arc behavior should be the same if the bindings come from the original source code or from the result of an AnnotationTransformer execution.

[mkouba]

If the set of interceptor bindings of a component class or interceptor, including bindings inherited from CDI stereotypes [3] and other interceptor bindings, has two instances of a certain interceptor binding type and the instances have different values of some annotation member, the container automatically detects the problem, treats it as a definition error, and causes deployment to fail.

If you interpret this to the letter, this says that repeatable annotations are not permitted (unless their members are @Nonbinding, in which case why bother?). Do I read that correctly? :-)

@Ladicek @gwenneg @manovotn Hm, but this part of the spec speaks about the set of interceptor bindings of a component class, i.e. it does not include the interceptor bindings declared on methods. Note that _An interceptor binding declared on a method or constructor replaces an interceptor binding of the same type declared at class level or inherited from a superclass_. So it's OK to have an binding declared on the class level and the same interceptor binding type on the method level.

In fact, the interceptors spec does not reflect JDK8+ features at all (interface default methods is another one...).

We can probably agree that the behavior is not well defined but I'd suggest to try to implement what makes sense for our use case.

[Ladicek]

@mkouba No, I agree, I reproduced the test from Weld (linked above), which only has method-level bindings, and that was rather straightforward to implement. Then I started wondering about class-level bindings and it became interesting :-) I'll tie some loose ends and submit a PR, then we can discuss.

[Ladicek]

OK and of course binding and non-binding annotation attributes make the situation even more interesting :-)

[Ladicek]

Submitted #13553. There's one TODO in the code, related to repeatable class-level interceptor bindings :-), but feedback to anything else (including tests) is of course also welcome.

[mkouba]

Hm, so I've just found: _"An interceptor binding declared on a bean class replaces an interceptor binding of the same type declared by a stereotype that is applied to the bean class."_ in 9.3. Binding an interceptor to a bean and there is even a TCK test: https://github.com/eclipse-ee4j/cdi-tck/blob/2.0.3.Final/impl/src/main/java/org/jboss/cdi/tck/tests/definition/stereotype/interceptor/StereotypeWithMultipleInterceptorBindingsTest.java

@Ladicek @manovotn Looks like a spec conflict, or am I missing something obvious?

[manovotn]

Hmm, you are sort of correct. However, interceptors spec refers to CDI as possibly applying additional rules and this could be just one of them. Furthermore, this still doesn't eliminate duplicate class-level bindings coming from transitive bindings, right? You still have a possible conflict that you need to resolve accordingly to interceptors spec (which reads "blow up").
I'd say this part of CDI spec just clarifies that you don't really need to blow up in this one case because we can deterministically infer which binding to use.

That being said, it'd be nice if interceptors spec didn't specifically mention the stereotype case :-D

If the set of interceptor bindings of a component class or interceptor, including bindings inherited from CDI stereotypes [3] and other interceptor bindings, ...

[Ladicek]

Such fun! If it was up to me, interceptor bindings would have to be declared only on methods, no inheritance whatsoever! :-)

[mkouba]

Well, it's a mess. Interceptor bindings are transitive, stereotype declarations are transitive, ... Anyway this spec rule seems to make sense for all non-repeatable bindings. For repeatable bindings, it should _not_ blow up if declared on the bean class but it probably does not make any sense to support interceptor classes with repeatable bindings. We should probably put together a list of common use cases that should be supported and ignore the unspecified stuff...

[manovotn]

We'd have to just guess those use cases. Maybe we could look into what Weld supports and copy the behavior to be consistent. But I don't know what that is from the top of my head; someone will have to play around with it and check it :-)

[Ladicek]

I'm in favor of looking at (and adding) test cases :-) The Weld ones for repeatable interceptor bindings linked above, unfortunately, only use method-level interceptor bindings. Does perhaps Weld have test cases for repeatable class-level interceptor bindings?

I've added some tests for repeatable interceptor bindings to my PR (https://github.com/quarkusio/quarkus/pull/13553), both method-level, class-level and combination.

I didn't add tests for interceptor bindings coming from multiple sources (inheritance, transitive interceptor bindings, stereotypes, anything else?). I know some tests for this are already there (for transitive interceptor bindings), and they obviously still pass, but we can add more. If you could point me to tests in Weld or CDI TCK, that would be great.

[manovotn]

So I scanned Weld tests and CDI TCKs. To my surprise, there are no tests apart from the one you discovered already.
Which is really weird, I have no idea why we test method-level bindings, but omit class-level ones. Sorry for my previous misleading comment, I really thought there would be more...

I've added some tests for repeatable interceptor bindings to my PR (#13553), both method-level, class-level and combination.

I think this will do ;-)

[Ladicek]

I've just added tests that verify repeatable interceptor bindings inherited from a superclass and from a stereotype to the PR.

I also added a few tests that verify [what I believe is] correct behavior when it comes to transitive stereotypes and stereotype inheritance, but those things are not implemented in ArC yet, so I left those tests as @Disabled.

I'd love to hear everyone's opinion on those tests. And what should we do about that one TODO in the code :-)