Skip to content
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

Merged
merged 25 commits into from
Jan 9, 2025

Conversation

yuanyuancin
Copy link
Collaborator

@yuanyuancin yuanyuancin commented Aug 7, 2024

fix koupleless/koupleless#383

  1. support plugin management for biz.
  2. support dynamic plugin installation in runtime.
  3. support biz/plugin installation in runtime with extension params.
  4. fix enforcement for embed.

Summary by CodeRabbit

Summary by CodeRabbit

  • New Features

    • Introduced flexible createBiz methods allowing for additional parameters and configurations.
    • Enhanced handling of BizConfig for more dynamic Biz creation.
    • Improved class path management for Biz instances with new methods.
    • Added new constants for enhanced plugin management and configuration.
    • Introduced methods for installing and checking plugins.
    • Added functionality to manage plugin dependencies within the BizModel.
    • Enhanced test functionality for plugin installation and resource handling.
  • Bug Fixes

    • Enhanced robustness of the Biz creation process with improved error handling and logging.
    • Improved test coverage for plugin management and resource handling.

Copy link
Contributor

coderabbitai bot commented Aug 7, 2024

Walkthrough

The recent updates to the SOFA Ark framework include significant enhancements to the BizModel, BizFactoryServiceImpl, and ArkClient classes, focusing on plugin management and business configurations. The BizModel class now includes new fields and methods for managing plugin dependencies and export relationships. The BizFactoryServiceImpl has introduced multiple overloaded methods for creating Biz instances with enhanced configurability, while the ArkClient class has been refactored to streamline plugin installation and management processes. Additionally, new constants have been added to support these functionalities.

Changes

File(s) Change Summary
.../container/model/BizModel.java Added fields and methods for managing plugin dependencies and class loader mappings.
.../service/biz/BizFactoryServiceImpl.java Overloaded createBiz methods to accept BizConfig and URL[] extensionUrls for enhanced configurability.
.../service/biz/BizFactoryServiceImpl.java Introduced new method createEmbedMasterBiz and modified prepareBizArchive to streamline archive preparation.
.../api/ArkClient.java Refactored installBiz to accept a BizConfig object and added methods for plugin management.
.../spi/constant/Constants.java Introduced new constant strings for plugin management and configuration.
.../test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java Updated test methods to utilize BizConfig in installBiz calls.
.../test/java/com/alipay/sofa/ark/container/service/classloader/BizClassLoaderTest.java Enhanced tests to verify interactions between plugins and the BizModel.
.../springboot/listener/ArkApplicationStartListener.java Added import for Constants, expanding class dependencies.

Assessment against linked issues

Objective Addressed Explanation
Support offline computation capabilities (#383) No changes related to offline computation were made.

Possibly related PRs

  • add biz jar url in biz module #928: This PR modifies the BizModel class by adding a new private field bizUrl and corresponding getter/setter methods, which is related to the changes in the main PR that enhance the BizModel class with new fields and methods for managing plugin dependencies and export relationships.
  • 统一静态合并部署 #949: This PR includes changes to the BizFactoryServiceImpl class that involve the createBiz method, which is also modified in the main PR to accommodate new methods that interact with the BizModel, indicating a direct relationship between the two.

Suggested labels

size/XL

🐰 In the meadow, changes take flight,
Biz creation now feels just right!
With configs and paths that intertwine,
Ark's magic grows, oh how it shines!
Hopping through code, we celebrate,
New beginnings await, oh what a fate! 🌼


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?

❤️ Share
🪧 Tips

Chat

There are 3 ways to chat with CodeRabbit:

  • Review comments: Directly reply to a review comment made by CodeRabbit. Example:
    • I pushed a fix in commit <commit_id>, please review it.
    • Generate unit testing code for this file.
    • Open a follow-up GitHub issue for this discussion.
  • Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
    • @coderabbitai generate unit testing code for this file.
    • @coderabbitai modularize this function.
  • PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
    • @coderabbitai gather interesting stats about this repository and render them as a table. Additionally, render a pie chart showing the language distribution in the codebase.
    • @coderabbitai read src/utils.ts and generate unit testing code.
    • @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
    • @coderabbitai help me debug CodeRabbit configuration file.

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)

  • @coderabbitai pause to pause the reviews on a PR.
  • @coderabbitai resume to resume the paused reviews.
  • @coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
  • @coderabbitai full review to do a full review from scratch and review all the files again.
  • @coderabbitai summary to regenerate the summary of the PR.
  • @coderabbitai generate docstrings to generate docstrings for this PR. (Beta)
  • @coderabbitai resolve resolve all the CodeRabbit review comments.
  • @coderabbitai configuration to show the current CodeRabbit configuration for the repository.
  • @coderabbitai help to get help.

