Skip to content

WICKET-7126 simplify CdiConfiguration + BeanManagerLookup #1369

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.

Sign up for GitHub

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

Open
pedrosans wants to merge 3 commits into
base: master
Choose a base branch
from
Open

WICKET-7126 simplify CdiConfiguration + BeanManagerLookup #1369

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
c250a0f
WICKET-7126 simplify CdiConfiguration + BeanManagerLookup
pedrosans Feb 15, 2026
a332335
WICKET-7126 javadoc + exception messages + code cleanup
pedrosans Feb 16, 2026
bcac02e
Configure japicmp to compare againt Wicket 11
pedrosans Feb 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -820,7 +820,7 @@
<dependency>
<groupId>${project.groupId}</groupId>
<artifactId>${project.artifactId}</artifactId>
<version>10.0.0</version>
<version>11.0.0</version>
<type>jar</type>
</dependency>
</oldVersion>
Expand Down
2 changes: 1 addition & 1 deletion wicket-cdi-tests/src/test/java/org/apache/wicket/cdi/AlternativeCdiConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ void testApplicationScope()
@Test
void testUsesCdiJUnitConfiguration()
{
configure(new CdiConfiguration().setBeanManager(beanManager));
configure(new CdiConfiguration(beanManager));
tester.startPage(TestPage.class);
tester.assertLabel("appscope", "Alternative ok");
}
Expand Down
14 changes: 1 addition & 13 deletions wicket-cdi-tests/src/test/java/org/apache/wicket/cdi/CdiConfigurationTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@

import jakarta.enterprise.inject.spi.BeanManager;
import jakarta.inject.Inject;
import org.apache.wicket.Application;
import org.apache.wicket.cdi.testapp.ModelWithInjectedDependency;
import org.apache.wicket.cdi.testapp.TestConversationPage;
import org.apache.wicket.cdi.testapp.TestPage;
Expand Down Expand Up @@ -46,7 +45,7 @@ void testApplicationScope()
@Test
void testUsesCdiJUnitConfiguration()
{
configure(new CdiConfiguration().setBeanManager(beanManager));
configure(new CdiConfiguration(beanManager));
tester.startPage(TestPage.class);
tester.assertLabel("appscope", "Test ok");
}
Expand All @@ -72,17 +71,6 @@ void testNotConfigured()

}

@Test
void testAlreadyConfigured()
{
configure(new CdiConfiguration());

assertThrows(IllegalStateException.class, () -> {
CdiConfiguration.get(Application.get()).setBeanManager(beanManager);
});

}

@Test
void testConfigureTwice()
{
Expand Down
10 changes: 3 additions & 7 deletions wicket-cdi-tests/src/test/java/org/apache/wicket/cdi/WicketCdiTestCase.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,9 @@
*/
package org.apache.wicket.cdi;

import io.github.cdiunit.AdditionalClasses;
import io.github.cdiunit.junit5.CdiJUnit5Extension;
import jakarta.inject.Inject;
import org.apache.wicket.Component;
import org.apache.wicket.Page;
import org.apache.wicket.ThreadContext;
Expand All @@ -30,10 +33,6 @@
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.extension.ExtendWith;

import io.github.cdiunit.AdditionalClasses;
import io.github.cdiunit.junit5.CdiJUnit5Extension;
import jakarta.inject.Inject;

/**
* @author jsarman
*/
Expand Down Expand Up @@ -70,9 +69,6 @@ public void end()
contextManager.destroy();
}
tester.destroy();

// make sure no leaked BeanManager are present
BeanManagerLookup.detach();
}

@BeforeEach
Expand Down
54 changes: 10 additions & 44 deletions wicket-cdi/src/main/java/org/apache/wicket/cdi/BeanManagerLookup.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -18,25 +18,24 @@

import jakarta.enterprise.inject.spi.BeanManager;
import jakarta.enterprise.inject.spi.CDI;
import javax.naming.InitialContext;
import javax.naming.NamingException;

import org.apache.wicket.Application;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import javax.naming.InitialContext;
import javax.naming.NamingException;

/**
* Defines several strategies for looking up a CDI BeanManager in a portable way. The following
* strategies are tried (in order):
* <ul>
* <li>JNDI under java:comp/BeanManager (default location)</li>
* <li>JNDI under java:comp/env/BeanManager (for servlet containers like Tomcat and Jetty)</li>
* <li>CDI.current().getBeanManager() (portable lookup)</li>
* <li>{@linkplain CdiConfiguration#getFallbackBeanManager() Fallback}</li>
* </ul>
*
* The last successful lookup strategy is saved and tried first next time.
*
*
* This is de default strategy used in {@link CdiConfiguration} to look for a BeanManger, unless
* one is defined in CdiConfiguration(BeanManager)
*
* @author papegaaij
*/
public final class BeanManagerLookup
Expand All @@ -45,19 +44,6 @@ public final class BeanManagerLookup

