[WFLY-11186][WFLY-11187] Declare a capability for weld subsystem

[yersan]

• Remove the org.jboss.as.weld dependency from modules which are not using it
• Add capbility hard requirements for subsystems that depends on Weld

Notice the following: weld dependencies of xts are being treated in #11800, so I don't remove them here

Jira issue: https://issues.jboss.org/browse/WFLY-11186
Includes: https://issues.jboss.org/browse/WFLY-11187

[bstansberry]

@yersan What is the meaning of the 'weld' capability? For the other capabilities that require it, what are they requiring? If they require something they must expect that some contract will be fulfilled. What is that contract?

A private capability doesn't have to have a clear contract. But externally depended on capabilities must.

Once the contract clear, a question is whether some other provider of the capability can fulfill the contract. IOW, is 'weld' an appropriate part of the capability name, or is it an implementation detail?

[yersan]

What is the meaning of the 'weld' capability? For the other capabilities that require it, what are they requiring? If they require something they must expect that some contract will be fulfilled. What is that contract?

The contract is the server is able to work with Context and Dependency injection and a CDI container will be available to register beans coming from user deployments, and this cdi container is the weld implementation.

Once the contract clear, a question is whether some other provider of the capability can fulfill the contract. IOW, is 'weld' an appropriate part of the capability name, or is it an implementation detail?

It is an implementation detail, but the weld implementation could have some features that are not in other cdi implementations, to me, it means weld in the name has some meaning. I'm not sure how we usually model it, maybe we could have in the future different capabilities (org.wildfly.cdi.owb / org.wildfly.cdi.weld) ?

I feel I'm missing something, does the above make sense to you?

[bstansberry]

@yersan I think that's too loose of a contract. Other capabilities have clear contracts; in almost all cases it's that they expose a Service with a particular value type that other services provided by other capabilities can depend on. A less simple one is the 'org.wildfly.transactions.global-default-local-provider' I describe in my comment from Oct 31 at #11745. That too I could describe quite precisely in a sentence or two in a doc in wildfly-capabilities. (Depend on the service the capability provides and once started you can safely use ContextTransactionManager/ContextTransactionSynchronizationRegistry/LocalUserTransaction knowing there is a local provider backing up those APIs.)

From the above I don't mean to imply this CDI one is a simple problem.

[kabir]

@yersan @bstansberry I am adding the hold label until you guys figure out the way forward. Ping me/merge when you're both happy

[yersan]

@bstansberry @jmesnil
To facilitate the initial PR review, I have updated it with the latest changes that are going to require this core PR merged before: wildfly/wildfly-core#3597

I decided to move the WeldDeploymenMarked to wildfly-core in that PR, because we had other classes with a similar purpose there and to avoid to force the use of Weld capability API to internal Weld modules (wildfly-weld-bean-validation requires it). The idea behind is to keep any public class in wildfly-weld giving the API functionality hidden from any external modules. External modules have to use wildfly-weld-common as dependency and to use the WeldCapability interface to access to the API.

Looking at it again, maybe the pain of using the capability API in an internal module is not too bad, and we can keep it in wildfly-weld-common. It will require to add additional jboss modules dependencies and maven dependencies on wildfly-weld-common though for other modules.

Another possibility is to not hide the functionality to the other modules, making the classes public in wildfly-weld-common, it will allow using the Weld functionalities in any weld module without accesing to the Weld API capability (all of them could depend on wildfly-weld-common without any circular references). That makes the code less restrictive, which could not be ideal.

[bstansberry]

I don't see any reason wildfly-weld-bean-validation shouldn't use the capability the same as any other module. It already depends on wildfly-weld-common so it will have access to WildFlyCapability.

The WeldDeploymentMarker functionality can be part of WildFlyCapability, removing the need to add a subsystem-specific to the WF Core kernel.

I believe the modules that would then have to depend on weld-common that currently do not would be:

org.jboss.as.jaxrs
org.wildfly.extension.bean-validation
org.wildfly.extension.microprofile.opentracing-smallrye

The first two can be optional dependencies (class only needed if capability present) so IMHO that's fine. (JAXRS already has such a thing for resteasy-cdi.) The opentracing one could be optional too, although in reality CDI is required for OT to work so it's not terrible to have a non-optional capability and module dependency.

I don't mind leaving WeldPortableExtensions public but deprecated in weld-common. Really at least for a period we have to, as moving it can break any external to WildFly module that depends on it.

[yersan]

@bstansberry @jmesnil I reworked the PR, in general terms:

• Weld Capability API is exposed via wildfly-weld-common
• WeldDeploymentMarker and WeldPortableExtensions were deprecated and moved to a _private package in widfly-weld. I think I should have done the same for BeanManagerService and org.jboss.as.weld.ServiceNames#beanManagerServiceName because they are exposing some functionality that must be used via WeldCapability, but I'm not complete sure about the consequences removing the public access from there, a documentation was added instead.
• It still requires wildfly/wildfly-core#3597 to be able to retrieve the Weld Capability API with an Optional (it removes the need of catch a checked exception)

[bstansberry]

