Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Initial specification text for method invokers #697

Merged
merged 1 commit into from
Jan 29, 2024

Conversation

Ladicek
Copy link
Contributor

@Ladicek Ladicek commented Nov 6, 2023

No description provided.

@Ladicek Ladicek added this to the CDI 4.1 milestone Nov 6, 2023
@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 6, 2023

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.

Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

spec/src/main/asciidoc/core/invokers.asciidoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/core/invokers.asciidoc Show resolved Hide resolved
spec/src/main/asciidoc/core/invokers.asciidoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/core/invokers.asciidoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/core/invokers.asciidoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/core/invokers.asciidoc Outdated Show resolved Hide resolved
@Ladicek
Copy link
Contributor Author

Ladicek commented Nov 14, 2023

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 Ladicek force-pushed the method-invokers-spec branch 2 times, most recently from f9d5ec2 to 3db2e81 Compare December 13, 2023 16:23
@Ladicek
Copy link
Contributor Author

Ladicek commented Dec 13, 2023

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

@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 4, 2024

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
Copy link
Contributor Author

Ladicek commented Jan 5, 2024

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.


If the target method is `static`, the `instance` is ignored; by convention, it should be `null`.
If the target method is not `static` and `instance` is `null`, a `RuntimeException` is thrown.
If the target method is not `static` and the class of the `instance` is not a subclass of the bean class of the target bean, a `RuntimeException` is thrown.
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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...

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Copy link
Contributor Author

Ladicek commented Jan 16, 2024

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
Copy link
Contributor

Azquelt commented Jan 16, 2024

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
Copy link
Contributor Author

Ladicek commented Jan 16, 2024

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.

@Ladicek Ladicek requested a review from manovotn January 22, 2024 11:35
Copy link
Contributor

@manovotn manovotn left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 :)

*
* @since 4.1
*/
public interface InvokerFactory {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

spec/src/main/asciidoc/core/invokers.asciidoc Outdated Show resolved Hide resolved
spec/src/main/asciidoc/core/invokers.asciidoc Outdated Show resolved Hide resolved
@Ladicek
Copy link
Contributor Author

Ladicek commented Jan 29, 2024

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.

@Ladicek Ladicek merged commit ec24b34 into jakartaee:main Jan 29, 2024
5 checks passed
@Ladicek Ladicek deleted the method-invokers-spec branch January 29, 2024 16:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants