Initial specification text for method invokers

[Ladicek]

No description provided.

[Ladicek]

This is an initial specification text for the Invoker part of method invokers. It doesn't include anything about InvokerBuilder except of the build() method. Lookups, transformers and wrappers are for later (I've recently started thinking that it might indeed be best to postpone some of these features to a later release, because there has been little practical experience with them, just my prototypes).

I also started writing TCK tests for this, but I need a few more days as I also need to figure out if some of the requirements stated here are straightforward to implement with Weld. Specifically, the rules about throwing exceptions in certain situations may need to be relaxed. I hope I'll be able to submit a PR to the TCK repo in a few days.

[manovotn]

Overall looks good, added few comments.
I'd also be +1 for adding docs for transformations/wrapper in this PR as well.

[Ladicek]

FYI, I submitted jakartaee/cdi-tck#502 with initial TCK tests that correspond to this PR in its current form. It is passing on my ArC implementation of method invokers (with a few fixes outside of the method invokers code, I'll probably submit a PR for those separately), and mostly passes on Weld (with a few modifications that I submitted a PR for).

[Ladicek]

Rebased and added one commit (to be squashed) that attempts to resolve most of the open questions here. Comments welcome.

[Ladicek]

Rebased and added one commit (to be squashed) that avoids the entire section on assignability for invokers. Turns out that the JLS already defines the exact rules we need (and both reflection and method handles reference those rules as well, under the name of "method invocation conversions").

[Ladicek]

Added one more commit (to be squashed) that:

1. lets Invoker.invoke() rethrow target method exceptions directly, instead of wrapping.
2. Uses InvokerFactory instead of BeanInfo.createInvoker().

This should be ready now. I also have the necessary TCK changes ready.

[Ladicek]

the class of the instance is not a subclass of the bean class of the target bean

We probably need to relax this somehow, because contextual references (and therefore injectable references) don't necessarily need to satisfy this constraint. Not yet sure...

[Ladicek]

I tried to clarify the requirements on the target instance in the last commit, but it probably needs more thinking.

I was originally convinced it should be enough to demand that the target instance is an instance of the bean class, but there are rules (https://jakarta.ee/specifications/cdi/4.0/jakarta-cdi-spec-4.0.html#typecasting_between_bean_types) that make this more complex (I vaguely recall something to do with EJBs too).

[Ladicek]

I think I figured this out, added one more commit. The gist is:

We already define that the target method must be declared by the bean class of the target bean, or inherited from a supertype. That remains, and failure is still a deployment problem (basically a bean/method mismatch). We however also define that the target method should be declared by a type that is present in the set of bean types of the target bean. If not, it is a non-portable behavior, since some implementations can support it. (As an aside, I also made it non-portable to register an invoker for a final method. Again, some implementations can support it.)

This lets us mandate minimum requirements on the target instance in case it is a contextual reference for the target bean (likely the most common case), in line with the rest of the specification. If the target method is declared on an interface, the contextual reference can always be cast to it, otherwise the contextual reference must be for the type that declares the target method. For a non-contextual instance, the class simply must declare the method or inherit from a supertype. Implementations may support more.

Took me a while to figure out, but I feel pretty happy about it.

[Ladicek]

As an aside, I also made it non-portable to register an invoker for a final method. Again, some implementations can support it.

Added one more commit where I expanded this to all target methods declared on unproxyable bean types.

My reasoning was that if a bean type is unproxyable, it does not need to be possible to obtain a contextual reference for a bean for that bean type, and invoking a method through a contextual reference is likely to be the most common case, so it's better to be open about it.

I however realize that this might be too broad. There are 3 cases when CDI cares about proxyability:

• normal scoped beans
• beans with bound interceptors
• beans with bound decorators

In the 1st case, CDI mandates that all bean types for which a contextual reference is ever obtained are proxyable. In the 2nd and 3rd case, CDI mandates just the bean class to be a proxyable type.

I hope we can tighten the requirement here somehow, but I'm not sure how much.

[Ladicek]

I narrowed this down slightly to just non-static methods, and only to normal scoped beans in case of unproxyable bean types.

In theory, we wouldn't even have to mention non-portable behavior here, because the inability to obtain a contextual reference for an unproxyable bean type or for a type that is not in the set of bean types follows from other parts of the specification. I have however in the meantime started working on specification text for lookups and that would require the same wording anyway.

[Ladicek]

Squashed all commits, I believe this is now ready to merge.

I started working on specification text for lookups, which I'll submit as another PR.

[Azquelt]

It looks good to me!

I had a couple of questions:

1. Am I correct in my reading that there's no requirement that the method belongs to any of the bean types of the bean, as long as it is non-private and declared on the bean class or one of its supertypes?
   • However, if a contextual reference is passed to invoke, then the method must be on one of the bean types
2. I don't see any technical reason that method invokers couldn't be extended to beans other than managed beans. If we wanted to extend this functionality to other beans in a future release, I think we could do that without any breaking changes?
   • some parts of the spec would have to be loosened (e.g. requiring that the method is either on the bean class of a managed bean or on one of the bean types) and some parts would not apply to non-managed beans (e.g. invoking methods on non-contextual instances)
   • at the moment I don't see any need to do this, I'm just trying to think through whether we've painted ourselves into a corner if someone were to come up with a good use-case in the future.

[Ladicek]

Am I correct in my reading that there's no requirement that the method belongs to any of the bean types of the bean, as long as it is non-private and declared on the bean class or one of its supertypes?

Right. If you pass a non-contextual instance to the invoker, that is still supposed to work. However, you will generally not be able to obtain a contextual reference for a type that is not in the set of bean types (albeit some implementations support it), so creating an invoker for such method is non-portable.

I don't see any technical reason that method invokers couldn't be extended to beans other than managed beans.

Agree.

[manovotn]

Looks good; especially the spec text is pretty impressive in how accurate it is while still remaining readable!
I left some two minor comments and a grumble about InvokerFactory, but I understand I got outvoted there :)

