Automatically exposes jdk modules #1340
Conversation
Signed-off-by: JermaineHua <crazyhzm@apache.org>
WalkthroughThe changes introduce a new versioning scheme in the Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant DynamicInitializer as DynamicModuleApplicationContextInitializer
participant ExportInitializer as DynamicModuleExportApplicationContextInitializer
participant AutoExportInitializer as AutoModuleExportApplicationContextInitializer
participant ModuleUtil as ModuleUtil
App->>DynamicInitializer: Initialize Application Context
DynamicInitializer->>ModuleUtil: Call exportAllJDKModulePackageToAll()
ModuleUtil-->>DynamicInitializer: Modify module accessibility
DynamicInitializer-->>App: Complete initialization
App->>ExportInitializer: Initialize Export Context
ExportInitializer->>ModuleUtil: Call exportAllModulePackageToAll()
ModuleUtil-->>ExportInitializer: Modify module package accessibility
ExportInitializer-->>App: Complete export initialization
App->>AutoExportInitializer: Initialize Auto Module Export
AutoExportInitializer->>ModuleUtil: Check configuration and export modules
ModuleUtil-->>AutoExportInitializer: Modify module export based on config
AutoExportInitializer-->>App: Complete auto module export initialization
Possibly related PRs
Suggested labels
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: 6
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (6)
- pom.xml (1 hunks)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/Initializer/DynamicModuleApplicationContextInitializer.java (1 hunks)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java (1 hunks)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/UnsafeUtil.java (1 hunks)
- sofa-boot-project/sofa-boot/src/main/resources/META-INF/spring.factories (1 hunks)
- sofa-boot-project/sofa-boot/src/test/java/com/alipay/sofa/boot/util/ModuleUtilTests.java (1 hunks)
Additional comments not posted (20)
sofa-boot-project/sofa-boot/src/main/resources/META-INF/spring.factories (1)
18-19: LGTM! But verify the implementation of the new initializer.The addition of
com.alipay.sofa.boot.Initializer.DynamicModuleApplicationContextInitializerto the list ofApplicationContextInitializerseems appropriate. Ensure that the new initializer is correctly implemented and does not introduce any issues.The code changes are approved.
Run the following script to verify the implementation of the new initializer:
Verification successful
Implementation Verified: DynamicModuleApplicationContextInitializer
The
DynamicModuleApplicationContextInitializeris correctly implemented as anApplicationContextInitializer. It overrides theinitializemethod to callModuleUtil.exportAllJDKModulePackageToAll(). Ensure that this utility function behaves as expected.
- File:
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/Initializer/DynamicModuleApplicationContextInitializer.javaScripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of the new initializer. # Test: Search for the implementation of the new initializer. Expect: Only occurrences of the new implementation. rg --type java $'DynamicModuleApplicationContextInitializer'Length of output: 238
Script:
#!/bin/bash # Extract the implementation details of the DynamicModuleApplicationContextInitializer class. ast-grep --lang java --pattern $'class DynamicModuleApplicationContextInitializer { $$$ }' sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/Initializer/DynamicModuleApplicationContextInitializer.javaLength of output: 216
Script:
#!/bin/bash # Use rg to extract the implementation details of the DynamicModuleApplicationContextInitializer class. rg 'class DynamicModuleApplicationContextInitializer' -A 50 sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/Initializer/DynamicModuleApplicationContextInitializer.javaLength of output: 580
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/Initializer/DynamicModuleApplicationContextInitializer.java (5)
1-16: LGTM!The license header is appropriate and correctly formatted.
The code changes are approved.
17-17: LGTM!The package declaration is appropriate and correctly formatted.
The code changes are approved.
19-21: LGTM!The imports are appropriate and correctly formatted.
The code changes are approved.
28-30: LGTM!The class declaration is appropriate and correctly formatted.
The code changes are approved.
31-34: LGTM! But verify the implementation ofModuleUtil.exportAllJDKModulePackageToAll().The method implementation is appropriate. Ensure that the
ModuleUtil.exportAllJDKModulePackageToAll()method is correctly implemented and does not introduce any issues.The code changes are approved.
Run the following script to verify the implementation of
ModuleUtil.exportAllJDKModulePackageToAll():Verification successful
Implementation Verified: Review the use of catch-all
ThrowableinexportAllJDKModulePackageToAll().The method
exportAllJDKModulePackageToAll()is implemented to export JDK module packages to all modules with exception handling. However, the use of a catch-allThrowablecould potentially hide issues. Consider refining this to handle specific exceptions.
- File:
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java- Lines: 47-61
Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Verify the implementation of `ModuleUtil.exportAllJDKModulePackageToAll()`. # Test: Search for the implementation of `ModuleUtil.exportAllJDKModulePackageToAll()`. Expect: Only occurrences of the implementation. rg --type java $'exportAllJDKModulePackageToAll'Length of output: 507
Script:
#!/bin/bash # Extract the implementation of `exportAllJDKModulePackageToAll()` from `ModuleUtil.java`. ast-grep --lang java --pattern $'public static void exportAllJDKModulePackageToAll() { $$$ }' sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.javaLength of output: 2044
sofa-boot-project/sofa-boot/src/test/java/com/alipay/sofa/boot/util/ModuleUtilTests.java (5)
1-16: LGTM!The license header is appropriate and correctly formatted.
The code changes are approved.
17-17: LGTM!The package declaration is appropriate and correctly formatted.
The code changes are approved.
19-27: LGTM!The imports are appropriate and correctly formatted.
The code changes are approved.
34-34: LGTM!The class declaration is appropriate and correctly formatted.
The code changes are approved.
36-50: LGTM!The test method is appropriate and correctly validates the behavior of
ModuleUtil.exportAllJDKModulePackageToAll().The code changes are approved.
pom.xml (1)
38-38: LGTM!The versioning change to
4.4.0-SNAPSHOTis appropriate for ongoing development.The code changes are approved.
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java (8)
91-93: LGTM!The method is correctly implemented.
The code changes are approved.
102-104: LGTM!The method is correctly implemented.
The code changes are approved.
113-115: LGTM!The method is correctly implemented.
The code changes are approved.
124-126: LGTM!The method is correctly implemented.
The code changes are approved.
135-137: LGTM!The method is correctly implemented.
The code changes are approved.
147-149: LGTM!The method is correctly implemented.
The code changes are approved.
158-160: LGTM!The method is correctly implemented.
The code changes are approved.
168-170: LGTM!The method is correctly implemented.
The code changes are approved.
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/UnsafeUtil.java
Outdated
Show resolved
Hide resolved
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/UnsafeUtil.java
Outdated
Show resolved
Hide resolved
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java
Outdated
Show resolved
Hide resolved
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java
Outdated
Show resolved
Hide resolved
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java
Outdated
Show resolved
Hide resolved
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java
Outdated
Show resolved
Hide resolved
CrazyHZM
force-pushed
the
feat/dynamic_export_and_open
branch
from
e289863
to
88c7e01
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/Initializer/DynamicModuleExportApplicationContextInitializer.java (1 hunks)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java (1 hunks)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/UnsafeUtil.java (1 hunks)
- sofa-boot-project/sofa-boot/src/main/resources/META-INF/spring.factories (1 hunks)
- sofa-boot-project/sofa-boot/src/test/java/com/alipay/sofa/boot/util/ModuleUtilTests.java (1 hunks)
Files skipped from review as they are similar to previous changes (4)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/UnsafeUtil.java
- sofa-boot-project/sofa-boot/src/main/resources/META-INF/spring.factories
- sofa-boot-project/sofa-boot/src/test/java/com/alipay/sofa/boot/util/ModuleUtilTests.java
Additional comments not posted (3)
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/Initializer/DynamicModuleExportApplicationContextInitializer.java (3)
1-16: LGTM!The license header is correctly formatted.
The code changes are approved.
17-22: LGTM!The package declaration and imports are appropriate.
The code changes are approved.
23-34: LGTM!The class
DynamicModuleExportApplicationContextInitializercorrectly implementsApplicationContextInitializer<ConfigurableApplicationContext>. Theinitializemethod is straightforward and calls a utility method to export JDK modules.The code changes are approved.
Signed-off-by: JermaineHua <crazyhzm@apache.org>
CrazyHZM
force-pushed
the
feat/dynamic_export_and_open
branch
from
88c7e01
to
97cc8c5
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (5)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/Initializer/DynamicModuleExportApplicationContextInitializer.java (1 hunks)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java (1 hunks)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/UnsafeUtil.java (1 hunks)
- sofa-boot-project/sofa-boot/src/main/resources/META-INF/spring.factories (1 hunks)
- sofa-boot-project/sofa-boot/src/test/java/com/alipay/sofa/boot/util/ModuleUtilTests.java (1 hunks)
Files skipped from review due to trivial changes (1)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java
Files skipped from review as they are similar to previous changes (4)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/Initializer/DynamicModuleExportApplicationContextInitializer.java
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/UnsafeUtil.java
- sofa-boot-project/sofa-boot/src/main/resources/META-INF/spring.factories
- sofa-boot-project/sofa-boot/src/test/java/com/alipay/sofa/boot/util/ModuleUtilTests.java
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1340 +/- ##
============================================
- Coverage 83.20% 82.93% -0.27%
- Complexity 2951 2972 +21
============================================
Files 337 340 +3
Lines 9739 9828 +89
Branches 1174 1176 +2
============================================
+ Hits 8103 8151 +48
- Misses 1125 1161 +36
- Partials 511 516 +5 ☔ View full report in Codecov by Sentry. |
.../java/com/alipay/sofa/boot/Initializer/DynamicModuleExportApplicationContextInitializer.java
Outdated
Show resolved
Hide resolved
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java
Outdated
Show resolved
Hide resolved
Signed-off-by: JermaineHua <crazyhzm@apache.org>
CrazyHZM
force-pushed
the
feat/dynamic_export_and_open
branch
from
2b16100
to
6d844ab
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Outside diff range and nitpick comments (1)
sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java (1)
218-227: Avoid usingUnsafeUtilfor reflection to enhance compatibilityThe use of
UnsafeUtilincreateModuleMethodHandle()andgetModuleLayerFieldsValue()relies on internal APIs, which can lead to compatibility issues across different Java versions. It's recommended to use standard reflection APIs orMethodHandlesprovided by the Java language to achieve the desired functionality without depending on unsafe operations.Also applies to: 235-245
| public static boolean invokeModuleMethod(MethodHandle method, String moduleName, | ||
| String packageName) { |
There was a problem hiding this comment.
Add null checks for method handles to prevent NullPointerException
The method parameter may be null if the method handle creation fails. Invoking method.invoke(...) without checking for null can lead to a NullPointerException. Please add a null check before using the method handle.
Apply this diff to add null checks:
public static boolean invokeModuleMethod(MethodHandle method, String moduleName,
String packageName) {
+ if (method == null) {
+ LOGGER.error("Method handle is null for module: {}, package: {}", moduleName, packageName);
+ return false;
+ }
Optional<Module> findModule = ModuleLayer.boot().findModule(moduleName);
if (findModule.isPresent()) {
try {
return invokeModuleMethod(method, findModule.get(), packageName);
} catch (Throwable t) {
LOGGER.error("Failed to invoke ModuleLayer method: {}", method, t);
}
}
return false;
}
public static boolean invokeModuleMethod(MethodHandle method, Module module, String packageName) {
+ if (method == null) {
+ LOGGER.error("Method handle is null for module: {}, package: {}", module.getName(), packageName);
+ return false;
+ }
try {
method.invoke(module, packageName);
return true;
} catch (Throwable t) {
LOGGER.error("Failed to invoke Module method: {}", method, t);
}
return false;
}Also applies to: 201-202
|
|
||
| private static final Map<String, Module> nameToModules; | ||
|
|
||
| private static boolean isExported = false; |
There was a problem hiding this comment.
Use separate flags for export methods to avoid unintended behavior
The isExported flag is shared between exportAllJDKModulePackageToAll() and exportAllModulePackageToAll(). If one method sets isExported to true, the other method will skip execution, which might not be the intended behavior. Consider using separate flags to control each method independently.
Apply this diff to introduce separate flags:
-private static boolean isExported = false;
+private static boolean isJdkModulesExported = false;
+private static boolean isAllModulesExported = false;
public static void exportAllJDKModulePackageToAll() {
try {
- if (!isExported && nameToModules != null) {
+ if (!isJdkModulesExported && nameToModules != null) {
nameToModules.forEach((name, module) -> module.getPackages().forEach(pkgName -> {
if (isJDKModulePackage(pkgName)) {
addOpensToAll(module, pkgName);
addExportsToAll(module, pkgName);
}
}));
}
- isExported = true;
+ isJdkModulesExported = true;
} catch (Throwable t) {
- isExported = false;
+ isJdkModulesExported = false;
LOGGER.error("Failed to export all JDK module package to all", t);
}
}
public static void exportAllModulePackageToAll() {
try {
- if (!isExported && nameToModules != null) {
+ if (!isAllModulesExported && nameToModules != null) {
nameToModules.forEach((name, module) -> module.getPackages().forEach(pkgName -> {
addOpensToAll(module, pkgName);
addExportsToAll(module, pkgName);
}));
}
- isExported = true;
+ isAllModulesExported = true;
} catch (Throwable t) {
- isExported = false;
+ isAllModulesExported = false;
LOGGER.error("Failed to export all module package to all", t);
}
}Also applies to: 63-63, 87-87
| return (Map<String, Module>) getModuleLayerFieldsValue("nameToModule"); | ||
| } |
There was a problem hiding this comment.
Handle potential ClassCastException in getNameToModule()
Casting the result of getModuleLayerFieldsValue("nameToModule") to (Map<String, Module>) may result in a ClassCastException if the field does not contain the expected type. Consider adding a type check before casting to ensure safety.
Apply this diff to safely handle the cast:
public static Map<String, Module> getNameToModule() {
- return (Map<String, Module>) getModuleLayerFieldsValue("nameToModule");
+ Object moduleMap = getModuleLayerFieldsValue("nameToModule");
+ if (moduleMap instanceof Map) {
+ return (Map<String, Module>) moduleMap;
+ } else {
+ LOGGER.error("Expected nameToModule to be a Map<String, Module>, but got: {}", moduleMap);
+ return null;
+ }
}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.
| return (Map<String, Module>) getModuleLayerFieldsValue("nameToModule"); | |
| } | |
| public static Map<String, Module> getNameToModule() { | |
| Object moduleMap = getModuleLayerFieldsValue("nameToModule"); | |
| if (moduleMap instanceof Map) { | |
| return (Map<String, Module>) moduleMap; | |
| } else { | |
| LOGGER.error("Expected nameToModule to be a Map<String, Module>, but got: {}", moduleMap); | |
| return null; | |
| } | |
| } |
CrazyHZM
force-pushed
the
feat/dynamic_export_and_open
branch
5 times, most recently
from
e95642b
to
3663c21
Compare
CrazyHZM
force-pushed
the
feat/dynamic_export_and_open
branch
from
3663c21
to
24ac036
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Outside diff range and nitpick comments (1)
sofa-boot-project/sofa-boot/src/test/java/com/alipay/sofa/boot/initializer/AutoModuleExportApplicationContextInitializerTests.java (1)
33-77: Consider adding tests for edge cases and combined configurations.The current test suite covers the main scenarios well. To further improve the test coverage, consider adding tests for:
- Combined configurations (e.g., both JDK and all module export enabled/disabled).
- Edge cases, such as invalid property values.
- Interaction with other related configurations or initializers, if any.
These additional tests would ensure robustness across a wider range of scenarios.
Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Files selected for processing (2)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java (1 hunks)
- sofa-boot-project/sofa-boot/src/test/java/com/alipay/sofa/boot/initializer/AutoModuleExportApplicationContextInitializerTests.java (1 hunks)
Files skipped from review as they are similar to previous changes (1)
- sofa-boot-project/sofa-boot/src/main/java/com/alipay/sofa/boot/util/ModuleUtil.java
Additional comments not posted (6)
sofa-boot-project/sofa-boot/src/test/java/com/alipay/sofa/boot/initializer/AutoModuleExportApplicationContextInitializerTests.java (6)
1-33: LGTM: File structure and imports are well-organized.The file has the appropriate license header, correct package declaration, and all necessary imports. The class is properly annotated with author and version information.
35-42: LGTM: Proper test setup.The
setUpmethod correctly resets theModuleUtilstate and initializes theApplicationContextRunnerwith theAutoModuleExportApplicationContextInitializer. This ensures a clean state before each test execution.
44-51: LGTM: Correct verification of default JDK module export behavior.The
jdkDefaultTruetest method effectively verifies thatexportAllJDKModulePackageToAllis called once when no specific properties are set, which is the expected default behavior.
53-59: LGTM: Correct verification of default all module export behavior.The
allDefaultFalsetest method effectively verifies thatexportAllModulePackageToAllis not called when no specific properties are set, which is the expected default behavior.
61-68: LGTM: Correct verification of disabled JDK module export behavior.The
jdkDisabletest method effectively verifies thatexportAllJDKModulePackageToAllis not called when the propertysofa.boot.auto.module.export.jdk.enableis set to false, which is the expected behavior when JDK module export is explicitly disabled.
70-76: LGTM: Correct verification of enabled all module export behavior.The
allEnabletest method effectively verifies thatexportAllModulePackageToAllis called once when the propertysofa.boot.auto.module.export.all.enableis set to true, which is the expected behavior when all module export is explicitly enabled.
Signed-off-by: JermaineHua <crazyhzm@apache.org>
CrazyHZM
force-pushed
the
feat/dynamic_export_and_open
branch
from
24ac036
to
065d200
Compare
Summary by CodeRabbit
Summary by CodeRabbit
New Features
Tests