Consider adding support of InterceptionFactory

[mkouba]

https://docs.jboss.org/cdi/spec/2.0/cdi-spec.html#interception_factory

The problematic part is InterceptionFactory#configure().

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you!
We are doing this automatically to ensure out-of-date issues does not stay around indefinitely.
If you believe this issue is still relevant please put a comment on it on why and if it truly needs to stay request or add 'pinned' label.

[mkouba]

The problematic part is InterceptionFactory#configure().

We could just ignore this method and throw a runtime exception if called at runtime...

[manovotn]

Giving this some more thought:

• We probably want to ignore BeanManager.createInterceptionFactory because this can be called anytime in runtime (and is mostly used by runtime extensions anyway)
• With producer methods, even though there is an injection point of type InterceptionFactory, we have no way of knowing the interceptor bindings user wants to add unless they call .configure() but that is too late for us to pre-generate the subclass at build time
  • Adding an annotation (binding) to the method doesn't help either because this would cause weird behaviour with interceptors on the method itself plus it is non-CDI approach
  • Adding any other kind of annotation is already a non-CDI approach requiring user to fall back to quarkus-specific stuff
• Alternatively, we can instead look into implementing this for extensions and leave out producers
  • With extensions we can be sure the user provides full information before we get to generate the subclass
  • However, this is fully out of CDI specification as quarkus uses custom extension model

[mkouba]

Giving this some more thought:

* We probably want to ignore `BeanManager.createInterceptionFactory` because this can be called anytime in runtime (and is mostly used by runtime extensions anyway)

+1

* With producer methods, even though there is an injection point of type `InterceptionFactory`, we have no way of knowing the interceptor bindings user wants to add unless they call `.configure()` but that is too late for us to pre-generate the subclass at build time

Yes, we can't do that but there are use cases where configuration is not needed, i.e. the class of the produced bean declares interceptor bindings. We could try to introduce a non-portable declarative approach, e.g. something like:

// "to" is a reg ex to match the method name...
@AssociateWithMethod(binding = Transactional.class, to = "(add|remove)")
@AssociateWithClass(binding = Logged.class)
@Produces
List<String> myListProducer() {
 // ...
}

  * Adding an annotation (binding) to the method doesn't help either because this would cause weird behaviour with interceptors on the method itself plus it is non-CDI approach

I agree that this is not a good idea...

  * Adding any other kind of annotation is already a non-CDI approach requiring user to fall back to quarkus-specific stuff

Yes, it wouldn't be portable but at least "doable" ;-)

* Alternatively, we can instead look into implementing this for extensions and leave out producers
  
  * With extensions we can be sure the user provides full information before we get to generate the subclass
  * However, this is fully out of CDI specification as quarkus uses custom extension model

[mkouba]

AssociateWithClass(binding = Logged.class) is actually too restrictive because many bindings declare members. A user would have to define a special annotation, e.g. something like:

@Singleton
class SimpleProducer {

    @BindTxToGet(@Transactional(TxType.REQUIRED))
    @Produces
    List<String> interceptedList(InterceptionFactory<List<String>> factory) {
       return factory.createInterceptedInstance(List.of());
    }

   @InterceptionFactoryBinding("get")
   @Retention(RUNTIME)
   public @interface BindTxToGet {
     Transactional value();
   }
}

// metaannotation used to identify the binding annotations
@Target(ElementType.ANNOTATION_TYPE)
@Retention(RUNTIME)
public @interface InterceptionFactoryBinding {
   // regex to match methods
   String value() default "*";
}

Which is far from practical.

[plopezm]

Is there any solution to this issue? A workaround is very welcome. Thanks

[manovotn]

No solution or workaround ATM. I don't think we came up with a viable way to do this.

This feature is very much runtime oriented (as dictated by spec) and anything we could do would be Quarkus-specific as we'd need an upfront information on what classes to generate at build time.
The closest thing is what Martin suggested in #3532 (comment) but it isn't very nice and doesn't have the full scale of operations that InterceptionFactory has either.

[Ladicek]

Copying my comment from #34436, because it really belongs here.

I was thinking we could maybe support something like this:

@Produces
@RequestScoped
@SupportInterceptionOf(MyClass.class) // name subject to bike shedding of course
public MyClass produceMyClass(InterceptionFactory factory) {
    return factory.createInterceptedInstance(new MyClass());
}

