[Stable plugin API] Load plugin named components #89969
Conversation
Stable plugins are using @extensible and @NamedComponents annotations to mark components to be loaded. This commit is loading extensible classNames from extensibles.json and named components from named_components.json relates elastic#88980
pgomulka
added
>enhancement
:Core/Infra/Plugins
|
Pinging @elastic/es-core-infra (Team:Core/Infra) |
|
Hi @pgomulka, I've created a changelog YAML for you. |
…ticsearch into plugin_scanning_just_files
server/src/main/java/org/elasticsearch/plugins/scanners/NameToPluginInfo.java
Outdated
Show resolved
Hide resolved
server/src/main/java/org/elasticsearch/plugins/scanners/StablePluginsRegistry.java
Outdated
Show resolved
Hide resolved
ChrisHegarty
left a comment
ChrisHegarty
left a comment
There was a problem hiding this comment.
Mostly looks good, just a few stray comments
| "org.elasticsearch.plugin.analysis.api.CharFilterFactory":"org.elasticsearch.plugin.analysis.api.CharFilterFactory", | ||
| "org.elasticsearch.plugin.analysis.api.TokenFilterFactory":"org.elasticsearch.plugin.analysis.api.TokenFilterFactory", | ||
| "org.elasticsearch.plugin.analysis.api.TokenizerFactory":"org.elasticsearch.plugin.analysis.api.TokenizerFactory" | ||
| } |
There was a problem hiding this comment.
Maybe locate this resource file in a directory structure that closer resembles its usage, say org/elasticsearch/plugins/scanner ?
| private static final String EXTENSIBLES_FILE = "extensibles.json"; | ||
| public static final ExtensiblesRegistry INSTANCE = new ExtensiblesRegistry(EXTENSIBLES_FILE); | ||
|
|
||
| private final ExtensibleFileReader extensibleFileReader; |
There was a problem hiding this comment.
This field can be dropped, no?
|
|
||
| public NamedComponentReader() { | ||
| this.extensiblesRegistry = ExtensiblesRegistry.INSTANCE; | ||
| } |
There was a problem hiding this comment.
let's chain this constructor, so we only have one substantive constructor body, e.g. this(ExtensiblesRegistry.INSTANCE)
| /** | ||
| * a registry of known classes marked or indirectly marked (extending marked class) with @Extensible | ||
| */ | ||
| private final ExtensiblesRegistry extensiblesRegistry; |
There was a problem hiding this comment.
at this scope of the PR it is not. We do not perform the scanning and we also do not register known extensibles (it will be its primary use). In NamedComponentReader it could be used for validating the read file. will add some simple validation
| Map<String, NameToPluginInfo> res = new HashMap<>(); | ||
|
|
||
| try (var json = new BufferedInputStream(Files.newInputStream(namedComponent))) { | ||
| try (XContentParser parser = JSON.xContent().createParser(XContentParserConfiguration.EMPTY, json)) { |
There was a problem hiding this comment.
A single try-with-resources should be sufficient here (rather than two)
| public Map<String, String> readFromFile() { | ||
| Map<String, String> res = new HashMap<>(); | ||
| // todo should it be BufferedInputStream ? | ||
| try (InputStream in = getClass().getResourceAsStream(extensibleFile)) { |
There was a problem hiding this comment.
I was wondering if getClass().getClassLoader().getResourceAsStream could be used here (no preference, but wanted to see the difference..)
what I found is that:
getClass().getResourceAsStream("/org/elasticsearch/plugins/scanners/extensibles.json) works without any modifications to module-info.java
but
getClass().getClassLoader().getResourceAsStream("org/elasticsearch/plugins/scanners/extensibles.json")
only works when opens org.elasticsearch.plugins.scanners; is added to module-info.java
this is mentioned in ClassLoader javadoc
Additionally, and except for the special case where the resource has a name ending with ".class", this method will only find resources in packages of named modules when the package is opened unconditionally (even if the caller of this method is in the same module as the resource).
https://docs.oracle.com/javase/9/docs/api/java/lang/ClassLoader.html#getResource-java.lang.String-
Class's javadoc does not seem to be that restrictive
Resources in named modules are subject to the rules for encapsulation specified in the Module getResourceAsStream method and so this method returns null when the resource is a non-".class" resource in a package that is not open to the caller's module.
@ChrisHegarty any ideas why the JDK did this that way? I suppose that Class#getResource is more preferred to Classloader#getResource ?
There was a problem hiding this comment.
@pgomulka The difference is clear from the javadoc/spec - j.l.Class::getResourceAsStream is caller-sensitive and determines the resource access based on the module of the immediate caller. Where as j.l.ClassLoader::getResourceAsStream is not caller-sensitive, therefore cannot determine resource access so behaves with no regard to the immediate caller. Ok, but why?
Generally speaking, caller-sensitive methods should not be extensible, as they pose problems for the implementing class, as well as opening the possibility to non-conformance. The method in j.l.Class is not extensible, so can be caller-sensitive. The method in ClassLoader is extensible, so is not caller-sensitive.
4 participants
Stable plugins are using @ extensible and @ NamedComponents annotations
to mark components to be loaded.
This commit is loading extensible classNames from extensibles.json and
named components from named_components.json
The scanning mechanism that can generate these files will be done later in a gradle plugin/plugin installer
relates #88980