WFLY-8656: External bean archives - support all bean discovery modes. #10649
Conversation
mkouba
commented
Nov 9, 2017
mkouba
commented
- https://issues.jboss.org/browse/WFLY-8656
- Test is taken from the original PR: Integration test (failing) for WFLY-8656 #9978
|
retest this please |
|
Retest this please |
| } | ||
| }; | ||
| } else { | ||
| // Build ClassInfo on the fly |
There was a problem hiding this comment.
We may have already built this and placed it under the org.jboss.as.server.deployment.Attachments#ADDITIONAL_ANNOTATION_INDEXES_BY_MODULE key if annotations are imported, it would be better to re-use this rather than building it again.
There was a problem hiding this comment.
Hm, but we only build the index if no META-INF/jandex.idx is found in exported resources for a specific beans.xml. Also it seems Attachments.ADDITIONAL_ANNOTATION_INDEXES_BY_MODULE only contains indexes for modules where META-INF/jandex.idx exists and annotations are imported.
Attachments.COMPOSITE_ANNOTATION_INDEX probably contains indexes from all deployment module dependencies (loaded or calculated on the fly). Shouldn't we use this instead?
There was a problem hiding this comment.
No, it will build an index if it does not exist (previous versions this was not the case, but we changed this a while ago to remove the need to manually index external modules, although this does only happen when annotations are imported).
Looking at the current implementation a bit closed I can see this actually won't work though, as it indexes the module as a whole while you are just looking at each jar individually, so the current implementation is fine.
There was a problem hiding this comment.
No, it will build...
I think it's built but not added to Attachments.ADDITIONAL_ANNOTATION_INDEXES_BY_MODULE, only to Attachments.COMPOSITE_ANNOTATION_INDEX:
https://github.com/wildfly/wildfly-core/blob/master/server/src/main/java/org/jboss/as/server/deployment/annotation/CompositeIndexProcessor.java#L101
...so the current implementation is fine
OK
| Indexer indexer = new Indexer(); | ||
| consumer = (name, classFile) -> { | ||
| try { | ||
| ClassInfo classInfo = indexer.index(classFile.openStream()); |
There was a problem hiding this comment.
It does not look like this stream is closed?
There was a problem hiding this comment.
You're right, I'll fix it right away. Thanks!
stuartwdouglas
added
the
ready-for-merge