Other keywords and placeholders

  • Add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.
  • Add @coderabbitai summary to generate the high-level summary at a specific location in the PR description.
  • Add @coderabbitai anywhere in the PR title to generate the title automatically.

CodeRabbit Configuration File (.coderabbit.yaml)

  • You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
  • Please see the configuration documentation for more information.
  • If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

  • Visit our Documentation for detailed information on how to use CodeRabbit.
  • Join our Discord Community to get help, request features, and share feedback.
  • Follow us on X/Twitter for updates and announcements.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 61e8a4f and 4ee8415.

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 parameter extensionUrls.

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 the createBiz(File file, URL[] extensionUrls) method.

The parameter is utilized to set the class path for the BizModel in the BizFactoryServiceImpl 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 parameter extensionUrls.

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 parameter extensions.

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 the extensions parameter.


To ensure the correct usage of the extensions parameter, we need to trace its usage in the createPlugin method it is passed to.


Let's search for all createPlugin method implementations to verify if the method with the extensions parameter exists and to understand how extensions is used.


Verify the usage of the new parameter extensions.

The extensions parameter is conditionally used in the implementation of the createPlugin 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): Calls createPlugin(JarPluginArchive jarPluginArchive, URL[] extensions, Set<String> exportPackages>).
    • createPlugin(JarPluginArchive jarPluginArchive, URL[] extensions, Set<String> exportPackages): Uses extensions 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) with SofaArkBootstrap.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: PluginFactoryService

The PluginFactoryService has been imported, indicating its usage within this class.


96-96: Enhancement: Integration of PluginFactoryService

The start method now sets the PluginFactoryService for the ArkClient, 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 the PluginFactoryService for the ArkClient, 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 getFinalPluginUrls

The 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 Method

The createPlugin method has been overloaded to include a new signature that accepts a File and an array of URL extensions. This enhances the flexibility of the method.


Line range hint 170-198:
Improvement: Refined Logic in createEmbedPlugin

The createEmbedPlugin method has been modified to refine the logic for determining the class loader. The combined check for overrideExportMode and enableClassIsolation 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 Method

The createBiz method has been overloaded to include a new signature that accepts a BizArchive and an array of URL extension URLs. This enhances the flexibility of the method.


126-131: New Method: getMergedBizClassPath

The getMergedBizClassPath method merges the bizArchiveUrls and extensionUrls into a single array using Java Streams. It ensures that if no extension URLs are provided, the original bizArchiveUrls are returned unchanged.


133-174: New Method: resolveExportMapIfNecessary

The resolveExportMapIfNecessary method manages the dependencies of plugins and updates various maps within the BizModel. 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 for Plugin looks good.

The import statement is necessary for the new functionality.


50-50: Import statement for ConcurrentHashMap looks good.

The import statement is necessary for the caching mechanisms.


116-116: Field dependentPlugins declaration looks good.

The field is correctly declared and will help manage plugin dependencies.


236-238: Method getDependentPlugins implementation looks good.

The method provides access to the dependentPlugins field.


240-243: Method setDependentPlugins implementation looks good.

