[
https://issues.apache.org/jira/browse/LOG4J2-3496?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17537245#comment-17537245
]
Matt Sicker commented on LOG4J2-3496:
-------------------------------------
I'll note there's still work to be done to update various plugin usage to
inject plugins this way along with some incompatible assumptions in various
plugin types preventing a straightforward migration. Some examples:
* {{ConfigurationFactory}} currently relies on the existing {{@Order}}
annotation, while plugins now have the generic {{@PluginOrder}} annotation.
* {{InjectorCallback}} services (and likely some other services) can be
migrated to plugins, but those will require switching to {{{}@PluginOrder{}}},
too.
* {{Interpolator}} is currently written to lazily load {{StrLookup}} plugins.
In order to emulate this behavior in an injectable style, this would likely
require injecting a {{Map<String, Supplier<? extends T>>}} style collection
rather than eagerly instantiating them.
* {{Watcher}} plugins currently expect all plugin implementations to have a
specific constructor signature and can't be eagerly injected without the
relevant args. Would likely need something similar to what {{Interpolator}}
would need.
* {{TypeConverter}} plugins could potentially be loaded like this, but we
store them in a dedicated map inside {{DefaultInjector}} that caches types with
converters as they're used. We don't keep beans of type converters. This is
vaguely similar to the Guice API, though I'm not sure what limitations they
impose on their type converters (or really how pervasive they are in Guice
compared to Log4j Core plugins).
* {{SecretKeyProvider}} can likely be migrated.
* {{BuilderManager}} might be able to be migrated.
* {{PatternConverter}} plugins might be tricker to migrate, but probably not a
big deal given that these types of plugins shouldn't be doing anything complex
since they're invoked in hot code paths.
* None of the plugins in {{JsonTemplateLayout}} name use plugin names matching
their logical names (i.e., they're usually named something like
{{FooBarFactory}} with a logical name of {{{}fooBar{}}}). While I tried
migrating this last night (making the plugin name match the
{{TemplateResolverFactory::getName}} value), this didn't seem sufficient for
relying on injecting a map of plugins as initially described in this issue. I
was able to take a list of plugins, though, then collect them into a map. One
key problem here is that plugin names are case-insensitive (and stored in
lowercase for map keys typically), though template resolver factory names
appear to be case-sensitive, so my refactor didn't work. Now if we introduced a
sort of case-insensitive variant of {{{}LinkedHashMap{}}}, then I could inject
that and have things work.
Overall, though, this general feature request has turned out to be fairly
promising, and it's making me reconsider how some other systems here work. For
example, we may not need nearly as many {{ServiceLoader}} things when they can
be instantiated via {{Injector}} via different plugin categories. Some of the
challenge here has been in gently migrating code to inversion of control. In
JTL, for example, inverting some dependencies to inject plugins would require
updating like 1000 unit tests, too (which also helps show the testability
related functionality desired here; should be able to create test fixtures with
the subset of functionality needed rather than loading the entire system every
time which also makes it much easier to mock or stub dependencies in tests).
However, I suspect that [~vy] will have a better idea on what he wants to do
around simplification here as things become more inverted. The existing code is
structured fairly cleanly which made it easy enough to integrate with some new
features as I added them, but I imagine you might have a better idea on how far
to go with that now that {{JsonTemplateLayout}} can inject plugins by category.
> Support injection via container types
> -------------------------------------
>
> Key: LOG4J2-3496
> URL: https://issues.apache.org/jira/browse/LOG4J2-3496
> Project: Log4j 2
> Issue Type: New Feature
> Components: Plugins
> Reporter: Volkan Yazici
> Assignee: Matt Sicker
> Priority: Major
> Fix For: 3.0.0
>
>
> Plugin system should support the injection via container types. Since this is
> not currently the case, whenever a, for instance, collection of plugins need
> to be injected (e.g., Pattern and JSON Template Layouts), ad-hoc access to
> {{PluginManager}} or {{PluginUtils}} is needed. This approach has certain
> drawbacks:
> * Duplication of ad-hoc code
> * Not aligned with the way plugins are loaded, that is, via {{@Inject}}
> * Impossible to test
> h3. Container types
> Ideally, the plugin system should support injection via the following
> container types:
> * {{Optional<P>}}
> * {{Collection<P>}}
> * {{Iterable<P>}}
> * {{Set<P>}}
> * {{Stream<P>}}
> * {{List<P>}}
> * {{Map<N, P>}}
> Above {{P}} denotes the type the plugin is assignable from and {{N}} denotes
> the plugin name.
> h3. Ordering problem
> A {{@PluginOrder}} annotation needs to be introduced to address the following
> corner cases:
> * In the case of {{Optional<P>}}, if there are multiple matches, the first
> one needs to be picked.
> * In the case of collection and map types, elements need to be sorted.
> Sorting can be done in the following way:
> * If one or more of the matches are annotated with {{@PluginOrder}}, they
> will have the precedence and get sorted according to {{@PluginOrder}} values.
> * The rest will be sorted according to their discovery order.
> The order in collection types can be preserved using the following
> implementations:
> * {{Set}} -> {{LinkedHashSet}}
> * {{Map}} -> {{LinkedHashMap}}
> * {{Collection}}, {{Iterable}}, {{Stream}} -> {{List}}
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
