WICKET-7126 simplify CdiConfiguration + BeanManagerLookup

[pedrosans]

This change cleans up CdiConfiguration and BeanManagerLookup by removing some logic and state related to fallbackBeanManager and lastSuccessful strategy, while removing none of the functionality. For instance, it's still possible to have a fallback BeanManager as a config option if the current lookup fail:

bm = BeanManagerLookup.lookup();
if (bm == null)
bm = customBeamManager;
new CdiConfiguration(bm);

Another benefit of this change is the reduced number of times the new code will make expensive (time consuming) JNDI lookups.

[pedrosans]

build fails with:

Error: Failed to execute goal com.github.siom79.japicmp:japicmp-maven-plugin:0.25.4:cmp (default) on project wicket-cdi: There is at least one incompatibility: org.apache.wicket.cdi.CdiConfiguration.getFallbackBeanManager():METHOD_REMOVED,org.apache.wicket.cdi.CdiConfiguration.setFallbackBeanManager(jakarta.enterprise.inject.spi.BeanManager):METHOD_REMOVED -> [Help 1]

but the current version (11.0.0-SNAPSHOT) has no previous 11.0.x one to be incompatible. Let me know if it's the case to better setup the japicmp tool or to ignore (if possible) the fail.

[martin-g]

The plugin should be disabled in master branch until 11.0.0 is released.

[martin-g]

Another benefit of this change is the reduced number of times the new code will make expensive (time consuming) JNDI lookups.

Is it really ?
Before there was a BeanManagerLookup#lastSuccessful cache. Now it is gone and every BeanManagerLookup#lookup() always makes two JNDI lookups.

[martin-g]

The error message is vague and does not suggest actionable guidance for the developers.

[pedrosans]

Because the message is inside CdiConfiguration#getBeanManager, I found it specific. Maybe to add actionable guidance would be enough: "Be sure to call CdiConfiguration#configure and to specify a BeanManager to CdiConfiguration or that one can be resolved by BeanManagerLookup."

[martin-g]

Why is this constructor removed ?
This is a utility class with a single static method.

[pedrosans]

My bad. Didn't noticed the constructor was private and would rather remove those three lines in benefit of static#lookup going up some lines to get a better highlight.

[martin-g]

Maybe add a non-null check ?!
Otherwise, it behaves as the no-arg constructor if null is passed.

[pedrosans]

My bad. Indeed it would:

new CdiConfiguration(null)

could also end up being a object with a not null beanManager. I meant to add the non-null check but forgot.

[martin-g]

What is the benefit here of returning null than throwing an exception ?

[pedrosans]

To maintain the fallbackBeanManager logic without a exception try/catch hassle. Right now the user can:

bm = BeanManagerLookup.lookup();
if (bm == null)
  bm = fallbackBeamManager;
new CdiConfiguration(bm);

provide their own fallback option if the lookup fail. Conceptually, the new BeanManagerLookup is just a lookup and not the class encapsulating the entire BeanManager resolution code as previously done. Being so I found it ok to return null. I can change the javadoc to better explain the new type role.

[martin-g]

Suggested change

	if(beanManager == null)
	beanManager = BeanManagerLookup.lookup();
	if (beanManager == null)
	{
	beanManager = BeanManagerLookup.lookup();
	}

[martin-g]

Suggested change

	if (beanManager == null)
	throw new IllegalStateException(
	"No BeanManager was set or found via the CDI provider. Check your CDI setup or specify a BeanManager in the CdiConfiguration.");
	if (beanManager == null)
	{
	throw new IllegalStateException(
	"No BeanManager was set or found via the CDI provider. Check your CDI setup or specify a BeanManager in the CdiConfiguration.");
	}

[pedrosans]

Another benefit of this change is the reduced number of times the new code will make expensive (time consuming) JNDI lookups.

Is it really ?
Before there was a BeanManagerLookup#lastSuccessful cache. Now it is gone and every BeanManagerLookup#lookup() always makes two JNDI lookups.

There are no code inside the module calling BeanManagerLookup lookup more than once (CdiConfiguration#configure is meant to be called once), contrary to the previous implementation. NonContextual called BeanManagerLookup during the request response, so the number of JNDI lookups was tied to the number of requests.