That would mean:

1. We'd pregenerate subclasses for the classes given in the @SupportInterceptionOf annotation.
2. If the InterceptionFactory receives an object whose class is present in the list given by @SupportInterceptionOf, it would instantiate the subclass with the given object as a delegate. So it wouldn't be like our current subclasses, where we delegate to the superclass, we'd have to delegate to another object, but I believe it's the exact same with regular InterceptionFactory.
3. If the InterceptionFactory receives an object whose class is not present in the list given by @SupportInterceptionOf, it would throw an exception.

Alternatively, we could ditch CDI's InterceptionFactory and use something of our own that would work similarly to managed beans (that is, delegation to superclass instead of a second instance). Something like:

@Produces
@RequestScoped
@SupportInterceptionOf(MyClass.class) // name subject to bike shedding of course
public MyClass produceMyClass(ArcInterceptedBeanFactory factory) { // name intentionally ugly
     // could also accept arguments to pass to the constructor, probably?
    return factory.createInterceptedInstance(MyClass.class);
}

Looking at the InterceptionFactory API again, we wouldn't be able to support configure() and ignoreFinalMethods() anyway -- createInterceptedInstance() is the only thing we could possibly do. So creating our own API is probably a good idea regardless of the "interception style".

[Ladicek]

Now, the comments in this issue also got me thinking about interceptor bindings. Clearly we can't support InterceptionFactory.configure(), so my previous comment would only make sense in case the target class (MyClass) already has interceptor bindings.

If it doesn't, I'd suggest we just use "annotation mixins". Say for example MyClass looks like this and I can't modify it:

public class MyClass {
    public int doSomething() { ... }
}

Now, if I want to create a producer bean for this class, and make the doSomething method @Transactional, I'd first create a mixin class:

public class MyClassInterceptorBindings {
    @Transactional
    public int doSomething() {
        return 0; // doesn't matter, this method is never executed
    }
}

Then, I create the producer, on which I declare that a return value of MyClass should be intercepted based on interceptor bindings in the mixin class:

@Produces
@RequestScoped
@SupportInterceptionOf(value = MyClass.class, bindingsFrom = MyClassInterceptorBindings.class)
public MyClass produceMyClass(InterceptionFactory factory) {
    return factory.createInterceptedInstance(new MyClass());
}

The meaning would be as follows:

1. SupportInterceptionOf has 2 members, Class<?> value() which is mandatory, and Class<?> bindingsFrom() default Object.class. It is @Repeatable to allow producer methods return instances of multiple classes.
2. When @SupportInterceptionOf occurs, we would pre-generate a subclass for the class given by the value member. We would gather interceptor bindings from the value class and from the bindingsFrom class, where interceptor bindings on methods in the bindingsFrom class are treated as if they were present on the corresponding methods from the value class. (A class-level binding on the bindingsFrom class would be treated as a class-level binding on the value class, obviously.) We'd have to define what happens when both classes have interceptor binding annotations, but that's a detail at this point.
3. When InterceptionFactory.createInterceptedInstance() is called, it must receive an object whose class is precisely one of the classes defined by @SupportInterceptionOf annotations. If not, an exception is thrown. If yes, we'd instantiate the pre-generated subclass.
4. If we used CDI's InterceptionFactory, we'd have to resort to interception by forwarding to a delegate instance. If we wanted interception by invoking superclass, we'd have to use a different API (like I sketched in previous comment).

WDYT?

[mkouba]

We'd pregenerate subclasses for the classes given in the @SupportInterceptionOf annotation.

Hm, why do we need the @SupportInterceptionOf annotation? We could just pregenerate subclasses for all return types of a producer method that accepts InterceptionFactory as a param, or?

If it doesn't, I'd suggest we just use "annotation mixins".

I'm not sure about this one. Wouldn't the aforementioned @AssociateWithMethod(binding = Transactional.class, to = "(add|remove)") and @AssociateWithClass(binding = Logged.class) be more practical?

[manovotn]

That's an interesting idea and it could be used for synth beans as well.

I'd probably steer away from using InterceptionFactory as that offers methods we cannot support anyway, we'd need to have Arc's custom variant.

Note that the mixin class should probably extend the target class so that we avoid some validation hell in terms of matching methods? The target class should be non-final anyway as we need to subclass it.