The capability API is not great here. The use of the WeldCapability class is not guarded by anything that ensures the class doesn't have to be loaded if the cap is not present. CapababilityServiceSupport.hasCapability is meant to be that guard. Using that combined with an Optional.ifPresent amounts to a double-check of whether the capability is there. Perhaps I should have just made NoSuchCapabilityException an unchecked exception. Making it checked basically helps force coders to think about the possibility of it not being there (i.e. call hasCapability first) but having to catch a checked exception is too big a cost.

[bstansberry]

Actually what you do at https://github.com/wildfly/wildfly/pull/11802/files#diff-b0185cd463b77ad4a276681302a1d18eR123 is reasonable enough looking, so I think the odd thing here would be the use of ifPresent. Which isn't truly necessary. You wouldn't even have to store a ref to WeldCapability in a var if addBeanManagerService did the isPartOfWeldDeployment check itself.

[yersan]

The capability API is not great here. The use of the WeldCapability class is not guarded by anything that ensures the class doesn't have to be loaded if the cap is not present.

I would say it is warded, the ifPresent only will unwrap the optional value, exposing the WeldCapability class to the caller, if only if the optional is not empty. If it is empty, then the code will not use a WeldCapability instance, and then the calling code could have the module that loads this class as an optional dependency.
The optional will be empty if only if the capability is not present. Indeed support.hasCapability(WELD_CAPABILITY_NAME) returning false should be equivalent. If they are not equivalent, a bug is present in any of them. Both methods will end up executing the same code: CapabilityRegistry.findSatisfactoryCapability

One possible problem with the Optional.ifPresent is that it requires a consumer as an argument, and any variable initialized outside of the consumer lamda expression must be final or effectively final to be used in it. If you need to use a variable that is not final or effectively final, then the caller is forced to use support.hasCapability(WELD_CAPABILITY_NAME) or Optional.isPresent() check before using the API to avoid the lamda expression, that means there could be inconsistencies in the code. @bstansberry If you still think this is a good argument to remove the uses of ifPresent, then I can change the PR according to use always support.hasCapability(WELD_CAPABILITY_NAME) check before getting the API, keeping all the code using the same structure. What do you think?

Perhaps I should have just made NoSuchCapabilityException an unchecked exception. Making it checked basically helps force coders to think about the possibility of it not being there (i.e. call hasCapability first) but having to catch a checked exception is too big a cost.

I think it is fine, we now should use the method with the checked exception if the capability is mandatory to the caller, and then catch and react according if the capability is not there. I think if we use an optional, the caller is still forced to think what to do if the capability is not present, although this method should now be used if the capability is optional in the system. Yes, the try/catch of the exception has a cost and the virtual machine is unable to do some optimizations in those blocks.

The inclusion of isPartOfWeldDeployment inside of addBeanManagerService or registerExtensionInstance makes sense; both methods only are relevant if the Deployment Unit is part of a Weld deployment, although we could lose some flexibility in the API, for example, to do something before addBeanManagerService or registerExtensionInstance and after checking that the deployment pass isPartOfWeldDeployment without a double execution of isPartOfWeldDeployment, see https://github.com/wildfly/wildfly/pull/11802/files#diff-9c678c3939366680a48d047f02f37d41R139 or https://github.com/wildfly/wildfly/pull/11802/files#diff-cdf1bb068d2ed81a62e021e1e0419c2aR1156.
The effect is a double execution of isPartOfWeldDeployment, but it is a very cheap operation so we can do it. @bstansberry Do you think we can assume the loss of this little flexibility?

[bstansberry]

Re: the use of WeldCapability being warded, the issue isn't the Optional API use, it's this:

support.getOptionalCapabilityRuntimeAPI(WELD_CAPABILITY_NAME, WeldCapability.class)

That has no guard.

If there is a case where the isPartOfWeldDeployment check ends up happening twice, that's fine. It's not particularly expensive.

If you prefer staying with the checked exception, that's fine with me. I originally wrote the current approach so I thought it was right then, and have minor doubts now. So my opinion on changing it is not a strong one. :) And your Optional addition shows there's a reasonable way to avoid the exception.

[yersan]

@bstansberry My apologies, I was completly blind on this, yes, WeldCapability.class is a reference the optional dependency on wildfly-weld-common.

I prefer the optional version, it removes bolerplate code in the client if the capability is optional.

[yersan]

As soon as wildfly-core 8.0.0.Beta3 be merged in wildfly, I'll rebase this PR and fix the conflict.

[yersan]

CI failures must be related. I'm reviewing the changes.

[yersan]

Rebased and resolved a new conflict

[yersan]

Rebased and resolved a new conflict. Added CDI layer to microprofile. Let's see CI results.

[yersan]

@bstansberry It would be nice if you add the hold label again here. This PR must be revised after merging all the layers stuff, and right now I'm not sure if it is fine as it is.

[yersan]

Rebased and resolved conflicts, commits squashed
@bstansberry I'm fine with the changes included in this PR, you can remove the hold label, finally, we agreed into include CDI on microprofile layer.

[bstansberry]

@yersan If you can fix the conflicts (HEAD is right; the changes in this PR are not needed any more in the conflicted module.xml and pom.xml) and @ ping me when pushed I'll get this merged as soon as CI is done.

[yersan]

@bstansberry Done, CI finished successfully.