private enum BeanManagerLookupStrategy
{
CUSTOM {
@Override
public BeanManager lookup()
{
CdiConfiguration cdiConfiguration = CdiConfiguration.get(Application.get());

if (cdiConfiguration == null)
throw new IllegalStateException(
"NonContextual injection can only be used after a CdiConfiguration is set");

return cdiConfiguration.getBeanManager();
}
},
JNDI {
@Override
public BeanManager lookup()
Expand Down Expand Up @@ -100,47 +86,27 @@ public BeanManager lookup()
return null;
}
}
},
FALLBACK {
@Override
public BeanManager lookup()
{
return CdiConfiguration.get(Application.get()).getFallbackBeanManager();
}
};

public abstract BeanManager lookup();
}

private static BeanManagerLookupStrategy lastSuccessful = BeanManagerLookupStrategy.CUSTOM;

private BeanManagerLookup()
Copy link
Member

@martin-g martin-g Feb 16, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@pedrosans pedrosans Feb 16, 2026

Choose a reason for hiding this comment

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

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.

{

}

public static BeanManager lookup()
{
BeanManager ret = lastSuccessful.lookup();
if (ret != null)
return ret;

for (BeanManagerLookupStrategy curStrategy : BeanManagerLookupStrategy.values())
{
ret = curStrategy.lookup();
BeanManager ret = curStrategy.lookup();
if (ret != null)
{
lastSuccessful = curStrategy;
return ret;
}
}

throw new IllegalStateException(
"No BeanManager found via the CDI provider and no fallback specified. Check your "
+ "CDI setup or specify a fallback BeanManager in the CdiConfiguration.");
return null;
Copy link
Member

@martin-g martin-g Feb 16, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@pedrosans pedrosans Feb 16, 2026

Choose a reason for hiding this comment

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

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.

}

static void detach()
{
lastSuccessful = BeanManagerLookupStrategy.CUSTOM;
}
}
71 changes: 28 additions & 43 deletions wicket-cdi/src/main/java/org/apache/wicket/cdi/CdiConfiguration.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,10 @@
package org.apache.wicket.cdi;

import jakarta.enterprise.inject.spi.BeanManager;

import org.apache.wicket.Application;
import org.apache.wicket.MetaDataKey;
import org.apache.wicket.request.cycle.RequestCycleListenerCollection;
import org.apache.wicket.util.lang.Args;

/**
* Configures CDI integration
Expand All @@ -39,15 +39,19 @@ public class CdiConfiguration

private BeanManager beanManager;

private BeanManager fallbackBeanManager;

/**
* Constructor
*/
public CdiConfiguration()
{
}

public CdiConfiguration(BeanManager beanManager)
{
Args.notNull(beanManager, "beanManager");
this.beanManager = beanManager;
Copy link
Member

@martin-g martin-g Feb 16, 2026

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

@pedrosans pedrosans Feb 16, 2026

Choose a reason for hiding this comment

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

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.

}

public IConversationPropagation getPropagation()
{
return propagation;
Expand All @@ -61,46 +65,13 @@ public CdiConfiguration setPropagation(IConversationPropagation propagation)

public BeanManager getBeanManager()
{
return beanManager;
}

/**
* Sets a BeanManager that should be used at first.
*
* @param beanManager
* @return this instance
*/
public CdiConfiguration setBeanManager(BeanManager beanManager)
{

if (Application.exists() && CdiConfiguration.get(Application.get()) != null)
if (beanManager == null)
{
throw new IllegalStateException(
"A CdiConfiguration is already set for the application.");

this.beanManager = beanManager;
return this;
}

public BeanManager getFallbackBeanManager()
{
return fallbackBeanManager;
}

/**
* Sets a BeanManager that should be used if all strategies to lookup a
* BeanManager fail. This can be used in scenarios where you do not have
* JNDI available and do not want to bootstrap the CDI provider. It should
* be noted that the fallback BeanManager can only be used within the
* context of a Wicket application (ie. Application.get() should return the
* application that was configured with this CdiConfiguration).
*
* @param fallbackBeanManager
* @return this instance
*/
public CdiConfiguration setFallbackBeanManager(BeanManager fallbackBeanManager)
{
this.fallbackBeanManager = fallbackBeanManager;
return this;
"No BeanManager was resolved during configuration. Be sure " +
"to specify a BeanManager in CdiConfiguration constructor or that one can be resolved by BeanManagerLookup, and that CdiConfiguration#configure is called.");
}
return beanManager;
}

/**
Expand All @@ -110,6 +81,17 @@ public CdiConfiguration setFallbackBeanManager(BeanManager fallbackBeanManager)
*/
public void configure(Application application)
{
if (beanManager == null)
{
beanManager = BeanManagerLookup.lookup();
}

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.");
Comment on lines +89 to +92
Copy link
Member

@martin-g martin-g Feb 16, 2026

Choose a reason for hiding this comment

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

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.");
}

Copy link
Contributor Author

@pedrosans pedrosans Feb 16, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

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.

}

if (application.getMetaData(CDI_CONFIGURATION_KEY) != null)
{
throw new IllegalStateException("Cdi already configured for this application");
Expand Down Expand Up @@ -145,6 +127,9 @@ public void configure(Application application)

public static CdiConfiguration get(Application application)
{
return application.getMetaData(CDI_CONFIGURATION_KEY);
CdiConfiguration configuration = application.getMetaData(CDI_CONFIGURATION_KEY);
if (configuration == null)
throw new IllegalStateException("No CdiConfiguration is set");
return configuration;
}
}
Loading