I'm not sure about this one. Wouldn't the aforementioned @AssociateWithMethod(binding = Transactional.class, to = "(add|remove)") and @AssociateWithClass(binding = Logged.class) be more practical?

@mkouba IIUIC, this still won't allow you to declare annotation values whereas Ladislav's approach will.

[Ladicek]

We'd pregenerate subclasses for the classes given in the @SupportInterceptionOf annotation.

Hm, why do we need the @SupportInterceptionOf annotation? We could just pregenerate subclasses for all return types of a producer method that accepts InterceptionFactory as a param, or?

That would get out of hands fairly quickly with interfaces like, I don't know, List? :-)

That's an interesting idea and it could be used for synth beans as well.

Great point!

[mkouba]

@mkouba IIUIC, this still won't allow you to declare annotation values whereas Ladislav's approach will.

@manovotn Not really. Binding does not have to a class but annotation instead...

That would get out of hands fairly quickly with interfaces like, I don't know, List? :-)

@Ladicek well, the value of to is a regex sooo... but the MyClassInterceptorBindings approach seems fragile/hard-to-sync to me. Also it will be a large class for the List too, or? ;-)

[manovotn]

@manovotn Not really. Binding does not have to a class but annotation instead...

Not sure I understand. I was talking about trying to declare an interceptor binding that would be something like @MyBinding("foo") which doesn't seem possible with your annotation-only approach?

[Ladicek]

@Ladicek well, the value of to is a regex sooo... but the MyClassInterceptorBindings approach seems fragile/hard-to-sync to me. Also it will be a large class for the List too, or? ;-)

I'm not sure what to you mean, but my proposal is that @SupportInterceptionFor(value = MyClass.class, MyClassBindings.class) means:

1. The InterceptionFactory must receive an object of class exactly MyClass and no other class.
2. MyClassBindings must only include methods that need an interceptor binding annotation. If you have a class with 50 methods and only 1 needs an interceptor binding, the mixin class can just declare that one method.

[Ladicek]

And, more generally, the annotation mixin approach has received a lot of criticism elsewhere, for good reasons. I still prefer it over other approaches because it just clicks with my brain. I know my brain is not typical 😆

[mkouba]

I'm not sure what to you mean,

It means add the binding to all methods with the name that matches the regex in the to attribute.

If you have a class with 50 methods and only 1 needs an interceptor binding, the mixin class can just declare that one method..

For sure, but if you need to add the binding to 40 methods?

[Ladicek]

Ah I see. Well, your approach only works for memberless annotations. If we want to support bindings with members, we need something more powerful. I think we could mix the two, actually:

class MyClassBindings {
    @ApplyToMethods("add|remove|doSomething")
    @Transactional
    void whatever() {
    }
}

Or something like that.

[mkouba]

@manovotn Not really. Binding does not have to a class but annotation instead...

Not sure I understand. I was talking about trying to declare an interceptor binding that would be something like @MyBinding("foo") which doesn't seem possible with your annotation-only approach?

You're right, we would need a special annotation for each binding...

[mkouba]

Ah I see. Well, your approach only works for memberless annotations. If we want to support bindings with members, we need something more powerful. I think we could mix the two, actually:

class MyClassBindings {
    @ApplyToMethods("add|remove|doSomething")
    @Transactional
    void whatever() {
    }
}

Or something like that.

Or even meta-annotations! 🙈

@Retention(RUNTIME)
@Target(ElementType.ANNOTATION_TYPE)
public @interface Association { }

@Association
@Retention(RUNTIME)
public @interface AssociateTransactional {   
    Transactional binding();
    String to();
}

and usage:

@Produces
@AssociateTransactional(binding = @Transactional(TxType.REQUIRES_NEW), to = "foo")
public MyClass produceMyClass(InterceptionFactory factory) {
    return factory.createInterceptedInstance(new MyClass());
}

[manovotn]

For sure, but if you need to add the binding to 40 methods?

If you need to intercept all methods, class level annotation is there.
If you just need lots of methods in a class, well, you're right there but it's still a one-time setup to perform.
If you have hundreds of beans that need to perform^^, you're probably doing in wrong anyway :D

I think we could mix the two, actually:

Hm, possible but quite confusing since you are now using some methods to map it 1:1 and some dummy methods as templates to apply to multiple other methods...

[Ladicek]

