jakartaee / cdi

CDI specification
Apache License 2.0
210 stars 78 forks source link

Support for intercepted self invocation #481

Open cen1 opened 3 years ago

cen1 commented 3 years ago

https://issues.redhat.com/browse/CDI-414

To me, this example from JIRA issue perfectly describes what one would want to achieve:

public class TimedMethodBean {

    @Timed(name = "timedMethod")
    public void timedMethod() {
    }

    public void selfInvocationTimedMethod() {
        timedMethod(); //this method call will not be timed :(
    }
}
manovotn commented 3 years ago

I have seen this debated repeatedly with arguments for and against self-interception. There are already conflicting implementations - Weld does not do self-interception (in fact it does quite some hacks to avoid it) while Quarkus does self-interception. Not sure about OWB or what Micronaut impl plans for. So we might have some conflicts and compatibility breakage with what's already out there.

More importantly though - in both of the above projects, I have seen users coming and asking for the exact opposite behavior ;-) So there are users expecting both.

If you go for self interception(1), there is no way a user can prevent it if it is undesirable. If you opt not to have self-interception(2), you can still invoke the method via injected proxy reference which will trigger the interception. I am not sure how to elegantly bypass the issue of (1) unless we'd have some extra annotation (or parameter in binding) but that looks clunky. In case of (2), it has only the downfall that you cannot trigger interception for non-proxied beans, which are effectively dependent and CDI singleton scopes (where singleton can be easily replaced by app scope for this purpose). With that in mind, I'd lean towards option (2). - spec saying that it is not supposed to work out of the box. But like I said, opinions are for both sides and we already have an impl split. So we could also state that this is non-portable which doesn't solve it but makes it understandable to users.

WDYT @mkouba @Ladicek ?

Side note - not sure if this belongs to CDI or interceptors spec?

Ladicek commented 3 years ago

I personally find self-interception nice and expected -- until it isn't :-) But you're right in that both options have enthusiastic proponents and opponents, no idea how to solve that situation.

Whether it belongs to CDI or Interceptors, I have no idea, but I'd lean towards Interceptors. That is, if I understand correctly, the right place where CDI and EJB should collaborate on this particular thing.

graemerocher commented 3 years ago

FWIW Micronaut is self invocation (if that is what it is called) by default, however there is a way to switch to proxying to a target instance

cen1 commented 3 years ago

As a CDI user with almost no perspective on the spec and current implementation behaviours, my thoughts are:

  1. Explicit self injection of a bean just looks like a bad design pattern/hack/workaround/you name it. If spec said that should work it's fine, one more tool in the belt but I wouldn't advocate usage of the pattern.

    public class TimedMethodBean {
    
    @Inject
    private TimedMethodBean self; //just..ugly
    }
  2. To preserve backward compatibility this would have to be configurable. Not sure about all the current implementations but my guess is an opt-in.

  3. Following no. 2 reasoning, some kind of new annotation or param seems inevitable. That way, as a developer I can clearly communicate the intentions of whether I want self-invocation to be used on a particular bean or not.