The method allows setting the dependentPlugins field.


633-633: Field exportClassAndClassLoaderMap declaration looks good.

The field is correctly declared and will help manage class and class loader relationships.


634-634: Field exportNodeAndClassLoaderMap declaration looks good.

The field is correctly declared and will help manage node and class loader relationships.


635-635: Field exportStemAndClassLoaderMap declaration looks good.

The field is correctly declared and will help manage stem and class loader relationships.


638-638: Field exportResourceAndClassLoaderMap declaration looks good.

The field is correctly declared and will help manage resource and class loader relationships.


639-639: Field exportPrefixStemResourceAndClassLoaderMap declaration looks good.

The field is correctly declared and will help manage prefix stem resource and class loader relationships.


640-640: Field exportSuffixStemResourceAndClassLoaderMap declaration looks good.

The field is correctly declared and will help manage suffix stem resource and class loader relationships.


642-644: Method getExportClassAndClassLoaderMap implementation looks good.

The method provides access to the exportClassAndClassLoaderMap field.


646-648: Method getExportNodeAndClassLoaderMap implementation looks good.

The method provides access to the exportNodeAndClassLoaderMap field.


650-652: Method getExportStemAndClassLoaderMap implementation looks good.

The method provides access to the exportStemAndClassLoaderMap field.


654-656: Method getExportResourceAndClassLoaderMap implementation looks good.

The method provides access to the exportResourceAndClassLoaderMap field.


658-660: Method getExportPrefixStemResourceAndClassLoaderMap implementation looks good.

The method provides access to the exportPrefixStemResourceAndClassLoaderMap field.


662-664: Method getExportSuffixStemResourceAndClassLoaderMap implementation looks good.

The method provides access to the exportSuffixStemResourceAndClassLoaderMap field.


666-684: Commented-out class PluginExportCache 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: Method doResolveExportClass implementation looks good.

The changes enhance flexibility by differentiating handling based on the class loader type.


564-569: Method getExportResource implementation looks good.

The changes enhance flexibility by differentiating handling based on the class loader type.


641-646: Method getExportResources implementation looks good.

The changes enhance flexibility by differentiating handling based on the class loader type.

Comment on lines 399 to 412
// 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);
Copy link
Contributor

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.

Suggested change
// 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);

Comment on lines 436 to 491
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;
}
Copy link
Contributor

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.

Suggested change
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;
Copy link
Contributor

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.

Suggested change
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;
Copy link
Contributor

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.

Suggested change
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;
Copy link
Contributor

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.

Suggested change
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;
Copy link
Contributor

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.

Suggested change
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;
}

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 4ee8415 and 58ca02a.

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是否存在的接口
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 58ca02a and 84c8548.

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 the PluginFactoryServiceImpl 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 the BizFactoryServiceImpl class. The implementation sets the extension URLs in a BizConfig object and utilizes it to create a Biz 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 the BizFactoryServiceImpl 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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 84c8548 and f81c2be.

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 and PluginClassLoader improves the accuracy of class resolution.

The code changes are approved.


564-575: LGTM!

The conditional logic for BizClassLoader and PluginClassLoader improves the accuracy of resource resolution.

The code changes are approved.


644-652: LGTM!

The conditional logic for BizClassLoader and PluginClassLoader improves the accuracy of resource resolution.

The code changes are approved.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between f81c2be and 0e2645b.

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.

@sofastack-cla sofastack-cla bot added size/XXL and removed size/XL labels Sep 2, 2024
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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

Commits

Files that changed from the base of the PR and between 0e2645b and c293814.

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 to Boolean.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 the ArkServiceContainerHolder in the test.

Also applies to: 26-26


57-59: LGTM!

The code changes are correctly using the BizManagerService to set the BizModel on the bizClassLoader. 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: Skipped

The 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 the ClassPath and ClassLoader.

Line range hint 182-199: LGTM!

