OWB-1448 Fix Issue with Cdi annotation and alternatives #126
Conversation
| catch (Exception e) | ||
| { | ||
| logger.info("Error occurred: " + e.getMessage()); | ||
| e.printStackTrace(); |
There was a problem hiding this comment.
I guess the e.printStackTrace is a left over?
There was a problem hiding this comment.
Hehe, yes, I forgot to remove that in my excitement to open the PR. :-)
|
Hi @struberg , thanks for the review and feedback. Does the overall approach look ok, or is there a better way to achieve this? |
| @@ -1756,6 +1759,34 @@ protected void deployFromXML(ScannerService scanner) | |||
| } | |||
|
|
|||
| logger.fine("Deploying configurations from XML has ended successfully."); | |||
|
|
|||
| try | |||
There was a problem hiding this comment.
wonder why the url is not in the scanner since it should so we do not need that hack at all in impl
There was a problem hiding this comment.
I was hoping there would be a public static final URL CDI_STANDALONE = ... somewhere, in order to avoid this. org.apache.openwebbeans.se.CDISeBeanArchiveService#EMBEDDED_URL was the closest I could find, but is a String as opposed to a URL.
Maybe we could make this a constant URL in org.apache.webbeans.xml.DefaultBeanArchiveService?
There was a problem hiding this comment.
hmm not sure I got it right but my point is that this code shouldn't be since the url should be injected in the scanner in SE mode so in owb-impl the url should already be seen (thanks addDeploymentUrl call)
what we likely do not want is impl to depend on se (like in this pr)
hope it makes more sense phrased this way
There was a problem hiding this comment.
I think I understand :-).
Happy to debug around addDeploymentUrl, and see what I can find. What I can tell you is that if you revert out the BeansDeployer change in this PR, the tests in this PR will fail (or at least they do for me). I'm very happy to look at a different fix (I might have questions.... :-) ), but I do think a fix is needed.
There was a problem hiding this comment.
Think we all agree the API should be respected and the regression be fixed if any (maybe java related, didnt check if latest jre changed url handling), but think we should stick to the original design of se on top of impl and not a cycle dep and url 100% taken from the scanner, even virtual ones which were just a hack to make it work without a new spi. This part can change if it help - but think we can make it work, there should be something fishy making the fake url dropped somewhere in the process.
can try to help to debug next week if there is no new finding
There was a problem hiding this comment.
addDeploymentUrl is called from org.apache.openwebbeans.se.CDISeScannerService#addClassesDeploymentUrl with the embedded URL (great), however, the scanner.getBeanXmls() call from org.apache.webbeans.config.BeansDeployer#deployFromXML does not return that URL, as addClassesDeploymentUrl adds the URL to org.apache.webbeans.corespi.scanner.AbstractMetaDataDiscovery#beanDeploymentUrls and scanner.getBeanXmls() gets the URLs from org.apache.webbeans.corespi.scanner.AbstractMetaDataDiscovery#beanArchiveLocations.
Could we just include the embedded URL in org.apache.webbeans.corespi.scanner.AbstractMetaDataDiscovery#beanArchiveLocations?
There was a problem hiding this comment.
Doing this:
protected void addDeploymentUrl(String beansXml, URL cpUrl)
{
beanDeploymentUrls.put(beansXml, cpUrl);
beanArchiveLocations.add(cpUrl);
}
in org.apache.webbeans.corespi.scanner.AbstractMetaDataDiscovery#addDeploymentUrl
works. Would that change be ok?
There was a problem hiding this comment.
guess it will need to call doAddWebBeansXmlLocation but looks like the additional call to do
that said doing it in addDeploymentUrl will change the lifecycle for the nominal case so i'd just align on the nominal case so adjust CDISeScannerService (a bit like web scanner has web-inb url handling specifically)
hope it makes sense
|
@rmannibucau I appreciate the feedback. I've pushed a different change, which seems to pass all the tests. Hopefully I've understood the feedback and this is a better solution. |
|
except the wildcard import LGTM, thanks |
|
thanks @rmannibucau ! |
No description provided.