-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Use Path instead of a File for plugin.dir #26899
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
Conversation
Reviewer's GuideReplace legacy File usage with java.nio.file.Path for plugin directory handling, refactoring loading logic to leverage NIO APIs and updating tests accordingly. Class diagram for updated plugin directory handlingclassDiagram
class ServerPluginsProviderConfig {
- List<Path> installedPluginsDirs
+ List<Path> getInstalledPluginsDirs()
+ ServerPluginsProviderConfig setInstalledPluginsDirs(List<Path> installedPluginsDirs)
}
class ServerPluginsProvider {
- List<Path> installedPluginsDirs
- Executor executor
+ void loadPlugins(Loader loader, ClassLoaderFactory createClassLoader)
+ static List<URL> buildClassPath(Path path)
+ static List<Path> listFiles(Path path)
+ static URL fileToUrl(Path file)
}
class PluginLoader {
+ static List<Plugin> loadPlugins(List<Path> path)
}
class PluginReader {
- List<Path> pluginDirs
}
ServerPluginsProviderConfig --> ServerPluginsProvider : uses
PluginLoader --> ServerPluginsProviderConfig : uses
PluginReader --> PluginLoader : uses
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
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.
Hey there - I've reviewed your changes - here's some feedback:
- ServerPluginsProvider now only passes the plugin name instead of its full path to loader.load—consider passing the absolute Path (or original Path) as the identifier to maintain the previous behavior and avoid name collisions.
- PluginReader and PluginLoader still declare some File-based fields (rootPom, impactedModulesFile)—migrating these to Path would keep the API consistent across the plugin handling code.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- ServerPluginsProvider now only passes the plugin name instead of its full path to loader.load—consider passing the absolute Path (or original Path) as the identifier to maintain the previous behavior and avoid name collisions.
- PluginReader and PluginLoader still declare some File-based fields (rootPom, impactedModulesFile)—migrating these to Path would keep the API consistent across the plugin handling code.
## Individual Comments
### Comment 1
<location> `core/trino-main/src/main/java/io/trino/server/ServerPluginsProvider.java:56` </location>
<code_context>
- .map(file -> (Callable<?>) () -> {
- loader.load(file.getAbsolutePath(), () ->
- createClassLoader.create(file.getName(), buildClassPath(file)));
+ .filter(Files::isDirectory)
+ .map(Path::toAbsolutePath)
+ .map(path -> (Callable<?>) () -> {
</code_context>
<issue_to_address>
**issue (bug_risk):** Using Files::isDirectory may throw unchecked exceptions for inaccessible paths.
Handle potential exceptions from Files::isDirectory to prevent failures when accessing unreadable or restricted paths.
</issue_to_address>
### Comment 2
<location> `core/trino-main/src/main/java/io/trino/server/ServerPluginsProvider.java:87` </location>
<code_context>
}
- private static URL fileToUrl(File file)
+ private static URL fileToUrl(Path file)
{
try {
</code_context>
<issue_to_address>
**suggestion:** Renaming fileToUrl to pathToUrl would improve clarity.
The method parameter type has changed from File to Path, so updating the name to pathToUrl would better reflect its purpose.
</issue_to_address>
Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
.map(file -> (Callable<?>) () -> { | ||
loader.load(file.getAbsolutePath(), () -> | ||
createClassLoader.create(file.getName(), buildClassPath(file))); | ||
.filter(Files::isDirectory) |
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.
issue (bug_risk): Using Files::isDirectory may throw unchecked exceptions for inaccessible paths.
Handle potential exceptions from Files::isDirectory to prevent failures when accessing unreadable or restricted paths.
Just a cleanup
Description
Additional context and related issues
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text:
Summary by Sourcery
Refactor plugin directory handling to use java.nio.file.Path instead of java.io.File across core and testing modules.
Enhancements:
Tests: