Support optional to optional extension dependencies #7729
Conversation
Allow optional extensions to depend on other optional extensions in the same add-on. Signed-off-by: ricekot <github@ricekot.com>
| @@ -126,6 +126,8 @@ public class AddOnLoader extends URLClassLoader { | |||
| */ | |||
| private Map<String, AddOnClassLoader> addOnLoaders = new HashMap<>(); | |||
|
|
|||
| private Map<String, Collection<AddOnClassLoader>> extensionLoaders = new HashMap<>(); | |||
There was a problem hiding this comment.
The class loaders are being added but never removed from the map, when the extension/add-on is unloaded.
| DEPENDENCIES_ELEMENT + "." + DEPENDENCIES_ADDONS_ALL_ELEMENTS; | ||
| private static final String EXTENSION_EXTENSION_DEPENDENCIES = |
There was a problem hiding this comment.
The JavaDoc could be updated to mention the new element.
| List<HierarchicalConfiguration> fields = | ||
| List<HierarchicalConfiguration> addOnFields = | ||
| node.configurationsAt(DEPENDENCIES_ADDONS_ALL_ELEMENTS); | ||
| if (fields.isEmpty()) { | ||
| return new Dependencies(javaVersion, Collections.<AddOnDep>emptyList()); | ||
| var extensionFields = node.configurationsAt(DEPENDENCIES_EXTENSIONS_ALL_ELEMENTS); | ||
| if (addOnFields.isEmpty() && extensionFields.isEmpty()) { | ||
| return new Dependencies(javaVersion, List.of(), List.of()); | ||
| } | ||
|
|
||
| List<AddOnDep> addOns = readAddOnDependencies(fields); | ||
| return new Dependencies(javaVersion, addOns); | ||
| List<AddOnDep> addOns = readAddOnDependencies(addOnFields); | ||
| List<ExtensionDep> extensions = readExtensionDependencies(extensionFields); | ||
| return new Dependencies(javaVersion, addOns, extensions); |
There was a problem hiding this comment.
These changes (and in Dependencies) should not be needed, the add-on will not depend on extensions(?).
| if (classname == null) { | ||
| continue; | ||
| } |
There was a problem hiding this comment.
This should not be needed, the name is required.
| @@ -320,16 +328,12 @@ protected Class<?> findClass(String name) throws ClassNotFoundException { | |||
| } | |||
| } | |||
|
|
|||
| try { | |||
There was a problem hiding this comment.
I think we should keep this the default behaviour (classes of the add-on should have priority over the ones in the dependencies) and allow to change it when needed.
| if (classname == null) { | ||
| continue; | ||
| } | ||
| if (addOn.getExtensionsWithDeps().contains(classname)) { |
There was a problem hiding this comment.
It should also check that it can be loaded (i.e. the transitive dependencies are all met).
|
Closing as per discussions in Slack. The original intention of this PR was to support the case where the GraphQL add-on wanted to use some functionalities of the quickstart add-on (launch the user's preferred browser with GraphQL IDE URL open) without explicitly depending on it. There are other ways to achieve this use case, e.g. move any functionality needed by two or more add-ons into commonlib (in this case, launching a browser). |
Allow optional extensions to depend on other optional extensions in the same add-on.
Signed-off-by: ricekot github@ricekot.com