-
Notifications
You must be signed in to change notification settings - Fork 3.5k
remove dependency on org.reflections #18295
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
🤖 GitHub commentsExpand to view the GitHub comments
Just comment with:
|
|
This pull request does not have a backport label. Could you fix it @jsvd? 🙏
|
💛 Build succeeded, but was flaky
Failed CI Steps |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR removes the dependency on org.reflections library and replaces it with a custom lightweight package scanner. The change eliminates verbose logging from the reflections library while maintaining the same plugin discovery functionality.
Key Changes:
- Implemented a custom
PackageScannerclass to scan for annotated plugin classes - Refactored
PluginRegistryto use the new scanner instead of the reflections library - Removed the
org.reflections:reflectionsdependency from build configuration and licensing files
Reviewed Changes
Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
logstash-core/src/main/java/org/logstash/plugins/discovery/PackageScanner.java |
New custom scanner implementation for finding annotated classes in packages |
logstash-core/src/main/java/org/logstash/plugins/discovery/PluginRegistry.java |
Updated to use PackageScanner instead of reflections library, simplified plugin discovery logic |
logstash-core/src/test/java/org/logstash/plugins/discovery/PluginRegistryTest.java |
New test suite verifying plugin discovery works for built-in plugins |
logstash-core/build.gradle |
Removed org.reflections:reflections dependency |
tools/dependencies-report/src/main/resources/licenseMapping.csv |
Removed license mapping entry for reflections |
tools/dependencies-report/src/main/resources/notices/org.reflections!reflections-NOTICE.txt |
Removed license notice file |
NOTICE.TXT |
Removed reflections library attribution |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| } else if ("jar".equals(protocol) || resource.toString().contains("!")) { | ||
| scanJar(resource, resourcePath, classNames); |
Copilot
AI
Oct 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The check resource.toString().contains(\"!\") is fragile and could match false positives. Consider using a more robust approach to detect JAR URLs, such as checking if the URL can be cast to JarURLConnection.
| } else if ("jar".equals(protocol) || resource.toString().contains("!")) { | |
| scanJar(resource, resourcePath, classNames); | |
| } else if ("jar".equals(protocol)) { | |
| scanJar(resource, resourcePath, classNames); | |
| } else { | |
| // Attempt to detect JAR resources more robustly | |
| try { | |
| if (resource.openConnection() instanceof JarURLConnection) { | |
| scanJar(resource, resourcePath, classNames); | |
| } | |
| } catch (IOException e) { | |
| throw new UncheckedIOException("Failed to open connection for resource: " + resource, e); | |
| } |
|
|
||
| private static void scanJar(URL resource, String resourcePath, Set<String> classNames) { | ||
| try { | ||
| JarURLConnection connection = (JarURLConnection) resource.openConnection(); |
Copilot
AI
Oct 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[nitpick] Consider adding a comment explaining why caching is disabled for JAR connections, as this may have performance implications during plugin discovery.
| JarURLConnection connection = (JarURLConnection) resource.openConnection(); | |
| JarURLConnection connection = (JarURLConnection) resource.openConnection(); | |
| // Disable caching to prevent file locking issues (especially on Windows) and to ensure | |
| // that the JAR file can be closed and deleted after use. |
| if (annotation == null) { | ||
| continue; | ||
| } |
Copilot
AI
Oct 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The null check for annotation at line 87 is redundant since scanForAnnotation already filters for classes with the LogstashPlugin annotation. This check can be removed to simplify the code.
| if (annotation == null) { | |
| continue; | |
| } |
This removes the annoying log info:
Given we only use reflectiong to grab locally installed java plugin classes by annotation, we implement here a tiny scanner to do the same, and then we can remove the dependency.
exhaustive test suite run: https://buildkite.com/elastic/logstash-exhaustive-tests-pipeline/builds/2701/steps/canvas