WICKET-7126 simplify CdiConfiguration + BeanManagerLookup#1369
WICKET-7126 simplify CdiConfiguration + BeanManagerLookup#1369
Conversation
|
build fails with:
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. |
|
The plugin should be disabled in master branch until 11.0.0 is released. |
martin-g
left a comment
martin-g
left a comment
There was a problem hiding this 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.
| { | ||
| this.fallbackBeanManager = fallbackBeanManager; | ||
| return this; | ||
| "app not configured or no BeanManager was resolved during the configuration"); |
There was a problem hiding this comment.
The error message is vague and does not suggest actionable guidance for the developers.
|
|
||
| private static BeanManagerLookupStrategy lastSuccessful = BeanManagerLookupStrategy.CUSTOM; | ||
|
|
||
| private BeanManagerLookup() |
There was a problem hiding this comment.
Why is this constructor removed ?
This is a utility class with a single static method.
|
|
||
| public CdiConfiguration(BeanManager beanManager) | ||
| { | ||
| this.beanManager = beanManager; |
There was a problem hiding this comment.
Maybe add a non-null check ?!
Otherwise, it behaves as the no-arg constructor if null is passed.
| 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; |
There was a problem hiding this comment.
What is the benefit here of returning null than throwing an exception ?
| if(beanManager == null) | ||
| beanManager = BeanManagerLookup.lookup(); |
There was a problem hiding this comment.
| if(beanManager == null) | |
| beanManager = BeanManagerLookup.lookup(); | |
| 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."); |
There was a problem hiding this comment.
| 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."); | |
| } |
2 participants
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.