Arc - Revisit BeanContainer API

[manovotn]

Fixes #35744

A draft of changes for BeanContainer API - some of these are breaking in behavior although they were already dubious in how they worked because we had conflicting javadoc and implementation.

The BeanContainer#beanInstanceFactory method stays in terms of what it does and it now has a javadoc documenting this behavior (i.e. no longer says it can return null). There is also a new variant allowing users to specify a custom fallback Factory implementation that can be used to perform arbitrary fallack in case the bean isn't present.

The BeanContainer#beanInstance method which is used in our own extensions remains in place but also changes in that it can no longer return null - instead it is a proper CDI lookup dealing with ambiguous and unsatisfied dependencies. Extensions looking to implement fallbacks in case the beans are missing can leverage the beanInstanceFactory method, or just access ArcContainer and query for a bean via dynamic resolution.

Last but not least, formerly deprecated methods (deprecated since version 2.14) are now deleted.

[quarkus-bot]

/cc @Sanne (hibernate-orm), @brunobat (opentelemetry), @ebullient (metrics), @gsmet (hibernate-orm), @jmartisk (metrics), @radcortez (opentelemetry), @yrodiere (hibernate-orm)

[manovotn]

/cc @Sanne (hibernate-orm), @brunobat (opentelemetry), @ebullient (metrics), @gsmet (hibernate-orm), @jmartisk (metrics), @radcortez (opentelemetry), @yrodiere (hibernate-orm)

Sorry for the label explosion, the PR changes those extensions only in terms of replacing now-deprecated Arc API call(s).
That is, unless the CI proves me otherwise :)

[manovotn]

I've reworked the PR after discussion with Martin over zulip - it avoid creating more deprecations and instead changes currently used methods. We would cause breaking changes anyway and this way it is less intrusive as most extensions (including our own in core repo) should "just work" with this approach as well.
I've updated the first initial comment of this PR to reflect what code changes are being done.

[gsmet]

as most extensions (including our own in core repo) should "just work" with this approach as well

Famous last words :]

[manovotn]

as most extensions (including our own in core repo) should "just work" with this approach as well

Famous last words :]

Indeed :-D
I mean I did expect some failures; will take a look tomorrow.

[manovotn]

Alright, it turns out the default behavior - fallback to instantiation via no-args ctor - is way more common than I anticipated.
Especially weird since the javadoc of related methods indicated different behavior and the ability to return null which basically never happened :)
I have verified that at least Undertow, Websockets and RR are all making use of this fallback.

With that in mind, we might instead consider changing the javadoc, keeping the default instantiation and the current method (I assume more extensions out in the wild might use that) and introducing a new method, that will use a custom Factory in case we cannot find the bean.
Something like this:

    /**
     * Returns an instance factory for given bean type and qualifiers.
     * <p/>
     * This method performs CDI ambiguous dependency resolution and throws and exception if there are two or more beans
     * with given type and qualifiers.
     * <p/>
     * If no matching bean is found, delegates all calls to the supplied factory fallback.
     *
     * @param fallbackSupplier supplier to delegate to if there is no bean
     * @param type bean type
     * @param qualifiers bean qualifiers
     * @return a bean instance factory, never {@code null}
     */
    <T> Factory<T> beanInstanceFactory(Supplier<Factory<T>> fallbackSupplier, Class<T> type,
            Annotation... qualifiers);

[manovotn]

I've updated the PR as per my suggestion above, let's see what CI thinks about it now :)

[manovotn]

Also, in RR there is a BeanFactory implementation using this API which throws somewhat suspicious exception.
See https://github.com/quarkusio/quarkus/blob/main/extensions/resteasy-reactive/quarkus-resteasy-reactive-common/runtime/src/main/java/io/quarkus/resteasy/reactive/common/runtime/ArcBeanFactory.java#L40-L43

I am not sure what was the original intention of this but right now the if clause catches an exception thrown by DefaultInstanceFactory trying to instantiate a non-CDI object, telling users that their class should be a CDI bean. However, if the fallback DefaultInstanceFactory kicks in and succeeds in creating the object (i.e. no-args ctor exists and is accessible), that's perfectly fine even though it isn't an actual CDI bean? Maybe I am just missing something...
CC @geoand

[geoand]

I am not sure what was the original intention of this but right now the if clause catches an exception thrown by DefaultInstanceFactory trying to instantiate a non-CDI object, telling users that their class should be a CDI bean

This was introduced in #19829 with the intent of making the error message better (described in #19750).

[quarkus-bot]

✔️ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.