[manovotn]

I have to say I am not a big fan of this.
You still need BeanInfo as an argument, so this just adds an extra step along the way where your @Registration method needs an extra argument.
Technically, you could also attempt to pass in some custom impl of BeanInfo which I don't suppose we want users to try.

[Ladicek]

Given that a @Registration method must declare exactly one parameter of type BeanInfo or InterceptorInfo or ObserverInfo, I considered specifying that an InvokerFactory parameter may be declared on a @Registration method that declares a parameter of type BeanInfo and that BeanInfo is the target bean of the invokers created by the factory, and then InvokerFactory.createInvoker() would only have a single parameter of type MethodInfo. In the end, it felt a little too magical to me.

The main reason why I don't want BeanInfo.createInvoker() is that BeanInfo (like all other *Info types) is a "passive" object that only hands out information. It shouldn't have any "active" methods that configure behavior.

An alternative would be something like

/**
 * Allows configuring a bean.
 *
 * @see Registration
 * @since 4.1
 */
public interface BeanConfig {
    /**
     * Returns the {@link BeanInfo} corresponding to this configured bean.
     *
     * @return the {@link BeanInfo} corresponding to this configured bean, never {@code null}
     */
    BeanInfo info();

    /**
     * Returns a new {@link InvokerBuilder} for given method of this bean. The builder
     * eventually produces an opaque representation of the generated invoker.
     * <p>
     * If an invoker may not be built for this bean or for given {@code method},
     * an exception is thrown.
     *
     * @param method target method of the invoker, must not be {@code null}
     * @return the invoker builder, never {@code null}
     */
    InvokerBuilder<InvokerInfo> createInvoker(MethodInfo targetMethod);
}

I'm a little hesitant about this, because it would also be a natural place for other bean configurations (bean attribute transformations, injection point transformations) and I have doubts we can do that in the @Registration phase, as currently specified.

[manovotn]

The main reason why I don't want BeanInfo.createInvoker() is that BeanInfo (like all other *Info types) is a "passive" object that only hands out information. It shouldn't have any "active" methods that configure behavior.

I understand and I agree that BeanInfo is a "final bean state" where you can read its information. To me, that's precisely why I think the method should be there; after all, you are not configuring bean metadata anymore. Instead, you are retrieving a handle to a bean method in its final state.

Like I said, I can accept being outvoted.
If this is the preferred version, I'll learn to live with it, hard as it may be ;-)

I'm a little hesitant about this, because it would also be a natural place for other bean configurations (bean attribute transformations, injection point transformations) and I have doubts we can do that in the @Registration phase, as currently specified.

I fully concur, let's not do that.

[Ladicek]

I'm going to merge this, because I'd really like to submit a PR for configuring lookups.

That doesn't mean that I'm overruling @manovotn's concern about InvokerFactory -- feel free to continue nagging me about that. Maybe there's another solution that I didn't think of.