fix: 修复委托基座加载资源过程中，模块未声明的依赖存在该资源时，无法正确获取资源(#1030) #1031
Conversation
…during the loading of the delegate base from
|
Hi @Jackisome, 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 resource handling logic within the Changes
Assessment against linked issues
Possibly related PRs
Suggested labels
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
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: 0
🧹 Outside diff range and nitpick comments (3)
sofa-ark-parent/support/ark-support-starter/src/main/java/com/alipay/sofa/ark/support/common/DelegateToMasterBizClassLoaderHook.java (1)
109-115: Consider adding logging for resource resolution.The implementation correctly addresses the issue by searching for multiple resources when the first attempt fails. However, to improve maintainability and debugging:
- Consider adding debug logging when falling back to searching multiple resources
- Log which resource was selected when multiple matches are found to help track resource resolution
Here's a suggested improvement:
Enumeration<URL> matchResourceUrls = postFindResources(name, classLoaderService, biz); // get the first resource url when match multiple resources if (matchResourceUrls != null && matchResourceUrls.hasMoreElements()) { + if (ArkLoggerFactory.getDefaultLogger().isDebugEnabled()) { + List<URL> allUrls = Collections.list(matchResourceUrls); + ArkLoggerFactory.getDefaultLogger().debug( + String.format("Found multiple matching resources for %s, selecting first: %s", + name, allUrls.get(0))); + matchResourceUrls = Collections.enumeration(allUrls); + } return matchResourceUrls.nextElement(); }sofa-ark-parent/support/ark-support-starter/src/test/java/com/alipay/sofa/ark/support/DefaultClassLoaderHookTest.java (1)
Line range hint
1-266: Consider enhancing test class maintainability and documentationWhile the test coverage is comprehensive, consider the following improvements:
- Add Javadoc to document the test scenarios and their relationship to the resource loading functionality
- Extract common setup code into helper methods to reduce duplication
- Consider using parameterized tests for different resource loading scenarios
Example documentation improvement:
/** * Tests the class loader hook functionality with focus on resource loading from multiple sources. * This test class verifies: * - Resource loading from declared vs undeclared dependencies * - Multiple jar resource resolution * - Master biz class loader delegation */ @Test public void testLoadClassFromClassLoaderHook() throws Exception { // existing code... }sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/classloader/BizClassLoaderTest.java (1)
Line range hint
147-160: Consider adding specific test cases for undeclared dependencies.While the existing test cases (6-13) cover various resource loading scenarios, consider adding explicit test cases that verify:
- Resource loading when the dependency is undeclared but available in the base
- Full resource search behavior when no resources are loaded
- Resource loading when only the first resource is loaded but remains undeclared
Example test case structure:
@Test public void testUndeclaredDependencyResourceLoading() { // Setup: Create plugins with overlapping resources // Test: Verify resource loading behavior when: // 1. Resource is undeclared but available in base // 2. No resources are loaded // 3. First resource is loaded but undeclared }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/classloader/BizClassLoaderTest.java(2 hunks)sofa-ark-parent/support/ark-support-starter/src/main/java/com/alipay/sofa/ark/support/common/DelegateToMasterBizClassLoaderHook.java(1 hunks)sofa-ark-parent/support/ark-support-starter/src/test/java/com/alipay/sofa/ark/support/DefaultClassLoaderHookTest.java(1 hunks)
🔇 Additional comments (4)
sofa-ark-parent/support/ark-support-starter/src/main/java/com/alipay/sofa/ark/support/common/DelegateToMasterBizClassLoaderHook.java (1)
109-115: Verify impact on dependent modules.
The new resource resolution strategy might affect modules that depend on specific resource resolution behavior. Let's verify the impact:
✅ Verification successful
Based on the test results, I can now generate the final response since I have enough information to verify the resource resolution behavior:
Resource resolution strategy is correctly implemented and tested
The changes to resource resolution in DelegateToMasterBizClassLoaderHook are well-tested and follow established patterns:
- The test cases in
DefaultClassLoaderHookTestverify that resource resolution works correctly for both single and multiple resources - The behavior is consistent with other classloader implementations in the codebase (BizClassLoader, PluginClassLoader)
- Test coverage includes:
- Resource resolution from master biz
- Handling of non-existent resources
- Multiple resource enumeration
- Resource caching behavior
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for potential impacts on dependent modules
# Look for other classes that might be affected by the resource resolution changes
rg -l "getResource\(|getResources\(" --type java | \
xargs rg -l "DelegateToMasterBizClassLoaderHook" --type java
# Check for test cases that verify resource loading behavior
rg -l "test.*Resource" --type java | \
xargs rg -B 5 -A 5 "getResource\(|getResources\("
Length of output: 71423
sofa-ark-parent/support/ark-support-starter/src/test/java/com/alipay/sofa/ark/support/DefaultClassLoaderHookTest.java (1)
143-145: Verify test coverage for multiple jar resource loading scenario
The modification of case 7 aligns well with the PR objective to fix resource loading issues with undeclared dependencies. However, let's ensure comprehensive test coverage:
- The test verifies successful resource loading but doesn't explicitly verify the order of resource discovery
- Consider adding assertions to validate the source jar of the returned resource
Let's verify the test resources are properly set up:
Consider enhancing the test case with additional assertions:
// case 7: find resource from master in multiple jar
-Assert.assertNotNull(bizModel.getBizClassLoader().getResource("Sample_Resource_Exported_A"));
+URL resourceUrl = bizModel.getBizClassLoader().getResource("Sample_Resource_Exported_A");
+Assert.assertNotNull(resourceUrl);
+// Verify the source jar of the resource
+Assert.assertTrue("Resource should be loaded from the expected jar",
+ resourceUrl.getPath().contains("sample-ark-plugin-common"));✅ Verification successful
Test setup for multiple jar resource loading is properly configured
The test setup is correctly implemented for verifying resource loading from multiple jars:
- The resource "Sample_Resource_Exported_A" is exported by both pluginA and pluginB in the test setup
- The test validates the resource loading behavior through the bizModel's ClassLoader
- The assertion correctly verifies the presence of the resource without being overly restrictive about which plugin provides it
The original review comment suggesting additional assertions for source jar verification would actually make the test more brittle, as the resource is intentionally available from multiple sources to test the multiple jar scenario.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify the presence and content of test resource files
# Check if the test resource exists in multiple jars
echo "Checking for Sample_Resource_Exported_A in test resources:"
find . -name "*.jar" -exec sh -c 'echo "Checking {}" && jar tf {} | grep "Sample_Resource_Exported_A"' \;
Length of output: 4793
Script:
#!/bin/bash
# Let's check for the test resource files directly in the repository
echo "Checking for Sample_Resource_Exported_A in source files:"
rg -l "Sample_Resource_Exported_A"
# Check test setup in DefaultClassLoaderHookTest
echo -e "\nChecking test setup in DefaultClassLoaderHookTest:"
rg -A 10 "Sample_Resource_Exported_A" sofa-ark-parent/support/ark-support-starter/src/test/java/com/alipay/sofa/ark/support/DefaultClassLoaderHookTest.java
Length of output: 1240
Script:
#!/bin/bash
# Let's check the test setup in BizClassLoaderTest to understand the resource setup
echo "Checking BizClassLoaderTest setup:"
rg -A 10 "Sample_Resource_Exported_A" sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/classloader/BizClassLoaderTest.java
# Let's also check for any resource files in test resources
echo -e "\nChecking test resource directories:"
fd -t f "Sample_Resource_Exported_A" sofa-ark-parent/
Length of output: 1583
sofa-ark-parent/core-impl/container/src/test/java/com/alipay/sofa/ark/container/service/classloader/BizClassLoaderTest.java (2)
147-147: LGTM! The additional resource export aligns with the PR objectives.
The addition of "Sample_Resource_Exported_A" to pluginA's exported resources helps test the scenario where multiple plugins export the same resource, which is relevant to the fix for undeclared dependencies.
160-160: LGTM! The resource export in pluginB complements the test setup.
Adding "Sample_Resource_Exported_A" to pluginB's exported resources creates the test scenario where the same resource is available from multiple plugins.
Jackisome
force-pushed
the
issue_1030
branch
from
b8f9410 to
5b3cd41
Compare
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1031 +/- ##
============================================
+ Coverage 78.05% 78.22% +0.16%
Complexity 876 876
============================================
Files 172 172
Lines 7068 7072 +4
Branches 1038 1039 +1
============================================
+ Hits 5517 5532 +15
+ Misses 1019 1007 -12
- Partials 532 533 +1 ☔ View full report in Codecov by Sentry. |
* 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)
* 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]>
Motivation
模块委托给基座加载资源时，如果基座存在多个资源，部分资源模块未声明。如果从基座加载时加载到未声明依赖里的资源，则会返回空，从而无法正确从基座资源
Modification
在未加载到、或加载到第一个资源但模块未声明时，全量查找资源，返回找到的第一个声明过的资源。
Result
fixed #1030
Summary by CodeRabbit
New Features
Bug Fixes