-
Notifications
You must be signed in to change notification settings - Fork 513
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Support plugin management and dynamic plugin installation with extensions #984
Conversation
# Conflicts: # pom.xml
WalkthroughThe recent updates to the SOFA Ark framework include significant enhancements to the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 6
Outside diff range, codebase verification and nitpick comments (3)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/classloader/ClassLoaderServiceImpl.java (3)
193-210
: Consider adding comments for clarity.The method
findExportClassLoaderByBiz
has a complex logic that could benefit from additional comments to explain the steps.
264-273
: Consider adding comments for clarity.The method
findExportResourceClassLoadersInOrderByBiz
has a complex logic that could benefit from additional comments to explain the steps.
275-293
: Consider adding comments for clarity.The method
findExportResourcePluginsInOrderByBiz
has a complex logic that could benefit from additional comments to explain the steps.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (14)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (5 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/ArkServiceContainer.java (2 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java (5 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/classloader/AbstractClasspathClassLoader.java (3 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/classloader/ClassLoaderServiceImpl.java (4 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/plugin/PluginFactoryServiceImpl.java (5 hunks)
- sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (7 hunks)
- sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkConfigs.java (1 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java (3 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/PluginOperation.java (1 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/biz/BizFactoryService.java (3 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/classloader/ClassLoaderService.java (3 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/plugin/PluginFactoryService.java (1 hunks)
- sofa-ark-parent/support/ark-springboot-integration/ark-springboot-starter/src/main/java/com/alipay/sofa/ark/springboot/listener/ArkApplicationStartListener.java (3 hunks)
Additional comments not posted (45)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/biz/BizFactoryService.java (2)
66-66
: Verify the usage of the new parameterextensionUrls
.Ensure that the
extensionUrls
parameter is used correctly in the implementation of this method.Verification successful
The
extensionUrls
parameter is used correctly in the implementation of thecreateBiz(File file, URL[] extensionUrls)
method.The parameter is utilized to set the class path for the
BizModel
in theBizFactoryServiceImpl
class.
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new parameter `extensionUrls` in the implementation of `createBiz`. # Test: Search for the method implementation. Expect: Correct usage of `extensionUrls`. rg --type java -A 10 $'Biz createBiz(File file, URL[] extensionUrls)'Length of output: 206
Script:
#!/bin/bash # Description: Verify the usage of the new parameter `extensionUrls` in the implementation of `createBiz`. # Test: Search for the method implementation. Expect: Correct usage of `extensionUrls`. rg --type java -A 10 $'Biz createBiz(File file, URL\\[\\] extensionUrls)'Length of output: 72
Script:
#!/bin/bash # Description: Verify the usage of the new parameter `extensionUrls` in the implementation of `createBiz`. # Test: Search for the method implementation by name and include more lines of context. rg --type java -A 20 'createBiz'Length of output: 120461
49-49
: Verify the usage of the new parameterextensionUrls
.Ensure that the
extensionUrls
parameter is used correctly in the implementation of this method.sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/plugin/PluginFactoryService.java (1)
63-63
: Verify the usage of the new parameterextensions
.Ensure that the
extensions
parameter is used correctly in the implementation of this method.Verification successful
Let's correct the regex pattern and search for the implementation of the
createPlugin(File file, URL[] extensions)
method to verify the usage of theextensions
parameter.
To ensure the correct usage of the
extensions
parameter, we need to trace its usage in thecreatePlugin
method it is passed to.
Let's search for all
createPlugin
method implementations to verify if the method with theextensions
parameter exists and to understand howextensions
is used.
Verify the usage of the new parameter
extensions
.The
extensions
parameter is conditionally used in the implementation of thecreatePlugin
method, influencing the method's behavior based on its presence. This confirms that the parameter is utilized correctly.
PluginFactoryServiceImpl.java
:
createPlugin(File file, URL[] extensions)
: CallscreatePlugin(JarPluginArchive jarPluginArchive, URL[] extensions, Set<String> exportPackages>)
.createPlugin(JarPluginArchive jarPluginArchive, URL[] extensions, Set<String> exportPackages)
: Usesextensions
to determine the method's behavior.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the usage of the new parameter `extensions` in the implementation of `createPlugin`. # Test: Search for the method implementation. Expect: Correct usage of `extensions`. rg --type java -A 10 $'Plugin createPlugin(File file, URL[] extensions)'Length of output: 218
Script:
#!/bin/bash # Description: Verify the usage of the new parameter `extensions` in the implementation of `createPlugin`. # Test: Search for the method implementation. Expect: Correct usage of `extensions`. rg --type java -A 10 'Plugin createPlugin\(File file, URL\[\] extensions\)'Length of output: 3425
Script:
#!/bin/bash # Description: Verify the usage of the `extensions` parameter in the other `createPlugin` method. # Test: Search for the implementation of the other `createPlugin` method. rg --type java -A 10 'createPlugin\(JarPluginArchive jarPluginArchive, URL\[\] extensions, Set<$_>\)'Length of output: 103
Script:
#!/bin/bash # Description: Search for all `createPlugin` method implementations to verify the usage of the `extensions` parameter. # Test: Search for all `createPlugin` method implementations. rg --type java -A 10 'createPlugin'Length of output: 27591
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/classloader/ClassLoaderService.java (2)
95-95
: LGTM!The method
findExportClassLoaderByBiz(Biz biz, String className)
is well-defined and follows the existing patterns.
114-114
: LGTM!The method
findExportResourceClassLoadersInOrderByBiz(Biz biz, String resourceName)
is well-defined and follows the existing patterns.sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkConfigs.java (1)
171-173
: LGTM!The method
areAllPluginsVisibleForBiz()
is well-defined and follows the existing patterns. The implementation correctly retrieves the boolean value from the system properties.sofa-ark-parent/support/ark-springboot-integration/ark-springboot-starter/src/main/java/com/alipay/sofa/ark/springboot/listener/ArkApplicationStartListener.java (2)
76-81
: LGTM!The replacement of
startUpArk(event)
withSofaArkBootstrap.launch(event.getArgs())
unifies the method of launching the SOFA Ark framework, potentially improving maintainability and consistency.
92-103
: LGTM!The update to the
isEmbedEnable
method enhances its efficiency by eliminating unnecessary checks when the system property is already defined. The new logic is correctly implemented.sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/ArkServiceContainer.java (2)
30-30
: New Import: PluginFactoryServiceThe
PluginFactoryService
has been imported, indicating its usage within this class.
96-96
: Enhancement: Integration of PluginFactoryServiceThe
start
method now sets thePluginFactoryService
for theArkClient
, enhancing plugin management capabilities.Ensure that all function calls to
start
are updated to accommodate this change.Verification successful
Enhancement: Integration of PluginFactoryService
The
start
method now sets thePluginFactoryService
for theArkClient
, enhancing plugin management capabilities.Ensure that all function calls to
start
are updated to accommodate this change.
- ArkServiceContainer.java: The main
start
method is updated.- ArkContainer.java: Calls
arkServiceContainer.start()
.- ArkServiceContainerTest.java: Calls
arkServiceContainer.start()
in tests.- PluginDeployServiceImpl.java: Calls
plugin.start()
.These locations should be verified to ensure they align with the new implementation.
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify all function calls to `start` in the codebase. # Test: Search for the function usage. Expect: All occurrences are updated. rg --type java -A 5 $'start()'Length of output: 252226
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/plugin/PluginFactoryServiceImpl.java (3)
117-144
: Improvement: Streamlined Logic in getFinalPluginUrlsThe
getFinalPluginUrls
method has been streamlined to improve readability and control flow. The exclusion of artifacts and addition of extension URLs are now handled more clearly.
155-161
: Enhancement: Overloaded createPlugin MethodThe
createPlugin
method has been overloaded to include a new signature that accepts aFile
and an array ofURL
extensions. This enhances the flexibility of the method.
Line range hint
170-198
:
Improvement: Refined Logic in createEmbedPluginThe
createEmbedPlugin
method has been modified to refine the logic for determining the class loader. The combined check foroverrideExportMode
andenableClassIsolation
improves clarity.sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java (3)
Line range hint
88-110
:
Enhancement: Overloaded createBiz MethodThe
createBiz
method has been overloaded to include a new signature that accepts aBizArchive
and an array ofURL
extension URLs. This enhances the flexibility of the method.
126-131
: New Method: getMergedBizClassPathThe
getMergedBizClassPath
method merges thebizArchiveUrls
andextensionUrls
into a single array using Java Streams. It ensures that if no extension URLs are provided, the originalbizArchiveUrls
are returned unchanged.
133-174
: New Method: resolveExportMapIfNecessaryThe
resolveExportMapIfNecessary
method manages the dependencies of plugins and updates various maps within theBizModel
. It constructs a set of plugins based on visibility configurations and specified dependent plugins, enhancing the export functionality.sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java (5)
116-116
: LGTM!The constant
DEPENDENT_PLUGINS
is appropriately named and defined.
205-205
: LGTM!The constant
PLUGIN_CLASS_ISOLATION_ENABLE
is appropriately named and defined.
206-206
: LGTM!The constant
ALL_PLUGINS_VISIBLE_FOR_BIZ
is appropriately named and defined.
241-241
: LGTM!The constant
CONFIG_INSTALL_PLUGIN_DIR
is appropriately named and defined.
243-243
: LGTM!The constant
BIZ_EXTENSION_URLS
is appropriately named and defined.sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (3)
180-183
: LGTM!The method signature update to include
extensionUrls
as a parameter is appropriate and necessary for the new functionality.
Line range hint
185-192
:
LGTM!The method signature update to include
extensionUrls
as a parameter is appropriate and necessary for the new functionality.
173-177
: LGTM!The method signature update to include
extensionUrls
as a parameter is appropriate and necessary for the new functionality.sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (18)
41-41
: Import statement forPlugin
looks good.The import statement is necessary for the new functionality.
50-50
: Import statement forConcurrentHashMap
looks good.The import statement is necessary for the caching mechanisms.
116-116
: FielddependentPlugins
declaration looks good.The field is correctly declared and will help manage plugin dependencies.
236-238
: MethodgetDependentPlugins
implementation looks good.The method provides access to the
dependentPlugins
field.
240-243
: MethodsetDependentPlugins
implementation looks good.The method allows setting the
dependentPlugins
field.
633-633
: FieldexportClassAndClassLoaderMap
declaration looks good.The field is correctly declared and will help manage class and class loader relationships.
634-634
: FieldexportNodeAndClassLoaderMap
declaration looks good.The field is correctly declared and will help manage node and class loader relationships.
635-635
: FieldexportStemAndClassLoaderMap
declaration looks good.The field is correctly declared and will help manage stem and class loader relationships.
638-638
: FieldexportResourceAndClassLoaderMap
declaration looks good.The field is correctly declared and will help manage resource and class loader relationships.
639-639
: FieldexportPrefixStemResourceAndClassLoaderMap
declaration looks good.The field is correctly declared and will help manage prefix stem resource and class loader relationships.
640-640
: FieldexportSuffixStemResourceAndClassLoaderMap
declaration looks good.The field is correctly declared and will help manage suffix stem resource and class loader relationships.
642-644
: MethodgetExportClassAndClassLoaderMap
implementation looks good.The method provides access to the
exportClassAndClassLoaderMap
field.
646-648
: MethodgetExportNodeAndClassLoaderMap
implementation looks good.The method provides access to the
exportNodeAndClassLoaderMap
field.
650-652
: MethodgetExportStemAndClassLoaderMap
implementation looks good.The method provides access to the
exportStemAndClassLoaderMap
field.
654-656
: MethodgetExportResourceAndClassLoaderMap
implementation looks good.The method provides access to the
exportResourceAndClassLoaderMap
field.
658-660
: MethodgetExportPrefixStemResourceAndClassLoaderMap
implementation looks good.The method provides access to the
exportPrefixStemResourceAndClassLoaderMap
field.
662-664
: MethodgetExportSuffixStemResourceAndClassLoaderMap
implementation looks good.The method provides access to the
exportSuffixStemResourceAndClassLoaderMap
field.
666-684
: Commented-out classPluginExportCache
looks good.The commented-out code is likely for future use or reference.
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/classloader/AbstractClasspathClassLoader.java (3)
460-466
: MethoddoResolveExportClass
implementation looks good.The changes enhance flexibility by differentiating handling based on the class loader type.
564-569
: MethodgetExportResource
implementation looks good.The changes enhance flexibility by differentiating handling based on the class loader type.
641-646
: MethodgetExportResources
implementation looks good.The changes enhance flexibility by differentiating handling based on the class loader type.
// prepare extension urls if necessary | ||
URL[] extensionUrls = null; | ||
if (bizOperation.getParameters().get(Constants.BIZ_EXTENSION_URLS) != null) { | ||
Set<String> extensionLibs = StringUtils.strToSet(bizOperation.getParameters().get(Constants.BIZ_EXTENSION_URLS), Constants.COMMA_SPLIT); | ||
List<URL> urlsList = new ArrayList<>(); | ||
if (!extensionLibs.isEmpty()) { | ||
for (String extension : extensionLibs) { | ||
URL url = new URL(extension); | ||
urlsList.add(url); | ||
} | ||
} | ||
extensionUrls = urlsList.toArray(new URL[0]); | ||
} | ||
return installBiz(bizFile, extensionUrls, args, envs); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding comments for clarity and handling potential exceptions.
The method doInstallOperation
has a complex logic that could benefit from additional comments to explain the steps. Additionally, handle potential exceptions when dealing with URLs.
+ try {
URL url = new URL(extension);
urlsList.add(url);
} catch (MalformedURLException e) {
getLogger().error("Invalid URL: " + extension, e);
}
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
// prepare extension urls if necessary | |
URL[] extensionUrls = null; | |
if (bizOperation.getParameters().get(Constants.BIZ_EXTENSION_URLS) != null) { | |
Set<String> extensionLibs = StringUtils.strToSet(bizOperation.getParameters().get(Constants.BIZ_EXTENSION_URLS), Constants.COMMA_SPLIT); | |
List<URL> urlsList = new ArrayList<>(); | |
if (!extensionLibs.isEmpty()) { | |
for (String extension : extensionLibs) { | |
URL url = new URL(extension); | |
urlsList.add(url); | |
} | |
} | |
extensionUrls = urlsList.toArray(new URL[0]); | |
} | |
return installBiz(bizFile, extensionUrls, args, envs); | |
// prepare extension urls if necessary | |
URL[] extensionUrls = null; | |
if (bizOperation.getParameters().get(Constants.BIZ_EXTENSION_URLS) != null) { | |
Set<String> extensionLibs = StringUtils.strToSet(bizOperation.getParameters().get(Constants.BIZ_EXTENSION_URLS), Constants.COMMA_SPLIT); | |
List<URL> urlsList = new ArrayList<>(); | |
if (!extensionLibs.isEmpty()) { | |
for (String extension : extensionLibs) { | |
try { | |
URL url = new URL(extension); | |
urlsList.add(url); | |
} catch (MalformedURLException e) { | |
getLogger().error("Invalid URL: " + extension, e); | |
} | |
} | |
} | |
extensionUrls = urlsList.toArray(new URL[0]); | |
} | |
return installBiz(bizFile, extensionUrls, args, envs); |
public static ClientResponse installPlugin(PluginOperation pluginOperation) throws Exception { | ||
AssertUtils.assertNotNull(pluginOperation, "pluginOperation must not be null"); | ||
|
||
// prepare plugin file | ||
File localFile = pluginOperation.getLocalFile(); | ||
if (localFile == null && !StringUtils.isEmpty(pluginOperation.getUrl())) { | ||
URL url = new URL(pluginOperation.getUrl()); | ||
String pluginDir = ArkConfigs.getStringValue(Constants.CONFIG_INSTALL_PLUGIN_DIR); | ||
File pluginDirectory = StringUtils.isEmpty(pluginDir) ? FileUtils | ||
.createTempDir("sofa-ark") : FileUtils.mkdir(pluginDir); | ||
localFile = new File(pluginDirectory, pluginOperation.getPluginName() + "-" | ||
+ pluginOperation.getPluginVersion() + "-" | ||
+ System.currentTimeMillis()); | ||
try (InputStream inputStream = url.openStream()) { | ||
FileUtils.copyInputStreamToFile(inputStream, localFile); | ||
} | ||
} | ||
|
||
// prepare extension urls if necessary | ||
List<String> extensionLibs = pluginOperation.getExtensionLibs(); | ||
List<URL> urlsList = new ArrayList<>(); | ||
if (extensionLibs != null && !extensionLibs.isEmpty()) { | ||
for (String extension : extensionLibs) { | ||
URL url = new URL(extension); | ||
urlsList.add(url); | ||
} | ||
} | ||
URL[] extensionUrls = urlsList.toArray(new URL[0]); | ||
|
||
long start = System.currentTimeMillis(); | ||
SimpleDateFormat sdf = new SimpleDateFormat("HH:mm:ss,SSS"); | ||
String startDate = sdf.format(new Date(start)); | ||
|
||
// create | ||
Plugin plugin = pluginFactoryService.createPlugin(localFile, extensionUrls); | ||
// register | ||
pluginManagerService.registerPlugin(plugin); | ||
// start | ||
ClientResponse response = new ClientResponse(); | ||
try { | ||
plugin.start(); | ||
long end = System.currentTimeMillis(); | ||
response.setCode(ResponseCode.SUCCESS).setMessage( | ||
String.format("Install Plugin: %s success, cost: %s ms, started at: %s", | ||
plugin.getPluginName() + ":" + plugin.getVersion(), end - start, startDate)); | ||
getLogger().info(response.getMessage()); | ||
} catch (Throwable throwable) { | ||
long end = System.currentTimeMillis(); | ||
response.setCode(ResponseCode.FAILED).setMessage( | ||
String.format("Install Plugin: %s fail,cost: %s ms, started at: %s", | ||
plugin.getPluginName() + ":" + plugin.getVersion(), end - start, startDate)); | ||
getLogger().error(response.getMessage(), throwable); | ||
throw throwable; | ||
} | ||
return response; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding comments for clarity and handling potential exceptions.
The method installPlugin
has a complex logic that could benefit from additional comments to explain the steps. Additionally, handle potential exceptions when dealing with URLs.
+ try {
URL url = new URL(extension);
urlsList.add(url);
} catch (MalformedURLException e) {
getLogger().error("Invalid URL: " + extension, e);
}
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
public static ClientResponse installPlugin(PluginOperation pluginOperation) throws Exception { | |
AssertUtils.assertNotNull(pluginOperation, "pluginOperation must not be null"); | |
// prepare plugin file | |
File localFile = pluginOperation.getLocalFile(); | |
if (localFile == null && !StringUtils.isEmpty(pluginOperation.getUrl())) { | |
URL url = new URL(pluginOperation.getUrl()); | |
String pluginDir = ArkConfigs.getStringValue(Constants.CONFIG_INSTALL_PLUGIN_DIR); | |
File pluginDirectory = StringUtils.isEmpty(pluginDir) ? FileUtils | |
.createTempDir("sofa-ark") : FileUtils.mkdir(pluginDir); | |
localFile = new File(pluginDirectory, pluginOperation.getPluginName() + "-" | |
+ pluginOperation.getPluginVersion() + "-" | |
+ System.currentTimeMillis()); | |
try (InputStream inputStream = url.openStream()) { | |
FileUtils.copyInputStreamToFile(inputStream, localFile); | |
} | |
} | |
// prepare extension urls if necessary | |
List<String> extensionLibs = pluginOperation.getExtensionLibs(); | |
List<URL> urlsList = new ArrayList<>(); | |
if (extensionLibs != null && !extensionLibs.isEmpty()) { | |
for (String extension : extensionLibs) { | |
URL url = new URL(extension); | |
urlsList.add(url); | |
} | |
} | |
URL[] extensionUrls = urlsList.toArray(new URL[0]); | |
long start = System.currentTimeMillis(); | |
SimpleDateFormat sdf = new SimpleDateFormat("HH:mm:ss,SSS"); | |
String startDate = sdf.format(new Date(start)); | |
// create | |
Plugin plugin = pluginFactoryService.createPlugin(localFile, extensionUrls); | |
// register | |
pluginManagerService.registerPlugin(plugin); | |
// start | |
ClientResponse response = new ClientResponse(); | |
try { | |
plugin.start(); | |
long end = System.currentTimeMillis(); | |
response.setCode(ResponseCode.SUCCESS).setMessage( | |
String.format("Install Plugin: %s success, cost: %s ms, started at: %s", | |
plugin.getPluginName() + ":" + plugin.getVersion(), end - start, startDate)); | |
getLogger().info(response.getMessage()); | |
} catch (Throwable throwable) { | |
long end = System.currentTimeMillis(); | |
response.setCode(ResponseCode.FAILED).setMessage( | |
String.format("Install Plugin: %s fail,cost: %s ms, started at: %s", | |
plugin.getPluginName() + ":" + plugin.getVersion(), end - start, startDate)); | |
getLogger().error(response.getMessage(), throwable); | |
throw throwable; | |
} | |
return response; | |
} | |
public static ClientResponse installPlugin(PluginOperation pluginOperation) throws Exception { | |
AssertUtils.assertNotNull(pluginOperation, "pluginOperation must not be null"); | |
// prepare plugin file | |
File localFile = pluginOperation.getLocalFile(); | |
if (localFile == null && !StringUtils.isEmpty(pluginOperation.getUrl())) { | |
URL url = new URL(pluginOperation.getUrl()); | |
String pluginDir = ArkConfigs.getStringValue(Constants.CONFIG_INSTALL_PLUGIN_DIR); | |
File pluginDirectory = StringUtils.isEmpty(pluginDir) ? FileUtils | |
.createTempDir("sofa-ark") : FileUtils.mkdir(pluginDir); | |
localFile = new File(pluginDirectory, pluginOperation.getPluginName() + "-" | |
+ pluginOperation.getPluginVersion() + "-" | |
+ System.currentTimeMillis()); | |
try (InputStream inputStream = url.openStream()) { | |
FileUtils.copyInputStreamToFile(inputStream, localFile); | |
} | |
} | |
// prepare extension urls if necessary | |
List<String> extensionLibs = pluginOperation.getExtensionLibs(); | |
List<URL> urlsList = new ArrayList<>(); | |
if (extensionLibs != null && !extensionLibs.isEmpty()) { | |
for (String extension : extensionLibs) { | |
try { | |
URL url = new URL(extension); | |
urlsList.add(url); | |
} catch (MalformedURLException e) { | |
getLogger().error("Invalid URL: " + extension, e); | |
} | |
} | |
} | |
URL[] extensionUrls = urlsList.toArray(new URL[0]); | |
long start = System.currentTimeMillis(); | |
SimpleDateFormat sdf = new SimpleDateFormat("HH:mm:ss,SSS"); | |
String startDate = sdf.format(new Date(start)); | |
// create | |
Plugin plugin = pluginFactoryService.createPlugin(localFile, extensionUrls); | |
// register | |
pluginManagerService.registerPlugin(plugin); | |
// start | |
ClientResponse response = new ClientResponse(); | |
try { | |
plugin.start(); | |
long end = System.currentTimeMillis(); | |
response.setCode(ResponseCode.SUCCESS).setMessage( | |
String.format("Install Plugin: %s success, cost: %s ms, started at: %s", | |
plugin.getPluginName() + ":" + plugin.getVersion(), end - start, startDate)); | |
getLogger().info(response.getMessage()); | |
} catch (Throwable throwable) { | |
long end = System.currentTimeMillis(); | |
response.setCode(ResponseCode.FAILED).setMessage( | |
String.format("Install Plugin: %s fail,cost: %s ms, started at: %s", | |
plugin.getPluginName() + ":" + plugin.getVersion(), end - start, startDate)); | |
getLogger().error(response.getMessage(), throwable); | |
throw throwable; | |
} | |
return response; | |
} |
|
||
private String pluginName; | ||
|
||
private String pluginVersion; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding validation for pluginVersion
.
Ensure that the pluginVersion
is not null or empty.
+ public void setPluginVersion(String pluginVersion) {
+ if (pluginVersion == null || pluginVersion.trim().isEmpty()) {
+ throw new IllegalArgumentException("Plugin version cannot be null or empty");
+ }
+ this.pluginVersion = pluginVersion;
+ }
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
private String pluginVersion; | |
private String pluginVersion; | |
public void setPluginVersion(String pluginVersion) { | |
if (pluginVersion == null || pluginVersion.trim().isEmpty()) { | |
throw new IllegalArgumentException("Plugin version cannot be null or empty"); | |
} | |
this.pluginVersion = pluginVersion; | |
} |
|
||
private File localFile; | ||
|
||
private List<String> extensionLibs; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding validation for extensionLibs
.
Ensure that the extensionLibs
list is not null.
+ public void setExtensionLibs(List<String> extensionLibs) {
+ if (extensionLibs == null) {
+ throw new IllegalArgumentException("Extension libraries list cannot be null");
+ }
+ this.extensionLibs = extensionLibs;
+ }
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
private List<String> extensionLibs; | |
private List<String> extensionLibs; | |
public void setExtensionLibs(List<String> extensionLibs) { | |
if (extensionLibs == null) { | |
throw new IllegalArgumentException("Extension libraries list cannot be null"); | |
} | |
this.extensionLibs = extensionLibs; | |
} |
|
||
private OperationType operationType; | ||
|
||
private String url; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding validation for url
.
Ensure that the url
is a valid URL format.
+ public void setUrl(String url) {
+ try {
+ new URL(url);
+ } catch (MalformedURLException e) {
+ throw new IllegalArgumentException("Invalid URL format");
+ }
+ this.url = url;
+ }
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
private String url; | |
private String url; | |
public void setUrl(String url) { | |
try { | |
new URL(url); | |
} catch (MalformedURLException e) { | |
throw new IllegalArgumentException("Invalid URL format"); | |
} | |
this.url = url; | |
} |
|
||
public class PluginOperation { | ||
|
||
private String pluginName; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Consider adding validation for pluginName
.
Ensure that the pluginName
is not null or empty.
+ public void setPluginName(String pluginName) {
+ if (pluginName == null || pluginName.trim().isEmpty()) {
+ throw new IllegalArgumentException("Plugin name cannot be null or empty");
+ }
+ this.pluginName = pluginName;
+ }
Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
private String pluginName; | |
private String pluginName; | |
public void setPluginName(String pluginName) { | |
if (pluginName == null || pluginName.trim().isEmpty()) { | |
throw new IllegalArgumentException("Plugin name cannot be null or empty"); | |
} | |
this.pluginName = pluginName; | |
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (5 hunks)
Files skipped from review as they are similar to previous changes (1)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java
2.biz的dependent-plugins参数通过params传入 3.plugin支持安装的时候传入name 4.BizFactoryServiceImpl#getBizUcp 这里构造biz的ucp的时候 把所有plugin都add进来了 这里应该是只需要加载biz依赖的plugin 5.ArkClient提供一个checkplugin/biz是否存在的接口
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (10)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java (5 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/plugin/PluginFactoryServiceImpl.java (5 hunks)
- sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (4 hunks)
- sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (7 hunks)
- sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ClientResponse.java (3 hunks)
- sofa-ark-parent/core/exception/src/main/java/com/alipay/sofa/ark/exception/ArkLoaderException.java (1 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizConfig.java (1 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/PluginConfig.java (1 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/biz/BizFactoryService.java (3 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/plugin/PluginFactoryService.java (2 hunks)
Additional comments not posted (50)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/PluginConfig.java (6)
22-24
: LGTM!The getter method is correctly implemented.
The code changes are approved.
26-28
: LGTM!The setter method is correctly implemented.
The code changes are approved.
30-32
: LGTM!The getter method is correctly implemented.
The code changes are approved.
34-36
: LGTM!The setter method is correctly implemented.
The code changes are approved.
38-40
: LGTM!The getter method is correctly implemented.
The code changes are approved.
42-44
: LGTM!The setter method is correctly implemented.
The code changes are approved.
sofa-ark-parent/core/exception/src/main/java/com/alipay/sofa/ark/exception/ArkLoaderException.java (3)
27-28
: LGTM!The constructor modification improves exception handling by storing the throwable in a private field.
The code changes are approved.
Also applies to: 34-35
41-43
: LGTM!The method is correctly implemented to avoid performance issues.
The code changes are approved.
44-46
: LGTM!The method is correctly implemented to retrieve the underlying cause of the exception.
The code changes are approved.
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizConfig.java (10)
31-33
: LGTM!The getter method is correctly implemented.
The code changes are approved.
35-37
: LGTM!The setter method is correctly implemented.
The code changes are approved.
39-41
: LGTM!The getter method is correctly implemented.
The code changes are approved.
43-45
: LGTM!The setter method is correctly implemented.
The code changes are approved.
47-49
: LGTM!The getter method is correctly implemented.
The code changes are approved.
51-53
: LGTM!The setter method is correctly implemented.
The code changes are approved.
55-57
: LGTM!The getter method is correctly implemented.
The code changes are approved.
59-61
: LGTM!The setter method is correctly implemented.
The code changes are approved.
63-65
: LGTM!The getter method is correctly implemented.
The code changes are approved.
67-69
: LGTM!The setter method is correctly implemented.
The code changes are approved.
sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ClientResponse.java (3)
35-35
: LGTM!The new private field
pluginInfos
is correctly added.The code changes are approved.
64-66
: LGTM!The new public method
getPluginInfos()
is correctly implemented.The code changes are approved.
68-71
: LGTM!The new public method
setPluginInfos(Set<Plugin> pluginInfos)
is correctly implemented.The code changes are approved.
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/plugin/PluginFactoryService.java (2)
64-65
: LGTM! But verify the method implementation.The new overloaded method
createPlugin(File file, URL[] extensions)
is correctly added. Ensure that the method is properly implemented in the corresponding class.Run the following script to verify the method implementation:
Verification successful
Method Implementation Verified
The
createPlugin(File file, URL[] extensions)
method is correctly implemented in thePluginFactoryServiceImpl
class. The implementation aligns with the expected functionality of handling plugin creation using the provided file and extensions.
- Location:
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/plugin/PluginFactoryServiceImpl.java
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the new overloaded method `createPlugin(File file, URL[] extensions)`. # Test: Search for the method implementation. Expect: The method should be implemented in the corresponding class. rg --type java -A 10 $'createPlugin(File file, URL[] extensions)'Length of output: 197
Script:
#!/bin/bash # Description: Verify the implementation of the new overloaded method `createPlugin(File file, URL[] extensions)`. # Test: Search for the method implementation. Expect: The method should be implemented in the corresponding class. rg --type java -A 10 'createPlugin\(File file, URL\[\] extensions\)'Length of output: 3618
66-67
: LGTM! But verify the method implementation.The new overloaded method
createPlugin(File file, PluginConfig pluginConfig)
is correctly added. Ensure that the method is properly implemented in the corresponding class.Run the following script to verify the method implementation:
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/service/biz/BizFactoryService.java (4)
90-91
: LGTM! But verify the method implementation.The new overloaded method
createBiz(BizArchive bizArchive, BizConfig bizConfig)
is correctly added. Ensure that the method is properly implemented in the corresponding class.Run the following script to verify the method implementation:
50-50
: LGTM! But verify the method implementation.The new overloaded method
createBiz(BizArchive bizArchive, URL[] extensionUrls)
is correctly added. Ensure that the method is properly implemented in the corresponding class.Run the following script to verify the method implementation:
Verification successful
Method Implementation Verified
The method
createBiz(BizArchive bizArchive, URL[] extensionUrls)
is correctly implemented in theBizFactoryServiceImpl
class. The implementation sets the extension URLs in aBizConfig
object and utilizes it to create aBiz
instance.
- Location:
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the new overloaded method `createBiz(BizArchive bizArchive, URL[] extensionUrls)`. # Test: Search for the method implementation. Expect: The method should be implemented in the corresponding class. rg --type java -A 10 $'createBiz(BizArchive bizArchive, URL[] extensionUrls)'Length of output: 230
Script:
#!/bin/bash # Description: Verify the implementation of the new overloaded method `createBiz(BizArchive bizArchive, URL[] extensionUrls)`. # Test: Search for the method implementation. Expect: The method should be implemented in the corresponding class. rg --type java -A 10 $'createBiz(BizArchive bizArchive, URL\\[\\] extensionUrls)'Length of output: 80
Script:
#!/bin/bash # Description: Search for any method named `createBiz` to verify its implementation. # Test: Search for any method named `createBiz`. Expect: Find implementations or variations of the method. rg --type java -A 10 'createBiz'Length of output: 87168
82-83
: LGTM! But verify the method implementation.The new overloaded method
createBiz(File file, BizConfig bizConfig)
is correctly added. Ensure that the method is properly implemented in the corresponding class.Run the following script to verify the method implementation:
Verification successful
Method Implementation Verified
The
createBiz(File file, BizConfig bizConfig)
method is correctly implemented in theBizFactoryServiceImpl
class, fulfilling the interface contract. No further action is needed.Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the new overloaded method `createBiz(File file, BizConfig bizConfig)`. # Test: Search for the method implementation. Expect: The method should be implemented in the corresponding class. rg --type java -A 10 $'createBiz(File file, BizConfig bizConfig)'Length of output: 66
Script:
#!/bin/bash # Find the class implementing the BizFactoryService interface ast-grep --lang java --pattern 'class $_ implements BizFactoryService { $$$ }'Length of output: 80
Script:
#!/bin/bash # Search for references to BizFactoryService to find potential implementing classes or usage rg --type java 'BizFactoryService'Length of output: 8729
Script:
#!/bin/bash # Search for the implementation of the createBiz method in BizFactoryServiceImpl ast-grep --lang java --pattern 'class BizFactoryServiceImpl { $$$ createBiz(File file, BizConfig bizConfig) { $$$ } $$$ }'Length of output: 124
Script:
#!/bin/bash # Search for any implementation of the createBiz(File file, BizConfig bizConfig) method in the codebase rg --type java 'createBiz\(File file, BizConfig bizConfig\)'Length of output: 427
67-67
: LGTM! But verify the method implementation.The new overloaded method
createBiz(File file, URL[] extensionUrls)
is correctly added. Ensure that the method is properly implemented in the corresponding class.Run the following script to verify the method implementation:
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/plugin/PluginFactoryServiceImpl.java (5)
123-129
: LGTM!The function is correctly implemented and utilizes the
extensions
parameter as expected.The code changes are approved.
131-161
: LGTM!The function is correctly implemented and utilizes the
pluginConfig
parameter as expected.The code changes are approved.
165-175
: LGTM!The function is correctly implemented and sets the attributes as expected.
The code changes are approved.
204-235
: LGTM!The function is correctly implemented and handles the URL construction as expected.
The code changes are approved.
Line range hint
204-241
: LGTM!The function is correctly implemented and handles the export mappings as expected.
The code changes are approved.
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java (7)
86-91
: LGTM!The function is correctly implemented and utilizes the
extensionUrls
parameter as expected.The code changes are approved.
100-105
: LGTM!The function is correctly implemented and utilizes the
extensionUrls
parameter as expected.The code changes are approved.
108-113
: LGTM!The function is correctly implemented and utilizes the
bizOperation
parameter as expected.The code changes are approved.
116-119
: LGTM!The function is correctly implemented and utilizes the
bizConfig
parameter as expected.The code changes are approved.
122-160
: LGTM!The function is correctly implemented and sets the attributes as expected.
The code changes are approved.
165-175
: LGTM!The function is correctly implemented and sets the attributes as expected.
The code changes are approved.
197-201
: LGTM!The function is correctly implemented and handles the URL merging as expected.
The code changes are approved.
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (3)
261-261
: LGTM!The function is correctly implemented and utilizes the
BizConfig
parameter as expected.The code changes are approved.
310-310
: LGTM!The function is correctly implemented and utilizes the
BizConfig
parameter as expected.The code changes are approved.
325-325
: LGTM!The function is correctly implemented and utilizes the
BizConfig
parameter as expected.The code changes are approved.
sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (7)
121-123
: LGTM!The method is correctly implemented.
The code changes are approved.
125-127
: LGTM!The method is correctly implemented.
The code changes are approved.
183-185
: LGTM!The refactoring enhances the encapsulation of configuration details related to business installations.
The code changes are approved.
188-193
: LGTM!The update ensures that the installation process is more coherent and maintainable.
The code changes are approved.
402-421
: LGTM!The update adds robustness to the method by preparing extension URLs if necessary.
The code changes are approved.
445-509
: LGTM!The method correctly handles the installation of plugins, including error handling and logging.
The code changes are approved.
511-540
: LGTM!The method correctly handles the verification of plugins.
The code changes are approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (7)
- pom.xml (1 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java (6 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/classloader/AbstractClasspathClassLoader.java (3 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/plugin/PluginFactoryServiceImpl.java (5 hunks)
- sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (7 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizConfig.java (1 hunks)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/PluginConfig.java (1 hunks)
Files skipped from review due to trivial changes (1)
- pom.xml
Files skipped from review as they are similar to previous changes (2)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/BizConfig.java
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/model/PluginConfig.java
Additional comments not posted (20)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java (8)
83-84
: LGTM!The method is correctly implemented.
The code changes are approved.
87-91
: LGTM!The method is correctly implemented.
The code changes are approved.
94-97
: LGTM!The method is correctly implemented.
The code changes are approved.
100-105
: LGTM!The method is correctly implemented.
The code changes are approved.
108-113
: LGTM!The method is correctly implemented.
The code changes are approved.
116-119
: LGTM!The method is correctly implemented.
The code changes are approved.
Line range hint
122-165
: LGTM!The method is correctly implemented and enhances configurability.
The code changes are approved.
170-180
: LGTM!The method is correctly implemented.
The code changes are approved.
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/plugin/PluginFactoryServiceImpl.java (3)
124-129
: LGTM!The method is correctly implemented.
The code changes are approved.
132-169
: LGTM!The method is correctly implemented and enhances configurability.
The code changes are approved.
211-242
: LGTM!The method is correctly implemented and the changes improve clarity and functionality.
The code changes are approved.
sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (6)
183-184
: LGTM!The method is correctly implemented and enhances configurability.
The code changes are approved.
Line range hint
187-214
: LGTM!The method is correctly implemented and enhances configurability.
The code changes are approved.
401-421
: LGTM!The method is correctly implemented and enhances flexibility.
The code changes are approved.
445-509
: LGTM!The method is correctly implemented and enhances flexibility.
The code changes are approved.
511-513
: LGTM!The method is correctly implemented.
The code changes are approved.
515-540
: LGTM!The method is correctly implemented.
The code changes are approved.
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/classloader/AbstractClasspathClassLoader.java (3)
460-466
: LGTM!The conditional logic for
BizClassLoader
andPluginClassLoader
improves the accuracy of class resolution.The code changes are approved.
564-575
: LGTM!The conditional logic for
BizClassLoader
andPluginClassLoader
improves the accuracy of resource resolution.The code changes are approved.
644-652
: LGTM!The conditional logic for
BizClassLoader
andPluginClassLoader
improves the accuracy of resource resolution.The code changes are approved.
# Conflicts: # pom.xml
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (1)
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java (3 hunks)
Additional comments not posted (5)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java (5)
116-116
: LGTM!The constant
DEPENDENT_PLUGINS
is correctly defined and follows the naming conventions.The code changes are approved.
205-205
: LGTM!The constant
PLUGIN_CLASS_ISOLATION_ENABLE
is correctly defined and follows the naming conventions.The code changes are approved.
206-206
: LGTM!The constant
ALL_PLUGINS_VISIBLE_FOR_BIZ
is correctly defined and follows the naming conventions.The code changes are approved.
241-241
: LGTM!The constant
CONFIG_INSTALL_PLUGIN_DIR
is correctly defined and follows the naming conventions.The code changes are approved.
243-243
: LGTM!The constant
BIZ_EXTENSION_URLS
is correctly defined and follows the naming conventions.The code changes are approved.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java (6 hunks)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/test/TestClassLoader.java (1 hunks)
- sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (4 hunks)
- sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceTest.java (2 hunks)
- sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/classloader/BizClassLoaderTest.java (9 hunks)
- sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/classloader/ClassLoaderHookTest.java (2 hunks)
Additional comments not posted (24)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceTest.java (2)
36-36
: LGTM!Importing all constants from the
Constants
class can enhance readability and reduce the need for individual constant imports.
71-71
: LGTM!Setting the system property
ALL_PLUGINS_VISIBLE_FOR_BIZ
toBoolean.TRUE.toString()
likely affects the visibility of plugins for the business context during the test execution. This change could influence the behavior of the plugin registration process, ensuring that all plugins are accessible when the business is created and registered.sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/test/TestClassLoader.java (1)
70-70
: LGTM!The code change is approved. Invoking the superclass's method for setting the business model is important for maintaining the integrity of the class hierarchy and ensuring that the
BizModel
is properly initialized in the context of the parent class.sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/classloader/ClassLoaderHookTest.java (3)
22-22
: LGTM!The new import statements are required to use the
BizManagerService
from theArkServiceContainerHolder
in the test.Also applies to: 26-26
57-59
: LGTM!The code changes are correctly using the
BizManagerService
to set theBizModel
on thebizClassLoader
. This allows the test to interact with the business model, which is crucial for validating the behavior of the class loader in the context of the business logic.
Line range hint
1-21
: SkippedThe code is unchanged and does not require a review.
Also applies to: 23-25, 27-56, 60-148
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java (5)
Line range hint
121-167
: LGTM!The code changes in the
createBiz(BizArchive bizArchive, BizConfig bizConfig)
method look good:
- The null check on
bizConfig
ensures that it's not null before using it.- The merging of the
BizArchive
's URLs and the extension URLs allows for extending the classpath.- The resolution of the export map based on the dependent plugins specified in the
BizConfig
or the manifest provides flexibility in specifying the dependencies.
170-180
: LGTM!The
createEmbedMasterBiz(ClassLoader masterClassLoader)
method looks good:
- It simplifies the process of creating an embedded master
Biz
instance by encapsulating the setup logic and using predefined attributes.- The attributes are set to reasonable defaults.
- It uses the provided
masterClassLoader
for theClassPath
andClassLoader
.
Line range hint
182-199
: LGTM!The
prepareBizArchive(File file)
method looks good:
- It handles the creation of the appropriate
BizArchive
based on theisEmbedEnable
configuration.- If embedding is enabled, it unpacks the
File
and creates anExplodedBizArchive
.- If embedding is disabled, it creates a
JarBizArchive
from theFile
.
202-206
: LGTM!The
getMergedBizClassPath(URL[] bizArchiveUrls, URL[] extensionUrls)
method looks good:
- It handles the merging of the
bizArchiveUrls
andextensionUrls
to create a merged classpath.- If the
extensionUrls
isnull
or empty, it returns thebizArchiveUrls
as is, avoiding unnecessary operations.- The use of
Stream.concat
provides a concise way to concatenate the arrays.
209-246
: LGTM!The
resolveExportMapIfNecessary(BizModel bizModel, List<String> dependentPlugins)
method looks good:
- It handles the resolution of the export map of the
BizModel
based on the dependent plugins.- The behavior is controlled by the
areAllPluginsVisibleForBiz
configuration and thedependentPlugins
list.- It populates the various export maps of the
BizModel
based on the export information of each dependent plugin.sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (6)
25-25
: LGTM!The newly added import statement for
BizConfig
class is approved.
258-258
: LGTM!The updated mocked behavior of
bizFactoryServiceMock
to expect aBizConfig
object is approved.
261-261
: LGTM!The updated method invocation of
installBiz
with aBizConfig
object is approved.
302-302
: LGTM!The updated mocked behavior of
bizFactoryServiceMock
to expect aBizConfig
object is approved.
310-310
: LGTM!The updated method invocation of
installBiz
with aBizConfig
object is approved.
325-325
: LGTM!The updated method invocation of
installBiz
with aBizConfig
object is approved.sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/classloader/BizClassLoaderTest.java (7)
105-106
: LGTM!The code changes are approved.
174-179
: LGTM!The code changes are approved.
271-273
: LGTM!The code changes are approved.
354-361
: LGTM!The code changes are approved.
376-377
: LGTM!The code changes are approved.
425-425
: LGTM!The code changes are approved.
439-441
: LGTM!The code changes are approved.
...container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java
Outdated
Show resolved
Hide resolved
} | ||
|
||
bizModel.setDependentPlugins(plugins); | ||
for (Plugin plugin : plugins) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
所以每个 plugin,是需要定义自己的 export 内容的,这部分需要在使用文档里说明。
sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkConfigs.java
Outdated
Show resolved
Hide resolved
...container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java
Show resolved
Hide resolved
sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java
Show resolved
Hide resolved
...re-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java
Show resolved
Hide resolved
...ain/java/com/alipay/sofa/ark/container/service/classloader/AbstractClasspathClassLoader.java
Show resolved
Hide resolved
...container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java
Show resolved
Hide resolved
...arter/src/main/java/com/alipay/sofa/ark/springboot/listener/ArkApplicationStartListener.java
Outdated
Show resolved
Hide resolved
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
This pull request has been automatically closed because it has not had activity in the last 37 days. Please feel free to give a status update now, ping for review, or re-open when it's ready. Thank you for your contributions! |
# Conflicts: # sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java # sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java # sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceTest.java # sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (12)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java (4)
34-34
: Consider avoiding wildcard imports.
Wildcard imports can sometimes introduce unintended class conflicts or reduce clarity.
48-48
: LinkedList usage check.
Using LinkedList often doesn't provide significant benefit over ArrayList unless frequent insertions at the head or removals in the middle are required. If random access or mostly append operations are expected, consider using ArrayList for better performance.
86-92
: Similar code duplication.
createBiz(File, URL[])
mirrorscreateBiz(File)
with extension URLs. Consider consolidating repeated logic into a helper method to adhere to DRY.- public Biz createBiz(File file, URL[] extensionUrls) throws IOException { - BizArchive bizArchive = prepareBizArchive(file); - BizConfig bizConfig = new BizConfig(); - bizConfig.setExtensionUrls(extensionUrls); - return createBiz(bizArchive, bizConfig); - } + // Consider a single doCreateBiz(File, BizConfig) helper that both methods call
Line range hint
176-195
: Potential partial failure in file handling.
IfFileUtils.unzip()
orfile.delete()
fails, partial or corrupted archives could remain. Consider robust error handling or rollbacks to prevent stale data.try { unpackFile = FileUtils.unzip(file, file.getAbsolutePath() + "-unpack"); } catch (IOException e) { // handle partial failures, log or revert }sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (5)
28-28
: Wildcard import note.
Consider using explicit imports to maintain clarity on which classes are used.
35-35
: Separate dedicated service.
PluginFactoryService
might be better injected rather than added as another static field, to avoid global state.
44-44
: General utilities.
Importing the entirejava.util.*
might be too broad—consider explicit imports if that aligns with the project style.
59-59
: Global static field caution.
StoringPluginFactoryService
in a static field can complicate concurrency and testing.
122-129
: Add javadoc or usage doc for new plugin factory service.
These getters and setters are fine, but clarifying their usage context would help other contributors.sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/classloader/BizClassLoaderTest.java (1)
31-32
: Wildcard import fromcom.alipay.sofa.ark.spi.model.*
.
Consider explicit imports for clarity, if it aligns with style.sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (2)
685-689
: Review thread-safety of the stored lists.
While the map itself is concurrent, storing a plainList<Plugin>
is not inherently safe for concurrent modifications. Consider wrapping them in a thread-safe collection or ensuring that only one thread mutates them at a time.
690-713
: Consider providing unmodifiable views for these maps.
Returning the rawConcurrentHashMap
allows external code to modify it directly, potentially undermining concurrency guarantees or invariants. Consider returning read-only or defensive copies.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java
(5 hunks)sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java
(6 hunks)sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java
(4 hunks)sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/classloader/BizClassLoaderTest.java
(9 hunks)sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java
(7 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java
(3 hunks)
🔇 Additional comments (48)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java (12)
52-52
: Stream import.
TheStream
usage to merge arrays is an appropriate choice here.
70-71
: Simple delegate to overloaded method.
This is a good pattern to reduce code duplication.
72-78
: Maintain consistent approach to extensionUrls.
Storing extension URLs inBizConfig
keeps the logic uniform across overloads.
80-84
: Leverage error handling fromprepareBizArchive
.
No issues found. The forwarded call is concise and clear.
94-100
: Ensure correct biz version.
createBiz(BizOperation, File)
sets the version frombizOperation
. This helps keep the config consistent.
102-106
: Straightforward delegation to main creation method.
No issues found.
164-174
: Embed master biz defaults.
Defaulting version to "1.0.0" and main class to "embed main" is intuitive for embedded scenarios.
198-202
: Merging class paths.
TheStream.concat
approach is clear and concise.
205-244
: Null checks for plugins.
A previous review already flagged a potential issue ifgetPluginByName(pluginName)
returns null.
268-270
: Combining biz classpath with plugin URLs.
Implementation is straightforward and aligns with the existing architecture.
275-277
: Safe retrieval of plugin URLs.
This method properly iterates overbizModel.getDependentPlugins()
. Watch out if any plugin might have a null URL, but that seems unlikely in normal usage.
Line range hint
108-159
: Validate potential missing attributes in the manifest.
Currently, the code sets the biz name, version, etc. from the manifest. If the manifest lacks these attributes, they become null. Consider verifying or falling back to defaults to avoid potential issues.+ String arkBizName = manifestMainAttributes.getValue(ARK_BIZ_NAME); + if (StringUtils.isEmpty(arkBizName)) { + // Provide a fallback or raise an informative exception + }sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (11)
39-39
: IOException import.
No issues here; required forinstallBiz
and plugin operations.
41-41
: HandleMalformedURLException
.
Be mindful that URLs may be malformed. The code throwsThrowable
, so it’s at least surfaced upstream.
172-174
: Overload returninginstallBiz(bizFile, args, null)
.
A clean approach to default the environment map tonull
.
178-181
: ConstructingBizConfig
.
Packaging arguments and envs intoBizConfig
ensures consistent usage in downstream calls.
184-185
: A direct pass-through forBizConfig
.
No issues found—keeps code DRY.
188-192
: EnsurebizConfig
is never null.
The added assertion clarifies the contract that a valid config is expected.
207-207
: Propagatingbiz.start(..)
errors.
The try/catch ensures rollback if startup fails. Good approach.
400-415
: Parsing extension URLs.
Consider error handling around invalid or unreachable URLs.
417-421
: Comprehensive configuration.
Storing extension URLs, arguments, and envs inBizConfig
ensures a unified approach for biz installation.
445-509
: Plugin installation flow.
The dynamic plugin install approach looks correct. Consider verifying plugin file integrity or signature if security is a concern.
511-540
: Plugin check.
Provides a helpful summary of installed plugins. Good for debug/troubleshooting.sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java (4)
116-116
: New constant DEPENDENT_PLUGINS.
Facilitates referencing the plugin dependency list from manifests or config.
209-210
: New constants for plugin/biz dependency config.
These added config flags support advanced plugin management scenarios.
263-263
: Specify plugin installation directory.
Allows custom plugin directory control.
265-265
: Extension URLs key.
BIZ_EXTENSION_URLS
is straightforward naming for additional library references.sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (6)
32-32
: UsingBizConfig
import.
Ensures test coverage for the new config usage.
294-294
: MockingcreateBiz(File, BizConfig)
.
Valid approach for verifying error handling.
297-297
: Install with a new BizConfig.
Ensures the test checks the path whereBizConfig
is explicitly provided.
372-372
: Repeated mock forbizFactoryServiceMock
.
Confirms consistent usage in error-handling scenarios.
380-380
: Testing failure with a new BizConfig.
Temporal logic verifying exception flow is correct.
395-395
: Verifying uninstall behavior when auto-uninstall is disabled.
Test is explicit, ensuring coverage for that configuration path.sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/classloader/BizClassLoaderTest.java (9)
23-23
: Importing FileUtils.
Used for potential file ops in test scenarios.
46-49
: Static imports.
Shortens call sites fornewArrayList
andcurrentThread
. Consistent with typical test patterns.
105-106
: Registering plugin exports.
PuttingITest
in the export node map ensures test code correctness.
174-179
: Multiple plugin exports.
Checking class and resource loading from multiple plugins is a robust testing strategy.
271-273
: Additional override test class.
Demonstrates how the biz class loader references plugin B’s class exports.
354-361
: PopulatedependentPlugins
and resource maps for denial tests.
Ensures thebizClassLoader
can handle blacklisted resources correctly.
376-376
: Denied class check.
Verifies that the plugin class loader is no longer used for a certain class if it’s denied.
425-425
: Validating resource block.
Null
result ensures correct deny logic.
439-441
: Associating the BizModel.
Ensures the customBizClassLoader
references the correctBizModel
for resource and class resolution.sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (6)
42-42
: No issues identified with the new import statement.
53-53
: Additional utility collections look good.
121-122
: Consider thread safety fordependentPlugins
.
If multiple threads can access or modify this set at runtime, you should use a thread-safe collection or introduce proper synchronization.
241-243
: Return unmodifiable set ingetDependentPlugins()
.
Exposing the internal mutable set allows external code to modify theBizModel
state without control. Return an unmodifiable set to preserve encapsulation.
245-248
: Make a defensive copy insetDependentPlugins()
.
Assigning the provided set directly lets callers unknowingly mutate theBizModel
. Creating a new set instance foils unintended side effects.
680-684
: Good use ofConcurrentHashMap
for concurrency.
The use of a thread-safe map is appropriate for storing relationships that may be read and written by multiple threads.
This reverts commit 0bbd4e3.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #984 +/- ##
============================================
- Coverage 78.11% 77.86% -0.26%
- Complexity 877 910 +33
============================================
Files 172 175 +3
Lines 7078 7376 +298
Branches 1041 1082 +41
============================================
+ Hits 5529 5743 +214
- Misses 1017 1082 +65
- Partials 532 551 +19 ☔ View full report in Codecov by Sentry. |
...container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java
Show resolved
Hide resolved
...container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.java
Show resolved
Hide resolved
@@ -409,6 +442,103 @@ public static ClientResponse checkOperation(BizOperation bizOperation) { | |||
return checkBiz(bizOperation.getBizName(), bizOperation.getBizVersion()); | |||
} | |||
|
|||
public static ClientResponse installPlugin(PluginOperation pluginOperation) throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
安装和卸载 plugin,没看到单测覆盖呢
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 2
♻️ Duplicate comments (1)
sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (1)
403-415
:⚠️ Potential issueAdd URL validation and exception handling.
The code processes URLs without proper validation or exception handling, which could lead to runtime errors.
Apply this diff to add proper validation and error handling:
if (bizOperation.getParameters().get(Constants.BIZ_EXTENSION_URLS) != null) { Set<String> extensionLibs = StringUtils.strToSet( bizOperation.getParameters().get(Constants.BIZ_EXTENSION_URLS), Constants.COMMA_SPLIT); List<URL> urlsList = new ArrayList<>(); if (!extensionLibs.isEmpty()) { for (String extension : extensionLibs) { + if (StringUtils.isEmpty(extension)) { + continue; + } + try { URL url = new URL(extension); urlsList.add(url); + } catch (MalformedURLException e) { + getLogger().error("Invalid extension URL: " + extension, e); + throw new IllegalArgumentException("Invalid extension URL: " + extension, e); + } } } extensionUrls = urlsList.toArray(new URL[0]); }
🧹 Nitpick comments (2)
sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (2)
28-28
: Consider using explicit imports instead of wildcard imports.Replace
import com.alipay.sofa.ark.spi.model.*;
with explicit imports for better maintainability and to avoid potential naming conflicts.
178-181
: Add validation for BizConfig parameters.While the code creates a new BizConfig object, it doesn't validate the args and envs before setting them. Consider adding validation to ensure data integrity.
BizConfig bizConfig = new BizConfig(); +if (args != null) { + for (String arg : args) { + AssertUtils.assertNotNull(arg, "Biz argument must not be null"); + } +} bizConfig.setArgs(args); bizConfig.setEnvs(envs);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java
(8 hunks)sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java
(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: build
- GitHub Check: build
- GitHub Check: sca
🔇 Additional comments (3)
sofa-ark-parent/core/api/src/main/java/com/alipay/sofa/ark/api/ArkClient.java (3)
59-59
: LGTM!The plugin factory service implementation follows the established pattern of other service fields in the class.
Also applies to: 122-128
445-445
: Add test coverage for plugin installation.The plugin installation functionality lacks test coverage, as noted in previous comments.
Run this script to verify test coverage:
#!/bin/bash # Description: Check for test coverage of plugin installation methods rg -l "test.*Plugin.*Install" ast-grep --pattern 'class $TestClass { $$$ test$Method($$$) { $$$ installPlugin($$$) $$$ } $$$ }'
482-487
:⚠️ Potential issueAdd URL validation for extension libraries.
Similar to the BizConfig extension URLs, plugin extension URLs lack proper validation and error handling.
if (extensionLibs != null && !extensionLibs.isEmpty()) { for (String extension : extensionLibs) { + if (StringUtils.isEmpty(extension)) { + continue; + } + try { URL url = new URL(extension); urlsList.add(url); + } catch (MalformedURLException e) { + getLogger().error("Invalid extension URL: " + extension, e); + throw new IllegalArgumentException("Invalid extension URL: " + extension, e); + } } }Likely invalid or redundant comment.
try (InputStream inputStream = url.openStream()) { | ||
FileUtils.copyInputStreamToFile(inputStream, localFile); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Ensure proper resource cleanup.
The code uses try-with-resources for the InputStream but doesn't handle cleanup of the temporary file in case of failures.
+boolean success = false;
try (InputStream inputStream = url.openStream()) {
FileUtils.copyInputStreamToFile(inputStream, localFile);
+ success = true;
+} finally {
+ if (!success && localFile != null && localFile.exists()) {
+ localFile.delete();
+ }
}
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
try (InputStream inputStream = url.openStream()) { | |
FileUtils.copyInputStreamToFile(inputStream, localFile); | |
} | |
} | |
boolean success = false; | |
try (InputStream inputStream = url.openStream()) { | |
FileUtils.copyInputStreamToFile(inputStream, localFile); | |
success = true; | |
} finally { | |
if (!success && localFile != null && localFile.exists()) { | |
localFile.delete(); | |
} | |
} | |
} |
PluginConfig pluginConfig = new PluginConfig(); | ||
if (!StringUtils.isEmpty(pluginOperation.getPluginName())) { | ||
pluginConfig.setSpecifiedName(pluginOperation.getPluginName()); | ||
} | ||
if (!StringUtils.isEmpty(pluginOperation.getPluginVersion())) { | ||
pluginConfig.setSpecifiedVersion(pluginOperation.getPluginVersion()); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Refactor suggestion
Validate plugin configuration before processing.
The code sets plugin configuration without validating the name and version format.
PluginConfig pluginConfig = new PluginConfig();
if (!StringUtils.isEmpty(pluginOperation.getPluginName())) {
+ AssertUtils.assertNotNull(pluginOperation.getPluginName().matches("^[a-zA-Z0-9._-]+$"),
+ "Plugin name contains invalid characters");
pluginConfig.setSpecifiedName(pluginOperation.getPluginName());
}
if (!StringUtils.isEmpty(pluginOperation.getPluginVersion())) {
+ AssertUtils.assertNotNull(pluginOperation.getPluginVersion().matches("^\\d+\\.\\d+(\\.\\d+)?(-.*)?$"),
+ "Plugin version format is invalid");
pluginConfig.setSpecifiedVersion(pluginOperation.getPluginVersion());
}
Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (2)
32-32
: Consider using explicit imports instead of wildcard imports.Using wildcard imports can make it harder to track specific model dependencies and may lead to naming conflicts. Consider listing the specific model classes being used.
-import com.alipay.sofa.ark.spi.model.*; +import com.alipay.sofa.ark.spi.model.Biz; +import com.alipay.sofa.ark.spi.model.BizInfo; +import com.alipay.sofa.ark.spi.model.BizOperation; +import com.alipay.sofa.ark.spi.model.PluginOperation;
452-470
: Enhance plugin installation test coverage.While the test covers basic scenarios, consider these improvements:
- Add descriptive messages to assertions
- Add cleanup after test execution
- Test additional scenarios (e.g., version conflicts, invalid plugin format)
@Test public void testInstallPlugin() throws Throwable { PluginOperation pluginOperation = new PluginOperation(); pluginOperation.setPluginName("plugin-demo"); ClientResponse clientResponse = installPlugin(pluginOperation); - assertEquals(FAILED, clientResponse.getCode()); + assertEquals("Installation should fail when local file is missing", FAILED, clientResponse.getCode()); pluginOperation.setLocalFile(new File(samplePlugin.toURI())); try { installPlugin(pluginOperation); } catch (Exception exception) { - assertTrue(exception instanceof ArkRuntimeException); + assertTrue("Should throw ArkRuntimeException for invalid plugin", + exception instanceof ArkRuntimeException); } clientResponse = checkPlugin(); - assertEquals(1, clientResponse.getPluginInfos().size()); + assertEquals("One plugin should be registered", 1, clientResponse.getPluginInfos().size()); clientResponse = checkPlugin("plugin-demo"); - assertEquals(1, clientResponse.getPluginInfos().size()); + assertEquals("Plugin should be findable by name", 1, clientResponse.getPluginInfos().size()); + + // Test additional scenarios + // Test version conflict + pluginOperation.setPluginVersion("1.0.0"); + // ... add more test cases + + // Cleanup + try { + uninstallPlugin("plugin-demo"); + } finally { + // Ensure cleanup even if uninstall fails + } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java
(8 hunks)
🔇 Additional comments (1)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (1)
281-284
: LGTM! Test methods properly updated for BizConfig.The test methods have been consistently updated to use BizConfig, maintaining proper error handling and assertions.
Also applies to: 359-367, 382-382
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
…ions (#984) * plugin management for flink poc * support dynamic install plugin * fix SofaArkBootstrap.launch * support dynamic plugin installation * fix plugin export when finding export class * support all plugin are visible for biz * support dynamic biz classpath extension * rm unuse code * ArkLoaderException.cause cache,not use ClassNotFoundException.ex field * getBizUcp只加载依赖的plugin * 1.biz安装支持多版本,版本从params传入,默认使用pom文件中的版本 2.biz的dependent-plugins参数通过params传入 3.plugin支持安装的时候传入name 4.BizFactoryServiceImpl#getBizUcp 这里构造biz的ucp的时候 把所有plugin都add进来了 这里应该是只需要加载biz依赖的plugin 5.ArkClient提供一个checkplugin/biz是否存在的接口 * format * fix ClassLoaderTest * fix ut * fix code review * 默认biz可见全部plugins,sofa.ark.biz.specify.dependent.plugins.enable=true时,指定可见范围 * format * Revert "fix SofaArkBootstrap.launch" This reverts commit 0bbd4e3. * fix ut * format * format --------- Co-authored-by: yuanyuan <[email protected]> (cherry picked from commit d1c277f)
* Module deployments are deduplicated when static merge deployments(#338) (#1035) * Module deployments are deduplicated when static merge deployments(#338) (cherry picked from commit 41fe026) * Support plugin management and dynamic plugin installation with extensions (#984) * plugin management for flink poc * support dynamic install plugin * fix SofaArkBootstrap.launch * support dynamic plugin installation * fix plugin export when finding export class * support all plugin are visible for biz * support dynamic biz classpath extension * rm unuse code * ArkLoaderException.cause cache,not use ClassNotFoundException.ex field * getBizUcp只加载依赖的plugin * 1.biz安装支持多版本,版本从params传入,默认使用pom文件中的版本 2.biz的dependent-plugins参数通过params传入 3.plugin支持安装的时候传入name 4.BizFactoryServiceImpl#getBizUcp 这里构造biz的ucp的时候 把所有plugin都add进来了 这里应该是只需要加载biz依赖的plugin 5.ArkClient提供一个checkplugin/biz是否存在的接口 * format * fix ClassLoaderTest * fix ut * fix code review * 默认biz可见全部plugins,sofa.ark.biz.specify.dependent.plugins.enable=true时,指定可见范围 * format * Revert "fix SofaArkBootstrap.launch" This reverts commit 0bbd4e3. * fix ut * format * format --------- Co-authored-by: yuanyuan <[email protected]> (cherry picked from commit d1c277f) * update to 3.1.10 * update to 3.1.10 * update test --------- Co-authored-by: Will <[email protected]> Co-authored-by: yuanyuancin <[email protected]>
fix koupleless/koupleless#383
Summary by CodeRabbit
Summary by CodeRabbit
New Features
createBiz
methods allowing for additional parameters and configurations.BizConfig
for more dynamicBiz
creation.Biz
instances with new methods.BizModel
.Bug Fixes
Biz
creation process with improved error handling and logging.