Yeah, I wouldn't exactly be a fan of @ApplyToMethods (not even speaking about @AssociateTransactional!), but with the genie out of the bottle, there's a plenty of options. (I like direct annotation mixin classes, but I don't insist on them.)

[mkouba]

If you just need lots of methods in a class, well, you're right there but it's still a one-time setup to perform.

@manovotn Unless you modify the original class which is something that does not happen very often, right? 😄

If you have hundreds of beans that need to perform^^, you're probably doing in wrong anyway :D

Very true.

[slinstaedt]

Would be great, if the solution could also work for non-managed pojos, one does not have control over their lifecycle.

Type and interceptors are known in advance, it just happens that they are created and destroyed on demand programmatically. So I would suggest we rather annotate a InterceptionFactory<MyClass> instance that is creating these proxies, than a producer method, that is limiting ourselves to intercepting managed beans.

This way we also easily distinguish between interceptors for bean creation and interceptors for bean method invocation, cause otherwise the producer method is potential overloaded with annotations.

[slinstaedt]

I thought about something like:

@ApplicationScoped
class MyFactory {

  @Inject
  @LogInvocations(LogLevel.INFO)
  @BindInterceptors(mode=mode.EXCLUDE, interceptors={LogInvocations.class}, methods={"method1", "method2"})
  private InterceptionFactory<MyClass> factory;

  @Inject
  private MyClassRestClient client;

  public MyClass create(String key) {
    return factory.createInterceptedInstance(client.create(key));
  }
}

Some notes:

• InterceptionFactory<X> is not from CDI, but a quarkus defined one for reasons discussed above
• @BindInterceptors is obviously repeatable
• @BindInterceptors(mode) probably might has INCLUDE as default
• @BindInterceptors(interceptors) probably might has an empty array as default, meaning it applies to all defined interceptors of the annotated InterceptionFactory
• If no specific includes/excludes are defined for an interceptor, it applies to all methods of MyClass
• We can reuse existing interceptor binding annotations without defining synthetic ones like @AssociateTransactional. I am aware that current interceptor's binding annotations probably don't have @target(FIELD) or @target(PARAMETER), but at least the custom ones (not defined by some spec) could by easily changed. For annotations defined by some spec like @Transactional, quarkus might introduce it's own extended version, that might additionally be used for this specific use case.
• Including/excluding interception of methods on a per-string basis does not cover overloaded methods :/

Open question regards the scope of interception:

1. Does interception only apply to public methods, or also protected/package-private? The later ones are supported for classical CDI beans afaik. But MyClass could be an interface or the provided instance for being intercepted a subclass. At least we need to make sure, users are aware of these interceptors only apply to the defined type at maximum, not the intercepted instance.

[Ladicek]

This was fixed like this:

1. For each producer method that injects InterceptionProxy (or synthetic bean that calls injectInterceptionProxy()), we generate a subclass of the return type of the producer method (or of the impl class of the synthetic bean). This accepts a delegate instance to which the invocation will be forwarded after all interceptors run.
2. In case the user needs to define (or override) interceptor bindings, they have to annotate the InterceptionProxy parameter of the producer method with @BindingsSource (or define the bindings source class in the call to injectInterceptionProxy()). The bindings source class copies the structure of the target class and defines interceptor bindings (in other words, it's an annotation mixin class as mentioned above in this issue).

This is documented in more detail in https://quarkus.io/version/main/guides/cdi-reference (not yet, as the PR was just merged; hopefully tomorrow?). Looking forward for feedback of any users!

[zhfeng]

@Ladicek Nice work!

Just wonder if it can support @Decorator as well? like https://quarkus.io/version/main/guides/cdi#decorators

[Ladicek]

That would in theory be possible, but I have no idea how much work that would entail. There's a feature request in CDI for a standardized solution, but there's been some reluctance from the implementation side.

Do you have a concrete usecase?

[zhfeng]

Hmm, I have a quarkus-pooled-jms extension which can wrap a ConnectionFactory.

And for quarkus-qpid-jms, there is an issue to add the pooling capability provied by quarkus-pooled-jms. ConnectionFactoryWrapperBuildItem had been introduced to solve the similar issue around interception. You can check all the discusses before.

Then if we can support @Decorator, there will be a much elegant solution from quarkus-pooled-jms and I think ConnectionFactoryWrapperBuildItem can be removed.