You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@wicket.apache.org by "pedrosans (via GitHub)" <gi...@apache.org> on 2026/02/15 00:15:10 UTC
[PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
pedrosans opened a new pull request, #1369:
URL: https://github.com/apache/wicket/pull/1369
This change clean 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;
Another benefit of this change is the reduced number of times the new code will make expensive (time consuming) JNDI lookups.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
Posted by "pedrosans (via GitHub)" <gi...@apache.org>.
pedrosans commented on code in PR #1369:
URL: https://github.com/apache/wicket/pull/1369#discussion_r2813568589
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/BeanManagerLookup.java:
##########
@@ -100,47 +83,22 @@ 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()
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #1369:
URL: https://github.com/apache/wicket/pull/1369#discussion_r2810967719
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/CdiConfiguration.java:
##########
@@ -110,6 +77,13 @@ 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.");
Review Comment:
```suggestion
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.");
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #1369:
URL: https://github.com/apache/wicket/pull/1369#discussion_r2810963679
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/CdiConfiguration.java:
##########
@@ -110,6 +77,13 @@ public CdiConfiguration setFallbackBeanManager(BeanManager fallbackBeanManager)
*/
public void configure(Application application)
{
+ if(beanManager == null)
+ beanManager = BeanManagerLookup.lookup();
Review Comment:
```suggestion
if (beanManager == null)
{
beanManager = BeanManagerLookup.lookup();
}
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
Posted by "pedrosans (via GitHub)" <gi...@apache.org>.
pedrosans commented on PR #1369:
URL: https://github.com/apache/wicket/pull/1369#issuecomment-3905068110
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #1369:
URL: https://github.com/apache/wicket/pull/1369#discussion_r2810907243
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/BeanManagerLookup.java:
##########
@@ -100,47 +83,22 @@ 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()
- {
- }
-
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;
Review Comment:
What is the benefit here of returning `null` than throwing an exception ?
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/CdiConfiguration.java:
##########
@@ -110,6 +77,13 @@ public CdiConfiguration setFallbackBeanManager(BeanManager fallbackBeanManager)
*/
public void configure(Application application)
{
+ if(beanManager == null)
Review Comment:
```suggestion
if (beanManager == null)
```
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/CdiConfiguration.java:
##########
@@ -61,46 +64,10 @@ 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;
+ "app not configured or no BeanManager was resolved during the configuration");
Review Comment:
The error message is vague and does not suggest actionable guidance for the developers.
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/CdiConfiguration.java:
##########
@@ -39,15 +39,18 @@ public class CdiConfiguration
private BeanManager beanManager;
- private BeanManager fallbackBeanManager;
-
/**
* Constructor
*/
public CdiConfiguration()
{
}
+ public CdiConfiguration(BeanManager beanManager)
+ {
+ this.beanManager = beanManager;
Review Comment:
Maybe add a non-null check ?!
Otherwise, it behaves as the no-arg constructor if `null` is passed.
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/BeanManagerLookup.java:
##########
@@ -100,47 +83,22 @@ 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()
Review Comment:
Why is this constructor removed ?
This is a utility class with a single static method.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
Posted by "pedrosans (via GitHub)" <gi...@apache.org>.
pedrosans commented on code in PR #1369:
URL: https://github.com/apache/wicket/pull/1369#discussion_r2813478246
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/CdiConfiguration.java:
##########
@@ -110,6 +77,13 @@ 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.");
Review Comment:
> > 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. Lets say NonContextual made one call to BeanManagerLookup during a request response; the number of times a JNDI lookup got made would be tied to the number of requests.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
Posted by "pedrosans (via GitHub)" <gi...@apache.org>.
pedrosans commented on code in PR #1369:
URL: https://github.com/apache/wicket/pull/1369#discussion_r2813553630
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/BeanManagerLookup.java:
##########
@@ -100,47 +83,22 @@ 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()
- {
- }
-
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;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
Posted by "pedrosans (via GitHub)" <gi...@apache.org>.
pedrosans commented on code in PR #1369:
URL: https://github.com/apache/wicket/pull/1369#discussion_r2813478246
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/CdiConfiguration.java:
##########
@@ -110,6 +77,13 @@ 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.");
Review Comment:
> > 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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on code in PR #1369:
URL: https://github.com/apache/wicket/pull/1369#discussion_r2810883396
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/CdiConfiguration.java:
##########
@@ -110,6 +77,13 @@ public CdiConfiguration setFallbackBeanManager(BeanManager fallbackBeanManager)
*/
public void configure(Application application)
{
+ if(beanManager == null)
Review Comment:
```suggestion
if (beanManager == null)
```
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
Posted by "martin-g (via GitHub)" <gi...@apache.org>.
martin-g commented on PR #1369:
URL: https://github.com/apache/wicket/pull/1369#issuecomment-3906649457
The plugin should be disabled in the master branch until 11.0.0 is released.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
Posted by "pedrosans (via GitHub)" <gi...@apache.org>.
pedrosans commented on code in PR #1369:
URL: https://github.com/apache/wicket/pull/1369#discussion_r2813501287
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/CdiConfiguration.java:
##########
@@ -61,46 +64,10 @@ 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;
+ "app not configured or no BeanManager was resolved during the configuration");
Review Comment:
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."
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] WICKET-7126 simplify CdiConfiguration + BeanManagerLookup [wicket]
Posted by "pedrosans (via GitHub)" <gi...@apache.org>.
pedrosans commented on code in PR #1369:
URL: https://github.com/apache/wicket/pull/1369#discussion_r2813519263
##########
wicket-cdi/src/main/java/org/apache/wicket/cdi/CdiConfiguration.java:
##########
@@ -39,15 +39,18 @@ public class CdiConfiguration
private BeanManager beanManager;
- private BeanManager fallbackBeanManager;
-
/**
* Constructor
*/
public CdiConfiguration()
{
}
+ public CdiConfiguration(BeanManager beanManager)
+ {
+ this.beanManager = beanManager;
Review Comment:
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.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: commits-unsubscribe@wicket.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org