The prepareBizArchive(File file) method looks good:

  • It handles the creation of the appropriate BizArchive based on the isEmbedEnable configuration.
  • If embedding is enabled, it unpacks the File and creates an ExplodedBizArchive.
  • If embedding is disabled, it creates a JarBizArchive from the File.

202-206: LGTM!

The getMergedBizClassPath(URL[] bizArchiveUrls, URL[] extensionUrls) method looks good:

  • It handles the merging of the bizArchiveUrls and extensionUrls to create a merged classpath.
  • If the extensionUrls is null or empty, it returns the bizArchiveUrls 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 the dependentPlugins 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 a BizConfig object is approved.


261-261: LGTM!

The updated method invocation of installBiz with a BizConfig object is approved.


302-302: LGTM!

The updated mocked behavior of bizFactoryServiceMock to expect a BizConfig object is approved.


310-310: LGTM!

The updated method invocation of installBiz with a BizConfig object is approved.


325-325: LGTM!

The updated method invocation of installBiz with a BizConfig 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.

pom.xml Outdated Show resolved Hide resolved
}

bizModel.setDependentPlugins(plugins);
for (Plugin plugin : plugins) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

所以每个 plugin,是需要定义自己的 export 内容的,这部分需要在使用文档里说明。

@github-actions github-actions bot closed this Nov 1, 2024
@lvjing2 lvjing2 reopened this Nov 1, 2024
Copy link

github-actions bot commented Nov 9, 2024

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!

@github-actions github-actions bot closed this Nov 9, 2024
@lvjing2 lvjing2 reopened this Nov 19, 2024
Copy link

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!

@github-actions github-actions bot closed this Nov 27, 2024
@lvjing2 lvjing2 reopened this Nov 29, 2024
Copy link

github-actions bot commented Dec 7, 2024

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!

@github-actions github-actions bot closed this Dec 7, 2024
@lvjing2 lvjing2 reopened this Dec 17, 2024
Copy link

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!

