Fix local cache path fallback for Spring Boot 3 executable JARs#136
Fix local cache path fallback for Spring Boot 3 executable JARs#136nobodyiam wants to merge 1 commit intoapolloconfig:mainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
✅ Files skipped from review due to trivial changes (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors classpath detection in ClassLoaderUtil by extracting resolution into Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
nobodyiam
force-pushed
the
codex/issue-5592-spring-boot3-cache
branch
from
3bbe114 to
1055f6b
Compare
There was a problem hiding this comment.
Pull request overview
This PR fixes Apollo Java’s local cache directory resolution when running from Spring Boot 3 executable (nested) JARs by avoiding treating nested-jar classpath URLs as filesystem paths, and instead falling back to a safe default (user.dir).
Changes:
- Refactors
ClassLoaderUtilclasspath resolution to only treatfile:URLs as filesystem locations and otherwise fall back to a provided default. - Adds regression tests covering both
file:URL resolution and Spring Boot 3 nested-jar URL fallback behavior.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| apollo-core/src/main/java/com/ctrip/framework/apollo/core/utils/ClassLoaderUtil.java | Introduces resolveClassPath(...) using URL protocol checks and Paths.get(url.toURI()) to prevent nested-jar URLs from being used as filesystem paths. |
| apollo-core/src/test/java/com/ctrip/framework/apollo/core/utils/ClassLoaderUtilTest.java | Adds tests validating correct handling for file: URLs and fallback behavior for nested-jar URLs. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| static String resolveClassPath(ClassLoader classLoader, String defaultClassPath) throws Exception { | ||
| URL url = classLoader.getResource(""); | ||
| if (url == null || !"file".equalsIgnoreCase(url.getProtocol())) { | ||
| return defaultClassPath; | ||
| } | ||
|
|
||
| String resolvedClassPath = Paths.get(url.toURI()).toString(); | ||
| if (Strings.isNullOrEmpty(resolvedClassPath)) { | ||
| return defaultClassPath; | ||
| } |
There was a problem hiding this comment.
resolveClassPath now derives the classpath via Paths.get(url.toURI()).toString(), which changes the returned format on Windows (it will no longer include the leading / that URL#getPath() produced). There is existing code in the repo that compensates for the old behavior (e.g. apollo-client/src/test/java/com/ctrip/framework/apollo/BaseIntegrationTest.java strips the leading /), which will become incorrect with this change and can break Windows builds/tests. Consider either preserving the previous Windows formatting (e.g. keep using the decoded url.getPath() for file: URLs) or updating the affected call sites/tests to stop assuming a leading / from getClassPath().
There was a problem hiding this comment.
Preserved the historical file: URL formatting in ClassLoaderUtil. resolveClassPath now decodes url.getPath() instead of using Paths.get(url.toURI()), so the existing Windows-leading-slash behavior is kept for callers that still rely on it. I also added a regression test for a file:/C:/Program%20Files/... URL while keeping the nested-jar fallback unchanged.
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.gh.lumenfield.work/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1055f6b21a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| loader = ClassLoader.getSystemClassLoader(); | ||
| } | ||
|
|
||
| String defaultClassPath = System.getProperty("user.dir"); |
There was a problem hiding this comment.
System.getProperty("user.dir") is now evaluated unconditionally at class initialization, so in JVMs that deny reading this property (for example, SecurityManager/policy-restricted Java 8 deployments) ClassLoaderUtil can fail to initialize with SecurityException even when classLoader.getResource("") resolves to a normal file: classpath. Before this change, user.dir was only read on fallback paths, so the common file-URL case did not require that permission.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Adjusted this so user.dir is only read on the fallback path again. The static initializer now tries resolveClassPath(loader, null) first and only calls getDefaultClassPath() when the classpath cannot be resolved, which restores the previous permission behavior for normal file: classpaths.
nobodyiam
force-pushed
the
codex/issue-5592-spring-boot3-cache
branch
from
1055f6b to
1828bd8
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #136 +/- ##
============================================
+ Coverage 68.68% 70.98% +2.30%
- Complexity 1503 1636 +133
============================================
Files 212 224 +12
Lines 6396 6732 +336
Branches 647 680 +33
============================================
+ Hits 4393 4779 +386
+ Misses 1673 1602 -71
- Partials 330 351 +21 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
nobodyiam
force-pushed
the
codex/issue-5592-spring-boot3-cache
branch
from
1828bd8 to
ea5f128
Compare
What's the purpose of this PR
Fix Apollo Java local cache directory resolution when running from a Spring Boot 3 executable JAR.
ClassLoaderUtilpreviously assumed that jar-internal classpath locations would contain.jar!, but Spring Boot 3 executable JARs expose nested URLs such asjar:nested:/.../!BOOT-INF/classes/!/. When Apollo falls back toClassLoaderUtil.getClassPath()for local cache persistence, that nested URL was treated as a filesystem path andconfig-cachedirectory creation failed.Which issue(s) this PR fixes:
Fixes apolloconfig/apollo#5592
Brief changelog
file:resources as filesystem classpath locations and fall back touser.dirfor nested-jar URLs.mvn -pl apollo-core -Dtest=ClassLoaderUtilTest test -Dmaven.gitcommitid.skip=truemvn -pl apollo-client -am -Dtest=ClassLoaderUtilTest,LocalFileConfigRepositoryTest,DefaultConfigTest -Dsurefire.failIfNoSpecifiedTests=false test -Dmaven.gitcommitid.skip=trueFollow this checklist to help us incorporate your contribution quickly and easily:
mvn clean testto make sure this pull request doesn't break anything.CHANGESlog.Summary by CodeRabbit
Bug Fixes
Tests
Documentation