Join GitHub today
GitHub is home to over 40 million developers working together to host and review code, manage projects, and build software together.
Sign upWFLY-11870 Do not process resource annotations on abstract classes or … #12217
Conversation
This comment has been minimized.
This comment has been minimized.
|
@stuartwdouglas as this is related to #11203, could you review? |
This comment has been minimized.
This comment has been minimized.
|
I'm temporarily closing this PR, before I verify that it's passing the TCK. |
This comment has been minimized.
This comment has been minimized.
|
Passes TCK 8 on my machine. |
| } else { | ||
| subclasses = index.getAllKnownSubclasses(classInfo.name()); | ||
| } | ||
| return subclasses.stream().anyMatch(ModuleJndiBindingProcessor::isConcreteClass); |
This comment has been minimized.
This comment has been minimized.
bstansberry
Apr 14, 2019
Contributor
Please use a for loop. The class metadata memory cost of a method reference vs a for loop is too high for us to be using this code pattern in server code.
| @@ -158,6 +162,7 @@ public void deploy(final DeploymentPhaseContext phaseContext) throws DeploymentU | |||
| //were only intended to be installed when running as an app client | |||
| boolean appClient = DeploymentTypeMarker.isType(DeploymentType.APPLICATION_CLIENT, deploymentUnit) || this.appclient; | |||
|
|
|||
| final CompositeIndex index = deploymentUnit.getAttachment(org.jboss.as.server.deployment.Attachments.COMPOSITE_ANNOTATION_INDEX); | |||
This comment has been minimized.
This comment has been minimized.
bstansberry
Apr 14, 2019
Contributor
This should be inside the if (!MetadatdCompleteMarker... block.
| @@ -167,6 +172,10 @@ public void deploy(final DeploymentPhaseContext phaseContext) throws DeploymentU | |||
| if(config.isInvalid()) { | |||
| continue; | |||
| } | |||
| ClassInfo classInfo = index.getClassByName(DotName.createSimple(config.getClassName())); | |||
This comment has been minimized.
This comment has been minimized.
bstansberry
Apr 14, 2019
Contributor
To ease future maintenance this needs a nice comment before it explaining why this will work; i.e. an Index contains info on all classes in a jar, and the CompositeIndex in the 'index' var aggregates all the Index objects for the jars visible to the deployment unit. So you can scan it for relationships between classes and not miss out on any.
| ClassInfo classInfo = index.getClassByName(DotName.createSimple(config.getClassName())); | ||
| if (!isConcreteClass(classInfo) && !hasConcreteSubclass(index, classInfo)) { | ||
| continue; | ||
| } | ||
| final Set<BindingConfiguration> classLevelBindings = new HashSet<>(config.getBindingConfigurations()); |
This comment has been minimized.
This comment has been minimized.
bstansberry
Apr 14, 2019
Contributor
I recommend moving L175-178 below this line and wrapping it in an 'if (!classLevelBindings.isEmpty())' check. The l175-178 scan is not cheap so it should be avoided whenever possible.
|
Please see comments above. Thanks for checking this using the TCK! |
… interfaces that do not have concrete subclasses
This comment has been minimized.
This comment has been minimized.
|
I updated according to your comments, thanks for input! |
This comment has been minimized.
This comment has been minimized.
|
@bstansberry We'll include this fix in EAP 7.2.2 if merged. Can you review the changes? Thanks! |
bstansberry
merged commit 7d4ba27
into
wildfly:master
TomasHofman commentedApr 10, 2019
…interfaces that do not have concrete subclasses
Issue: https://issues.jboss.org/browse/WFLY-11870
Downstream issue: https://issues.jboss.org/browse/JBEAP-16576
Related to: #11203
This issue comes from a customer case, they are unable to deploy an application to EAP 7.2, which was deployable on EAP 7.1.
The point is that their deployment contains abstract class with @ejb annotated field referencing an interface:
Neither the referencing abstract class nor the referenced interface have concrete subclasses in this deployment, so theoretically they should be ignored? The deployment fails because a binding processor can't find any bean that would satisfy this injection point.
In the PR, I'm trying to prevent the resource annotations from being processed if an annotated class is an abstract class or an interface and there's no concrete subclass in the deployment.