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 (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
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
findExportClassLoaderByBizhas a complex logic that could benefit from additional comments to explain the steps.
264-273: Consider adding comments for clarity.The method
findExportResourceClassLoadersInOrderByBizhas a complex logic that could benefit from additional comments to explain the steps.
275-293: Consider adding comments for clarity.The method
findExportResourcePluginsInOrderByBizhas 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
extensionUrlsparameter is used correctly in the implementation of this method.Verification successful
The
extensionUrlsparameter is used correctly in the implementation of thecreateBiz(File file, URL[] extensionUrls)method.The parameter is utilized to set the class path for the
BizModelin theBizFactoryServiceImplclass.
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.javaScripts 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
extensionUrlsparameter 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
extensionsparameter 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 theextensionsparameter.
To ensure the correct usage of the
extensionsparameter, we need to trace its usage in thecreatePluginmethod it is passed to.
Let's search for all
createPluginmethod implementations to verify if the method with theextensionsparameter exists and to understand howextensionsis used.
Verify the usage of the new parameter
extensions.The
extensionsparameter is conditionally used in the implementation of thecreatePluginmethod, 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): Usesextensionsto 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
isEmbedEnablemethod 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
PluginFactoryServicehas been imported, indicating its usage within this class.
96-96: Enhancement: Integration of PluginFactoryServiceThe
startmethod now sets thePluginFactoryServicefor theArkClient, enhancing plugin management capabilities.Ensure that all function calls to
startare updated to accommodate this change.Verification successful
Enhancement: Integration of PluginFactoryService
The
startmethod now sets thePluginFactoryServicefor theArkClient, enhancing plugin management capabilities.Ensure that all function calls to
startare updated to accommodate this change.
- ArkServiceContainer.java: The main
startmethod 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
getFinalPluginUrlsmethod 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
createPluginmethod has been overloaded to include a new signature that accepts aFileand an array ofURLextensions. This enhances the flexibility of the method.
Line range hint
170-198:
Improvement: Refined Logic in createEmbedPluginThe
createEmbedPluginmethod has been modified to refine the logic for determining the class loader. The combined check foroverrideExportModeandenableClassIsolationimproves 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
createBizmethod has been overloaded to include a new signature that accepts aBizArchiveand an array ofURLextension URLs. This enhances the flexibility of the method.
126-131: New Method: getMergedBizClassPathThe
getMergedBizClassPathmethod merges thebizArchiveUrlsandextensionUrlsinto a single array using Java Streams. It ensures that if no extension URLs are provided, the originalbizArchiveUrlsare returned unchanged.
133-174: New Method: resolveExportMapIfNecessaryThe
resolveExportMapIfNecessarymethod 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_PLUGINSis appropriately named and defined.
205-205: LGTM!The constant
PLUGIN_CLASS_ISOLATION_ENABLEis appropriately named and defined.
206-206: LGTM!The constant
ALL_PLUGINS_VISIBLE_FOR_BIZis appropriately named and defined.
241-241: LGTM!The constant
CONFIG_INSTALL_PLUGIN_DIRis appropriately named and defined.
243-243: LGTM!The constant
BIZ_EXTENSION_URLSis 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
extensionUrlsas a parameter is appropriate and necessary for the new functionality.
Line range hint
185-192:
LGTM!The method signature update to include
extensionUrlsas a parameter is appropriate and necessary for the new functionality.
173-177: LGTM!The method signature update to include
extensionUrlsas 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 forPluginlooks good.The import statement is necessary for the new functionality.
50-50: Import statement forConcurrentHashMaplooks good.The import statement is necessary for the caching mechanisms.
116-116: FielddependentPluginsdeclaration looks good.The field is correctly declared and will help manage plugin dependencies.
236-238: MethodgetDependentPluginsimplementation looks good.The method provides access to the
dependentPluginsfield.
240-243: MethodsetDependentPluginsimplementation looks good.The method allows setting the
dependentPluginsfield.
633-633: FieldexportClassAndClassLoaderMapdeclaration looks good.The field is correctly declared and will help manage class and class loader relationships.
634-634: FieldexportNodeAndClassLoaderMapdeclaration looks good.The field is correctly declared and will help manage node and class loader relationships.
635-635: FieldexportStemAndClassLoaderMapdeclaration looks good.The field is correctly declared and will help manage stem and class loader relationships.
638-638: FieldexportResourceAndClassLoaderMapdeclaration looks good.The field is correctly declared and will help manage resource and class loader relationships.
639-639: FieldexportPrefixStemResourceAndClassLoaderMapdeclaration looks good.The field is correctly declared and will help manage prefix stem resource and class loader relationships.
640-640: FieldexportSuffixStemResourceAndClassLoaderMapdeclaration looks good.The field is correctly declared and will help manage suffix stem resource and class loader relationships.
642-644: MethodgetExportClassAndClassLoaderMapimplementation looks good.The method provides access to the
exportClassAndClassLoaderMapfield.
646-648: MethodgetExportNodeAndClassLoaderMapimplementation looks good.The method provides access to the
exportNodeAndClassLoaderMapfield.
650-652: MethodgetExportStemAndClassLoaderMapimplementation looks good.The method provides access to the
exportStemAndClassLoaderMapfield.
654-656: MethodgetExportResourceAndClassLoaderMapimplementation looks good.The method provides access to the
exportResourceAndClassLoaderMapfield.
658-660: MethodgetExportPrefixStemResourceAndClassLoaderMapimplementation looks good.The method provides access to the
exportPrefixStemResourceAndClassLoaderMapfield.
662-664: MethodgetExportSuffixStemResourceAndClassLoaderMapimplementation looks good.The method provides access to the
exportSuffixStemResourceAndClassLoaderMapfield.
666-684: Commented-out classPluginExportCachelooks 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: MethoddoResolveExportClassimplementation looks good.The changes enhance flexibility by differentiating handling based on the class loader type.
564-569: MethodgetExportResourceimplementation looks good.The changes enhance flexibility by differentiating handling based on the class loader type.
641-646: MethodgetExportResourcesimplementation 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.
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.
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.
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.
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.
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.
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.
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.
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
pluginInfosis 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 thePluginFactoryServiceImplclass. 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.javaScripts 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 theBizFactoryServiceImplclass. The implementation sets the extension URLs in aBizConfigobject and utilizes it to create aBizinstance.
- Location:
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/service/biz/BizFactoryServiceImpl.javaScripts 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 theBizFactoryServiceImplclass, 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
extensionsparameter as expected.The code changes are approved.
131-161: LGTM!The function is correctly implemented and utilizes the
pluginConfigparameter 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
extensionUrlsparameter as expected.The code changes are approved.
100-105: LGTM!The function is correctly implemented and utilizes the
extensionUrlsparameter as expected.The code changes are approved.
108-113: LGTM!The function is correctly implemented and utilizes the
bizOperationparameter as expected.The code changes are approved.
116-119: LGTM!The function is correctly implemented and utilizes the
bizConfigparameter 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
BizConfigparameter as expected.The code changes are approved.
310-310: LGTM!The function is correctly implemented and utilizes the
BizConfigparameter as expected.The code changes are approved.
325-325: LGTM!The function is correctly implemented and utilizes the
BizConfigparameter 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.
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
BizClassLoaderandPluginClassLoaderimproves the accuracy of class resolution.The code changes are approved.
564-575: LGTM!The conditional logic for
BizClassLoaderandPluginClassLoaderimproves the accuracy of resource resolution.The code changes are approved.
644-652: LGTM!The conditional logic for
BizClassLoaderandPluginClassLoaderimproves the accuracy of resource resolution.The code changes are approved.
# Conflicts: # pom.xml
There was a problem hiding this comment.
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_PLUGINSis correctly defined and follows the naming conventions.The code changes are approved.
205-205: LGTM!The constant
PLUGIN_CLASS_ISOLATION_ENABLEis correctly defined and follows the naming conventions.The code changes are approved.
206-206: LGTM!The constant
ALL_PLUGINS_VISIBLE_FOR_BIZis correctly defined and follows the naming conventions.The code changes are approved.
241-241: LGTM!The constant
CONFIG_INSTALL_PLUGIN_DIRis correctly defined and follows the naming conventions.The code changes are approved.
243-243: LGTM!The constant
BIZ_EXTENSION_URLSis correctly defined and follows the naming conventions.The code changes are approved.
There was a problem hiding this comment.
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
Constantsclass can enhance readability and reduce the need for individual constant imports.
71-71: LGTM!Setting the system property
ALL_PLUGINS_VISIBLE_FOR_BIZtoBoolean.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
BizModelis 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
BizManagerServicefrom theArkServiceContainerHolderin the test.Also applies to: 26-26
57-59: LGTM!The code changes are correctly using the
BizManagerServiceto set theBizModelon 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
bizConfigensures 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
BizConfigor 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
Bizinstance by encapsulating the setup logic and using predefined attributes.- The attributes are set to reasonable defaults.
- It uses the provided
masterClassLoaderfor theClassPathandClassLoader.
Line range hint
182-199: LGTM!The
prepareBizArchive(File file)method looks good:
- It handles the creation of the appropriate
BizArchivebased on theisEmbedEnableconfiguration.- If embedding is enabled, it unpacks the
Fileand creates anExplodedBizArchive.- If embedding is disabled, it creates a
JarBizArchivefrom theFile.
202-206: LGTM!The
getMergedBizClassPath(URL[] bizArchiveUrls, URL[] extensionUrls)method looks good:
- It handles the merging of the
bizArchiveUrlsandextensionUrlsto create a merged classpath.- If the
extensionUrlsisnullor empty, it returns thebizArchiveUrlsas is, avoiding unnecessary operations.- The use of
Stream.concatprovides 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
BizModelbased on the dependent plugins.- The behavior is controlled by the
areAllPluginsVisibleForBizconfiguration and thedependentPluginslist.- It populates the various export maps of the
BizModelbased 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
BizConfigclass is approved.
258-258: LGTM!The updated mocked behavior of
bizFactoryServiceMockto expect aBizConfigobject is approved.
261-261: LGTM!The updated method invocation of
installBizwith aBizConfigobject is approved.
302-302: LGTM!The updated mocked behavior of
bizFactoryServiceMockto expect aBizConfigobject is approved.
310-310: LGTM!The updated method invocation of
installBizwith aBizConfigobject is approved.
325-325: LGTM!The updated method invocation of
installBizwith aBizConfigobject 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.
所以每个 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.
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.
PluginFactoryServicemight 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.
StoringPluginFactoryServicein 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 rawConcurrentHashMapallows 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.
TheStreamusage 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 inBizConfigkeeps 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.concatapproach 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 forinstallBizand 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 intoBizConfigensures consistent usage in downstream calls.
184-185: A direct pass-through forBizConfig.
No issues found—keeps code DRY.
188-192: EnsurebizConfigis 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 inBizConfigensures 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_URLSis 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: UsingBizConfigimport.
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 whereBizConfigis 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 fornewArrayListandcurrentThread. Consistent with typical test patterns.
105-106: Registering plugin exports.
PuttingITestin 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: PopulatedependentPluginsand resource maps for denial tests.
Ensures thebizClassLoadercan 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.
Nullresult ensures correct deny logic.
439-441: Associating the BizModel.
Ensures the customBizClassLoaderreferences the correctBizModelfor 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 theBizModelstate 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 ofConcurrentHashMapfor 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.
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.
🛠️ 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.
🛠️ 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.
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
…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
createBizmethods allowing for additional parameters and configurations.BizConfigfor more dynamicBizcreation.Bizinstances with new methods.BizModel.Bug Fixes
Bizcreation process with improved error handling and logging.