[WFCORE-3720] Update ServiceModuleLoader to use non-deprecated JBoss Modules APIs #6247
Conversation
|
Core -> Full Integration Build 14045 outcome was FAILURE using a merge of 070bc6a |
|
Core -> WildFly Preview Integration Build 14122 outcome was FAILURE using a merge of 070bc6a |
|
Core -> Full Integration Build 14341 outcome was FAILURE using a merge of 070bc6a |
|
In general ModuleIdentifier.getName() is not a safe replacement for ModuleIdentifier. It does not include any slot value. ModuleIdentifier.toString() provides the String variant of the module id. |
| @@ -74,7 +74,7 @@ public void execute(OperationContext context, ModelNode operation) { | |||
| moduleIdentifier = deploymentUnit.getAttachment(Attachments.MODULE_IDENTIFIER); | |||
| } | |||
|
|
|||
| final ServiceController<?> moduleLoadServiceController = sr.getService(ServiceModuleLoader.moduleServiceName(moduleIdentifier)); | |||
| final ServiceController<?> moduleLoadServiceController = sr.getService(ServiceModuleLoader.moduleServiceName(moduleIdentifier.getName())); | |||
There was a problem hiding this comment.
All the changes like this need to be updated to use toString(), not getName().
| return name.startsWith(MODULE_PREFIX); | ||
| } | ||
|
|
||
| @Deprecated |
There was a problem hiding this comment.
Please add forRemoval=true and add javadoc @deprecated pointing to the new method.
|
Hi @bstansberry, thank you for the review, change requests should be addressed now. |
server/src/main/java/org/jboss/as/server/moduleservice/ServiceModuleLoader.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/jboss/as/server/moduleservice/ServiceModuleLoader.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/jboss/as/server/moduleservice/ServiceModuleLoader.java
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
|
Thanks @yersan, updated. |
| public static ServiceName moduleSpecServiceName(String name) { | ||
| if (!isDynamicModule(name)) { | ||
| throw ServerLogger.ROOT_LOGGER.missingModulePrefix(name, MODULE_PREFIX); | ||
| } | ||
| return MODULE_SPEC_SERVICE_PREFIX.append(identifier.getName()).append(identifier.getSlot()); | ||
| return MODULE_SPEC_SERVICE_PREFIX.append(name); | ||
| } |
There was a problem hiding this comment.
I would say we should Keep the ServiceName moduleSpecServiceName(ModuleIdentifier identifier) variant and deprecate it for removal.
There was a problem hiding this comment.
nitpick: I would also keep the argument name as identifier. Even being a string, it will be an identifier (name+slot). It is not a hard requirement, but I think it is more coherent with what it is. The ModuleIdentifier class keeps the two fields (name and slot). It also has a method named moduleIdentifier.getName() which will return only the name, without a slot. So, naming the argument as name here could be misleading and make someone think that it should be invoked with moduleIdentifier.getName().
| final org.jboss.msc.Service resolvedService = org.jboss.msc.Service.newInstance(moduleIdConsumer, identifier); | ||
| final Consumer<String> moduleIdConsumer = sb.provides(sn); | ||
| sb.requires(moduleSpecServiceName(name)); | ||
| final org.jboss.msc.Service resolvedService = org.jboss.msc.Service.newInstance(moduleIdConsumer, name); | ||
| sb.setInstance(resolvedService); | ||
| sb.install(); | ||
| } |
There was a problem hiding this comment.
The same here, do not remove the previous method without deprecating it first.
|
Thanks @yersan, updated. |
|
FYI re #6270 I don't know if that will be the ultimate approach, and I haven't looked at the current state of this to see if it's applicable to the change here. But FYI. My expected use of that utility is that in any place where we are taking external input as the string naming a module, we run it through that utility and then use the canonicalized string. Internal places where we know any passed in string has already been through that canonicalization can just use the provided string. Understanding what case is what requires care. External inputs include things like management model 'module' attribute values, management model ee subsystem global directory resource names, data parsed from deployment descriptors and data parsed from MANIFEST files. Perhaps others but I think that's the list. If what I'm talking about is applicable here, please hold this until we get something like #6270 merged and then this PR should use it. |
|
This is tricky. The new String variants have been created from the However, it extends the |
Well, the only one inherited from ModuleLoader is The others I guess are up to us, but if they cannot manage a canonized version, we should document them as such since they are public methods. I think the problem here is that they are public methods so we cannot control who can invoke them outside of our code. @bstansberry / @lvydra any thoughts on this? |
|
If there is no other feedback, just to sum up what is pending:
|
|
@lvydra In addition to my previous comment, could you also add a "since=27.0.0" attribute to the @deprecated annotations? it will help us track when they were deprecated without looking at the Git history, thanks! |
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
| @@ -128,9 +130,20 @@ protected Module preloadModule(final String name) throws ModuleLoadException { | |||
| } | |||
| } | |||
|
|
|||
| /** | |||
| * @deprecated Use {@link ServiceModuleLoader#findModule(String)} | |||
There was a problem hiding this comment.
Please change this to:
@deprecated Will be made protected in line with this method in the parent class
There may have been a reason for this being public in 2011 when it was changed to public, but I don't see it now. It should be an internal call within ModuleLoader, not used by outside code. (WF and WF Core code don't call this method directly.)
I doubt any code is calling this and if they are they are misusing something. Removing ModuleIdentifier will be a breaking change to whatever the code might be so they might as well stop doing whatever that is.
|
|
||
| @SuppressWarnings("unchecked") | ||
| @Override | ||
| public ModuleSpec findModule(String identifier) throws ModuleLoadException { |
There was a problem hiding this comment.
I think this should be protected, in line with the superclass.
I don't think it's our responsibility to handle this. JBoss Modules itself doesn't, so I don't think we should. As I commented on the new method, I think it should be protected, as it's not meant to be called by arbitrary code. (And isn't). Note that nothing in WF calls findModule. It's called by ModuleLoader.loadModuleLocal. If the identifier passed from there is non-canonical then lots of other code in JBM has already been trying to deal with that string.
The responsibility for using canonicalModuleIdentifier should belong to the code that is accepting external inputs for module names. If we start using it inside tons methods that take a String passed in by other code we'll end up canonicalizing the same String multiple times. +1 to javadoc; the way it's handled in the PR looks good. |
|
Thanks @bstansberry, updated. |
|
Core -> WildFly Preview Integration Build 14210 outcome was FAILURE using a merge of cd0f508 Failed tests |
|
Core -> Full Integration Build 14429 outcome was FAILURE using a merge of cd0f508 Failed tests |
yersan
added
the
ready-for-merge
|
Thanks @lvydra and @bstansberry |
https://issues.redhat.com/browse/WFCORE-3720