@github-actions github-actions bot closed this Dec 25, 2024
# 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
@yuanyuancin yuanyuancin reopened this Jan 3, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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[]) mirrors createBiz(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.
If FileUtils.unzip() or file.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 entire java.util.* might be too broad—consider explicit imports if that aligns with the project style.


59-59: Global static field caution.
Storing PluginFactoryService 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 from com.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 plain List<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 raw ConcurrentHashMap 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

📥 Commits

Reviewing files that changed from the base of the PR and between 792c726 and f6eb235.

📒 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.
The Stream 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 in BizConfig keeps the logic uniform across overloads.


80-84: Leverage error handling from prepareBizArchive.
No issues found. The forwarded call is concise and clear.


94-100: Ensure correct biz version.
createBiz(BizOperation, File) sets the version from bizOperation. 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.
The Stream.concat approach is clear and concise.


205-244: Null checks for plugins.
A previous review already flagged a potential issue if getPluginByName(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 over bizModel.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 for installBiz and plugin operations.


41-41: Handle MalformedURLException.
Be mindful that URLs may be malformed. The code throws Throwable, so it’s at least surfaced upstream.


172-174: Overload returning installBiz(bizFile, args, null).
A clean approach to default the environment map to null.


178-181: Constructing BizConfig.
Packaging arguments and envs into BizConfig ensures consistent usage in downstream calls.


184-185: A direct pass-through for BizConfig.
No issues found—keeps code DRY.


188-192: Ensure bizConfig is never null.
The added assertion clarifies the contract that a valid config is expected.


207-207: Propagating biz.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 in BizConfig 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: Using BizConfig import.
Ensures test coverage for the new config usage.


294-294: Mocking createBiz(File, BizConfig).
Valid approach for verifying error handling.


297-297: Install with a new BizConfig.
Ensures the test checks the path where BizConfig is explicitly provided.


372-372: Repeated mock for bizFactoryServiceMock.
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 for newArrayList and currentThread. Consistent with typical test patterns.


105-106: Registering plugin exports.
Putting ITest 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: Populate dependentPlugins and resource maps for denial tests.
Ensures the bizClassLoader 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 custom BizClassLoader references the correct BizModel 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 for dependentPlugins.
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 in getDependentPlugins().
Exposing the internal mutable set allows external code to modify the BizModel state without control. Return an unmodifiable set to preserve encapsulation.


245-248: Make a defensive copy in setDependentPlugins().
Assigning the provided set directly lets callers unknowingly mutate the BizModel. Creating a new set instance foils unintended side effects.


680-684: Good use of ConcurrentHashMap for concurrency.
The use of a thread-safe map is appropriate for storing relationships that may be read and written by multiple threads.

Copy link

codecov bot commented Jan 3, 2025

Codecov Report

Attention: Patch coverage is 71.03825% with 106 lines in your changes missing coverage. Please review.

Project coverage is 77.86%. Comparing base (41fe026) to head (f82991e).
Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
...c/main/java/com/alipay/sofa/ark/api/ArkClient.java 62.37% 32 Missing and 6 partials ⚠️
...k/container/service/biz/BizFactoryServiceImpl.java 61.25% 21 Missing and 10 partials ⚠️
...ainer/service/plugin/PluginFactoryServiceImpl.java 82.53% 6 Missing and 5 partials ⚠️
...com/alipay/sofa/ark/spi/model/PluginOperation.java 47.61% 11 Missing ⚠️
...er/service/classloader/ClassLoaderServiceImpl.java 70.37% 6 Missing and 2 partials ⚠️
...vice/classloader/AbstractClasspathClassLoader.java 86.95% 0 Missing and 3 partials ⚠️
.../java/com/alipay/sofa/ark/spi/model/BizConfig.java 87.50% 2 Missing ⚠️
...va/com/alipay/sofa/ark/spi/model/PluginConfig.java 80.00% 2 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

@@ -409,6 +442,103 @@ public static ClientResponse checkOperation(BizOperation bizOperation) {
return checkBiz(bizOperation.getBizName(), bizOperation.getBizVersion());
}

public static ClientResponse installPlugin(PluginOperation pluginOperation) throws Exception {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

安装和卸载 plugin,没看到单测覆盖呢

@sofastack-cla sofastack-cla bot added size/XXL and removed size/XL labels Jan 7, 2025
Copy link
Contributor

@coderabbitai coderabbitai bot left a 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 issue

Add 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

📥 Commits

Reviewing files that changed from the base of the PR and between 9bd6901 and 2103ed4.

📒 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 issue

Add 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.

Comment on lines +458 to +461
try (InputStream inputStream = url.openStream()) {
FileUtils.copyInputStreamToFile(inputStream, localFile);
}
}
Copy link
Contributor

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.

Suggested change
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();
}
}
}

Comment on lines +471 to +477
PluginConfig pluginConfig = new PluginConfig();
if (!StringUtils.isEmpty(pluginOperation.getPluginName())) {
pluginConfig.setSpecifiedName(pluginOperation.getPluginName());
}
if (!StringUtils.isEmpty(pluginOperation.getPluginVersion())) {
pluginConfig.setSpecifiedVersion(pluginOperation.getPluginVersion());
}
Copy link
Contributor

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.

Copy link
Contributor

@coderabbitai coderabbitai bot left a 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:

  1. Add descriptive messages to assertions
  2. Add cleanup after test execution
  3. 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2103ed4 and f82991e.

📒 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

Copy link
Collaborator

@lvjing2 lvjing2 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@lvjing2 lvjing2 merged commit d1c277f into sofastack:master Jan 9, 2025
6 of 8 checks passed
lvjing2 pushed a commit that referenced this pull request Jan 23, 2025
…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)
lvjing2 added a commit that referenced this pull request Jan 23, 2025
* 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]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

新增离线计算能力
4 participants