feat: 支持模块启动时通过传递env参数动态指定模块的启动类 (#999) #1029
Conversation
|
Hi @liufeng-xiwo, welcome to SOFAStack community, Please sign Contributor License Agreement! After you signed CLA, we will automatically sync the status of this pull request in 3 minutes. |
sofastack-cla
bot
added
bug
WalkthroughThe changes involve modifications to the Changes
Possibly related PRs
Suggested reviewers
Poem
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java (1)
206-206: LGTM! Consider adding a Javadoc comment for better maintainability.The new constant
BIZ_MAIN_CLASSis well-defined and follows the established naming conventions. Its purpose aligns with the PR objective of supporting dynamic module startup class specification.Consider adding a Javadoc comment to document the purpose and usage of this constant, similar to other sections in the file. Example:
+ /** + * Environment property to dynamically specify the module's startup class + */ public final static String BIZ_MAIN_CLASS = "sofa.ark.biz.main.class";sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (1)
326-332: Add validation and documentation for the main class override feature.While the implementation allows for dynamic specification of the main class, consider these improvements:
- Validate that
mainClassFromEnvcontains a valid class name- Add Javadoc to document this feature in the
startanddoStartmethods// 支持运行时根据环境变量动态设置模块的启动类 String mainClassFromEnv = envs.get(Constants.BIZ_MAIN_CLASS); -if (mainClassFromEnv != null){ +if (!StringUtils.isEmpty(mainClassFromEnv)) { + // Validate class name format + AssertUtils.isFalse(StringUtils.isEmpty(mainClassFromEnv), "Main Class from environment must not be empty!"); mainClass = mainClassFromEnv; ArkLoggerFactory.getDefaultLogger().info("Ark biz {} will start with main class {} from envs", getIdentity(), mainClassFromEnv); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java(1 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java(1 hunks)
🔇 Additional comments (1)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java (1)
333-333: LGTM: Appropriate error handling for missing main class.
The error handling is well implemented with a descriptive error message that includes the biz name for better debugging context.
|
|
||
| // 支持运行时根据环境变量动态设置模块的启动类 | ||
| String mainClassFromEnv = envs.get(Constants.BIZ_MAIN_CLASS); | ||
| if (mainClassFromEnv != null){ | ||
| mainClass = mainClassFromEnv; | ||
| ArkLoggerFactory.getDefaultLogger().info("Ark biz {} will start with main class {} from envs", getIdentity(), mainClassFromEnv); | ||
| } | ||
|
|
There was a problem hiding this comment.
💡 Codebase verification
Test coverage needed for environment-based main class configuration
The verification reveals:
- The
BIZ_MAIN_CLASSconstant is properly defined inConstants.java - The only usage of this constant is in the new implementation
- No test coverage found for this new environment-based main class configuration feature
Action items:
- Add test cases to verify the environment-based main class configuration
- Include both positive (when env var is set) and negative (when env var is not set) test scenarios
- Test the logging behavior when environment variable is used
🔗 Analysis chain
Verify constant usage and test coverage.
Let's verify the implementation of this feature:
- Check if the
BIZ_MAIN_CLASSconstant is properly defined - Look for test coverage of this new functionality
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check for constant definition
echo "Checking for BIZ_MAIN_CLASS constant definition..."
rg -p "BIZ_MAIN_CLASS.*=.*\"sofa.ark.biz.main.class\""
# Check for test coverage
echo "Checking for test coverage..."
fd -e java -e groovy -e scala test | rg -l "BIZ_MAIN_CLASS|mainClassFromEnv"
Length of output: 516
Script:
#!/bin/bash
# Let's check for test files that might cover BizModel or main class configuration
echo "Checking for BizModel test coverage..."
rg -p "class.*Test.*BizModel" -g "*.java"
# Check for any test files that might test environment variable configuration
echo "Checking for environment variable test coverage..."
rg -p "test.*[Ee]nv.*main.*class" -g "*.java"
# Check for actual usage of this constant in the codebase
echo "Checking for usage of BIZ_MAIN_CLASS constant..."
rg -p "Constants\.BIZ_MAIN_CLASS" -A 2 -g "*.java"
Length of output: 712
liufeng-xiwo
force-pushed
the
issue999
branch
2 times, most recently
from
33f3319 to
dc93ed7
Compare
liufeng-xiwo
force-pushed
the
issue999
branch
from
dc93ed7 to
e1c3dd2
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (3)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (3)
318-318: Convert comment to English for consistencyThe comment should be in English to maintain consistency with the codebase.
- // 传 env 参数，使用动态的模块启动类 + // Pass env parameter to use dynamic module startup class
315-330: Enhance test coverage for dynamic main class featureWhile the happy path is tested, consider adding test cases for:
- Invalid main class name
- Empty main class name
- Non-existent main class
This would ensure the feature handles error cases gracefully.
Example addition:
@Test public void testInstallOperationWithInvalidDynamicMainClass() throws Throwable { BizOperation bizOperation = new BizOperation(); bizOperation.setOperationType(INSTALL); bizOperation.getParameters().put(CONFIG_BIZ_URL, bizUrl5.toString()); bizOperation.setBizName("biz-demo"); bizOperation.setBizVersion("5.0.0"); // Test with non-existent class Map<String, String> envs = Collections.singletonMap(Constants.BIZ_MAIN_CLASS, "org.example.NonExistentClass"); ClientResponse response = installOperation(bizOperation, new String[] {}, envs); // Add appropriate assertions based on expected behavior }
327-329: Make assertions more descriptiveThe assertions could be more descriptive to better document the test's intent and make failures more informative.
- ClientResponse response2 = installOperation(bizOperation, new String[] {}, envs); - assertEquals(SUCCESS, response2.getCode()); - assertEquals("org.example.Main2", (new ArrayList<>(response2.getBizInfos())).get(0).getMainClass()); + ClientResponse response = installOperation(bizOperation, new String[] {}, envs); + assertEquals("Installation should succeed", SUCCESS, response.getCode()); + BizInfo bizInfo = (new ArrayList<>(response.getBizInfos())).get(0); + assertEquals("Main class should be set to the specified class", + "org.example.Main2", + bizInfo.getMainClass());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
sofa-ark-parent/core-impl/container/src/test/resources/sample-ark-5.0.0-ark-biz.jaris excluded by!**/*.jar
📒 Files selected for processing (3)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java(1 hunks)sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java(5 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java
liufeng-xiwo
force-pushed
the
issue999
branch
from
eaed6ae to
6833d9c
Compare
liufeng-xiwo
changed the title
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (1)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (1)
322-349: Good test implementation, but some improvements suggested.The test comprehensively validates the dynamic main class functionality. However, consider these improvements:
- Replace Chinese comments with English for international collaboration
- Extract magic strings as constants
- Add test cases for error scenarios
Consider applying these improvements:
- // 传 env 参数，使用动态的模块启动类 org.example.Main2 + // Test dynamic module startup class using env parameter + private static final String DYNAMIC_MAIN_CLASS = "org.example.Main2"; + private static final String ORIGINAL_MAIN_CLASS = "org.example.Main1"; BizOperation bizOperation = new BizOperation(); bizOperation.setOperationType(INSTALL); bizOperation.getParameters().put(CONFIG_BIZ_URL, bizUrl5.toString()); bizOperation.setBizName("biz-demo"); bizOperation.setBizVersion("5.0.0"); - Map<String, String> envs = Collections.singletonMap(Constants.BIZ_MAIN_CLASS, "org.example.Main2"); + Map<String, String> envs = Collections.singletonMap(Constants.BIZ_MAIN_CLASS, DYNAMIC_MAIN_CLASS); ClientResponse response2 = installOperation(bizOperation, new String[] {}, envs); assertEquals(SUCCESS, response2.getCode()); - assertEquals("org.example.Main2", (new ArrayList<>(response2.getBizInfos())).get(0).getMainClass()); + assertEquals(DYNAMIC_MAIN_CLASS, (new ArrayList<>(response2.getBizInfos())).get(0).getMainClass()); - // 实际模块jar包里配置启动类是 org.example.Main1 + // Verify original main class in module JAR manifestConsider adding these test cases:
- Test with invalid main class
- Test with empty/null main class
- Test with non-existent main class
Would you like me to help implement these additional test cases?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
sofa-ark-parent/core-impl/container/src/test/resources/sample-ark-5.0.0-ark-biz.jaris excluded by!**/*.jar
📒 Files selected for processing (3)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java(1 hunks)sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java(5 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java
🔇 Additional comments (2)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (2)
106-107: LGTM! Field declaration follows existing patterns.
The new field bizUrl5 is well-documented and maintains consistency with the existing field declarations.
120-121: LGTM! Field initialization is consistent.
The initialization of bizUrl5 follows the established pattern and maintains consistency with other URL initializations.
liufeng-xiwo
force-pushed
the
issue999
branch
from
6833d9c to
50b6b6e
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (2)
322-349: LGTM! Consider adding more test cases for comprehensive coverage.The test effectively validates the dynamic main class specification feature. Consider adding these test cases for more thorough coverage:
- Negative test case when the specified main class doesn't exist
- Test behavior when environment parameter is not set
- Test with invalid main class values
341-347: Ensure proper resource cleanup.The test creates temporary files and opens JarFile resources. While the InputStream is properly closed using try-with-resources, the JarFile and JarFileArchive resources should also be closed.
Apply this diff to ensure proper resource cleanup:
- JarFile bizFile = new JarFile(file); - JarFileArchive jarFileArchive = new JarFileArchive(bizFile); - BizArchive bizArchive = new JarBizArchive(jarFileArchive); - assertEquals("org.example.Main1", bizArchive.getManifest().getMainAttributes().getValue(Constants.MAIN_CLASS_ATTRIBUTE)); - assertEquals("org.example.Main1", bizArchive.getManifest().getMainAttributes().getValue(Constants.START_CLASS_ATTRIBUTE)); + try (JarFile bizFile = new JarFile(file)) { + JarFileArchive jarFileArchive = new JarFileArchive(bizFile); + BizArchive bizArchive = new JarBizArchive(jarFileArchive); + assertEquals("org.example.Main1", bizArchive.getManifest().getMainAttributes().getValue(Constants.MAIN_CLASS_ATTRIBUTE)); + assertEquals("org.example.Main1", bizArchive.getManifest().getMainAttributes().getValue(Constants.START_CLASS_ATTRIBUTE)); + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
sofa-ark-parent/core-impl/container/src/test/resources/sample-ark-5.0.0-ark-biz.jaris excluded by!**/*.jar
📒 Files selected for processing (3)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java(1 hunks)sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java(5 hunks)sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java
- sofa-ark-parent/core/spi/src/main/java/com/alipay/sofa/ark/spi/constant/Constants.java
🔇 Additional comments (1)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (1)
106-107: LGTM! Variable declaration and initialization are consistent.
The new URL variable bizUrl5 follows the established pattern and is properly initialized in the before() method.
Also applies to: 120-121
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (2)
120-121: Consider adding a null check for the resource.The resource loading could return null if the jar file is not found in the classpath. Consider adding a verification:
// bizName=biz-demo, bizVersion=5.0.0 -bizUrl5 = this.getClass().getClassLoader().getResource("sample-ark-5.0.0-ark-biz.jar"); +URL bizUrl5 = this.getClass().getClassLoader().getResource("sample-ark-5.0.0-ark-biz.jar"); +if (bizUrl5 == null) { + throw new IllegalStateException("Required test resource 'sample-ark-5.0.0-ark-biz.jar' not found"); +} +this.bizUrl5 = bizUrl5;
322-354: Enhance test robustness and resource handling.While the test effectively verifies the dynamic main class functionality, consider these improvements:
- Add more descriptive comments explaining the test scenario:
- // the biz module will start with dynamic mainClass specified in env parameters, which is org.example.Main2 + // Test Scenario: + // 1. Install a biz module with a dynamic main class specified via environment variables + // 2. Verify that the runtime uses the dynamic main class (org.example.Main2) + // 3. Verify that the original packaged main class (org.example.Main1) remains unchanged
- Use try-with-resources for proper resource cleanup:
- JarFile bizFile = new JarFile(file); - JarFileArchive jarFileArchive = new JarFileArchive(bizFile); + try (JarFile bizFile = new JarFile(file); + JarFileArchive jarFileArchive = new JarFileArchive(bizFile)) { BizArchive bizArchive = new JarBizArchive(jarFileArchive); assertEquals("org.example.Main1", bizArchive.getManifest().getMainAttributes().getValue(Constants.MAIN_CLASS_ATTRIBUTE)); assertEquals("org.example.Main1", bizArchive.getManifest().getMainAttributes().getValue(Constants.START_CLASS_ATTRIBUTE)); + }
- Add error case verification:
+ // Verify error case with invalid main class + Map<String, String> invalidEnvs = Collections.singletonMap(Constants.BIZ_MAIN_CLASS, + "org.example.NonExistentClass"); + ClientResponse response3 = installOperation(bizOperation, new String[] {}, invalidEnvs); + assertNotEquals(SUCCESS, response3.getCode());
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
sofa-ark-parent/core-impl/container/src/main/java/com/alipay/sofa/ark/container/model/BizModel.java(1 hunks)sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.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
🔇 Additional comments (1)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/api/ArkClientTest.java (1)
106-107: LGTM!
The new field declaration follows the existing pattern and is properly documented.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1029 +/- ##
============================================
+ Coverage 78.08% 78.27% +0.18%
- Complexity 876 878 +2
============================================
Files 172 172
Lines 7072 7078 +6
Branches 1039 1041 +2
============================================
+ Hits 5522 5540 +18
+ Misses 1018 1006 -12
Partials 532 532 ☔ View full report in Codecov by Sentry. |
* feat: 支持模块启动时通过传递env参数动态指定模块的启动类 (#999) * fix format --------- Co-authored-by: leo james <[email protected]> (cherry picked from commit a282aa5)
* add param to determine whether unpack biz when install (#1042) * add param to judge whether unpack biz when install (#325) --------- Co-authored-by: beikai <[email protected]> (cherry picked from commit e9e7f34) * add event after all biz startup when bizs playback in base restart (#1046) * set logback context name=default * add event after all biz startup when bizs playback in base restart (cherry picked from commit f4e4c1d) * fix: 修复委托基座加载资源过程中，模块未声明的依赖存在该资源时，无法正确获取资源(#1030) (#1031) * fix: correctly retrieve resources when undeclared dependencies exist during the loading of the delegate base from * fix: spaces map type forcible transfer * fix: test --------- Co-authored-by: fanyue <[email protected]> Co-authored-by: leo james <[email protected]> (cherry picked from commit cd1e640) * feat: 支持模块启动时通过传递env参数动态指定模块的启动类 (#999) (#1029) * feat: 支持模块启动时通过传递env参数动态指定模块的启动类 (#999) * fix format --------- Co-authored-by: leo james <[email protected]> (cherry picked from commit a282aa5) --------- Co-authored-by: Kennan <[email protected]> Co-authored-by: Jackisome <[email protected]> Co-authored-by: 刘丰 <[email protected]>
* add param to determine whether unpack biz when install (#1042) * add param to judge whether unpack biz when install (#325) --------- Co-authored-by: beikai <[email protected]> (cherry picked from commit e9e7f34) * add event after all biz startup when bizs playback in base restart (#1046) * set logback context name=default * add event after all biz startup when bizs playback in base restart (cherry picked from commit f4e4c1d) * fix: 修复委托基座加载资源过程中，模块未声明的依赖存在该资源时，无法正确获取资源(#1030) (#1031) * fix: correctly retrieve resources when undeclared dependencies exist during the loading of the delegate base from * fix: spaces map type forcible transfer * fix: test --------- Co-authored-by: fanyue <[email protected]> Co-authored-by: leo james <[email protected]> (cherry picked from commit cd1e640) * feat: 支持模块启动时通过传递env参数动态指定模块的启动类 (#999) (#1029) * feat: 支持模块启动时通过传递env参数动态指定模块的启动类 (#999) * fix format --------- Co-authored-by: leo james <[email protected]> (cherry picked from commit a282aa5) * update to 3.1.9 --------- Co-authored-by: Kennan <[email protected]> Co-authored-by: Jackisome <[email protected]> Co-authored-by: 刘丰 <[email protected]>
模块启动时，可以通过 envs["sofa.ark.biz.main.class"] 参数传递指定的类作为模块的启动类
Summary by CodeRabbit
New Features
BIZ_MAIN_CLASSfor improved configuration options related to business logic.Bug Fixes