[
https://issues.apache.org/jira/browse/WICKET-7126?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=18058822#comment-18058822
]
ASF GitHub Bot commented on WICKET-7126:
----------------------------------------
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.
> CdiConfiguration new API
> ------------------------
>
> Key: WICKET-7126
> URL: https://issues.apache.org/jira/browse/WICKET-7126
> Project: Wicket
> Issue Type: Improvement
> Components: wicket-cdi
> Reporter: Pedro Henrique Oliveira dos Santos
> Assignee: Pedro Santos
> Priority: Minor
>
> Context: originally BeanManagerLookup contained the entire logic to resolve
> the app BeanManager, with no option to set one manually, for instance. This
> was improved last year, but the code became a mix of states being kept in
> CdiConfiguration and BeanManagerLookup.
> Improvement:
> * move the entire logic + state to CdiConfiguration
> ** to remove the CdiConfiguration.set/getFallbackBeanManager and its related
> logic
> ** to remove the BeanManagerLookup lastSuccessful state
--
This message was sent by Atlassian Jira
(v8.20.10#820010)
