修复静态合并部署时，springcloud依赖兼容性问题导致模块先于基座被加载的问题(#1032) #1033
Conversation
|
Hi @YanZhipeng-UESTC, 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. |
WalkthroughThe changes involve a modification to the Changes
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: 0
🧹 Outside diff range and nitpick comments (2)
sofa-ark-parent/support/ark-springboot-integration/ark-springboot-starter/src/main/java/com/alipay/sofa/ark/springboot/listener/ArkDeployStaticBizListener.java (2)
21-21: Consider documenting web server requirementThe addition of
ConfigurableWebServerApplicationContextcheck makes this component specific to web applications. This architectural constraint should be documented to prevent misuse in non-web contexts.Consider adding a class-level JavaDoc comment explaining this requirement:
/** * Listener for deploying static biz modules after master biz startup. * Note: This listener requires a web application context and won't deploy modules in non-web environments. */
37-41: Consider adding debug loggingFor better troubleshooting of deployment timing issues, consider adding debug logging to explain why deployment was skipped or executed.
Add logging statements:
if (event instanceof ContextRefreshedEvent && event.getApplicationContext() instanceof ConfigurableWebServerApplicationContext) { + logger.debug("Deploying static biz after master biz started in web context"); EmbedSofaArkBootstrap.deployStaticBizAfterEmbedMasterBizStarted(); + } else { + logger.debug("Skipping static biz deployment: refreshEvent={}, webContext={}", + event instanceof ContextRefreshedEvent, + event.getApplicationContext() instanceof ConfigurableWebServerApplicationContext); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
sofa-ark-parent/support/ark-springboot-integration/ark-springboot-starter/src/main/java/com/alipay/sofa/ark/springboot/listener/ArkDeployStaticBizListener.java(2 hunks)
🔇 Additional comments (1)
sofa-ark-parent/support/ark-springboot-integration/ark-springboot-starter/src/main/java/com/alipay/sofa/ark/springboot/listener/ArkDeployStaticBizListener.java (1)
37-38: Verify compatibility with existing use cases
The additional check for ConfigurableWebServerApplicationContext fixes the Spring Cloud initialization issue, but we should verify this doesn't affect other valid use cases.
Let's check for any non-web context usage patterns in tests or examples:
✅ Verification successful
The ConfigurableWebServerApplicationContext check appears safe to implement
The verification shows that:
- The listener is registered via spring.factories as a general ApplicationListener
- No explicit non-web usage patterns were found in the codebase
- No specific configuration properties or documentation indicate non-web use cases
- The static deployment feature appears to be inherently tied to web applications
The change to check for ConfigurableWebServerApplicationContext is a targeted fix for Spring Cloud initialization issues and doesn't appear to break any existing use cases, as static biz deployment seems to be a web-specific feature.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for test cases or examples that might be affected
rg -l "ArkDeployStaticBizListener" | while read -r file; do
if [[ $file =~ .*Test.* ]] || [[ $file =~ .*Example.* ]]; then
echo "=== $file ==="
rg -A 10 "ArkDeployStaticBizListener" "$file"
fi
done
Length of output: 723
Script:
#!/bin/bash
# Let's check how this listener is registered and used
echo "=== Checking spring.factories configuration ==="
cat sofa-ark-parent/support/ark-springboot-integration/ark-springboot-starter/src/main/resources/META-INF/spring.factories
echo -e "\n=== Checking for non-web application usage patterns ==="
rg -B 3 -A 3 "ArkDeployStaticBizListener|StaticBizDeployment" --type java
echo -e "\n=== Checking for any documentation or comments about usage ==="
fd README.md | xargs rg -i "static.*biz|deploy.*static"
Length of output: 2586
Script:
#!/bin/bash
# Let's check for any configuration properties or deployment logic related to static biz deployment
echo "=== Checking for static deployment configuration ==="
rg -B 2 -A 2 "static.*deployment|deploy.*static" --type java
echo -e "\n=== Checking for any Spring Boot configuration properties ==="
fd "application.properties|application.yml|bootstrap.properties|bootstrap.yml" | xargs rg -i "static.*biz|deploy.*static"
echo -e "\n=== Checking for any web-specific configuration or usage ==="
ast-grep --pattern 'class $_ implements ApplicationListener<$_> {
$$$
onApplicationEvent($_) {
$$$
}
}'
Length of output: 693
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1033 +/- ##
============================================
+ Coverage 78.20% 78.34% +0.14%
Complexity 874 874
============================================
Files 171 171
Lines 7056 7056
Branches 1037 1037
============================================
+ Hits 5518 5528 +10
+ Misses 1006 995 -11
- Partials 532 533 +1 ☔ View full report in Codecov by Sentry. 🚨 Try these New Features:
|
| @@ -33,7 +34,8 @@ public void onApplicationEvent(ApplicationContextEvent event) { | |||
| return; | |||
| } | |||
| if (ArkConfigs.isEmbedEnable() && ArkConfigs.isEmbedStaticBizEnable()) { | |||
| if (event instanceof ContextRefreshedEvent) { | |||
| if (event instanceof ContextRefreshedEvent | |||
| && event.getApplicationContext() instanceof ConfigurableWebServerApplicationContext) { | |||
There was a problem hiding this comment.
如果master biz 非web应用，也无法走进这个if
issue: #1032
Summary by CodeRabbit