Matt, some more questions/remarks:
1. AFAIU, plugin `key` is the normalized `name`. Right? If so, what is
the purpose of the `name`?
2. I was expecting `PluginEntry` to be simplified similar to `Plugin`
and only contain a `String key`, `String name`, and `String className`. For
`@Configurable`, we could create a `ConfigurablePlugin` class extending
from `Plugin` with extra `elementType`, `printable`, etc. fields. Generated
`Log4jPlugins.java` could instantiate either `Plugin` or
`ConfigurablePlugin` depending on the need. In its current state, `Plugin`
looks pretty generic, though the rest still resembles the past architecture.
3. What is the purpose of populating interfaces (i.e.,
`PluginEntry#interfaces`) at compile-time? What does use them and how?
4. I see quite some overlap between `plugins.condition` and
`plugins.validation`. I would prefer the latter to be modernized and merged
into the former.
5. I would prefer `Configurable`, `Node`, et al. to be moved to their
own `plugins.config` package.
6. I would prefer `di.Scope` et al. to be moved to their own
`plugins.scope` package – or at least, be located in the same package. For
instance, consider `plugins.di.Scope` vs. `plugins.ScopeType`.
On Tue, May 24, 2022 at 2:02 AM Matt Sicker <boa...@gmail.com> wrote:
> And to answer your initial question, something could be Configurable and
> not a Plugin, but that would only have the effect of putting it in the Core
> namespace. The rest of the annotation only has interpretation in
> Injector::configure.
>
> —
> Matt Sicker
>
> > On May 23, 2022, at 15:48, Matt Sicker <boa...@gmail.com> wrote:
> >
> > Configurable and Plugin were combined as a single annotation before,
> > but the Core-category/namespace-specific configurability options don't
> > apply to other types of plugins, so I separated them. The benefit of
> > separating them is so that people don't get the strange idea that it's
> > possible to use multiple namespaces of plugins to configure in a flat
> > namespace (i.e., we're not using XML namespaces, and the other formats
> > don't have a concept of namespaces). At this point, Plugin is
> > basically the same thing as Named, but we keep an index of Plugin
> > classes. Since we don't want to just scan and load every possible
> > bean-like class at startup, we need a way to distinguish them.
> >
> > In a sense, Configurable could potentially have a value() or name()
> > element for the plugin name, and that can work via a NameProvider (or
> > maybe through the recent annotation stereotyping code I added), though
> > the annotation processor would need to be updated to index them, too
> > (and supporting annotation stereotypes there is harder as the
> > annotation processing API is far more verbose when dealing with
> > dynamic annotations like that). In a sense, a plugin is kind of like a
> > Guice module or Spring configuration class that you still need to
> > import into your main config or other classes first before they're
> > activated.
> >
> > This also makes use of Plugin a bit simpler for non-configurable plugins.
> >
> > Note that this is mainly explaining how things work as of now, not
> > necessarily how they must be. Thanks for continuing to examine the
> > architecture here! If you have ideas on how you'd like the use of an
> > ideal DI API would look, that also helps as I can look into how
> > feasible it is to support. Some things are simpler than others to port
> > over, too, from other DI systems, while others are beyond the scope
> > I'd feel comfortable shipping. For example, advanced DI libraries like
> > Spring tend to use bytecode manipulation to support proxying
> > non-interfaces at runtime (such as via cglib, asm, bytebuddy,
> > whatever) in order to support interceptors, decorators, and other AOP
> > stuff, while I don't want to add that type of dependency to
> > log4j-{api,plugins,core}. If it's possible to implement via Java 11
> > standard modules (including java.compiler for the annotation
> > processor, though perhaps even the jdk modules that go with it if we
> > want to do more advanced Lombok-like things), then that's something to
> > consider.
> >
> > See also some interesting looking modules that come with Java that I
> > never really noticed before (I mean, they seem relatively new, but a
> > version of 9 could mean that was when the _module_ was introduced and
> > not the API itself):
> > -
> https://docs.oracle.com/en/java/javase/11/docs/api/jdk.dynalink/module-summary.html
> > -
> https://docs.oracle.com/en/java/javase/11/docs/api/jdk.compiler/module-summary.html
> >
> >> On Mon, May 23, 2022 at 2:54 PM Volkan Yazıcı <vol...@yazi.ci> wrote:
> >>
> >> Matt, maybe I am not getting it but... I understand your explanation
> about
> >> Configurable-vs-Plugin, yet my question still stands: Can something be
> >> `@Configurable` but not `@Plugin`? If I am not mistaken, every single
> >> `@Configurable` usage is followed by a `@Plugin`. Hence, I am inclined
> to
> >> make `Configurable` extend from `Plugin`. This will not only simplify
> the
> >> call-site, but will communicate a more clear message: this is a plugin
> that
> >> will be configured. I can also reverse my question: What would be the
> >> benefit of keeping `@Configurable` and `@Plugin` separate?
> >>
> >>> On Sun, May 22, 2022 at 11:49 PM Matt Sicker <boa...@gmail.com> wrote:
> >>>
> >>> Those lazy wrappers are an insignificant detail. Feel free to simplify.
> >>>
> >>> Configurable is a namespace annotation, not a name annotation. Plugin
> is a
> >>> name annotation that gets indexed.
> >>>
> >>> If you’d like fancier conditional annotations, please describe what
> parts
> >>> you want. I don’t want to port over every feature of Spring’s DI
> system as
> >>> it’s fairly extensive and mostly over complex for our use case.
> >>>
> >>> —
> >>> Matt Sicker
> >>>
> >>>> On May 22, 2022, at 15:36, Volkan Yazıcı <vol...@yazi.ci> wrote:
> >>>>
> >>>> Great work Matt! I have some questions/remarks:
> >>>>
> >>>> 1. Why did we introduce `LazyInt`, `LazyBoolean`, etc. rather than
> >>>> simply leveraging `Lazy<V>`? If the concern is nullability, we could
> >>> have
> >>>> checked the `supplier` response against `null` in `Lazy<V>` itself.
> >>>> 2. Why is `Configurable` not annotated with `Plugin`? Can something
> be
> >>>> `@Configurable` but not `@Plugin`?
> >>>> 3. `ConditionalOnProperty` is missing `havingValue` and
> >>> `matchIfMissing`.
> >>>>
> >>>>
> >>>>> On Fri, May 20, 2022 at 11:37 PM Matt Sicker <boa...@gmail.com>
> wrote:
> >>>>>
> >>>>> By the way, I hope my latest commit renaming categories to namespaces
> >>>>> and moving some annotation metadata around should help clarify the
> >>>>> scope of things. In particular, I made an alias annotation for Core
> >>>>> category/namespace plugins called @Configurable which makes their use
> >>>>> case more obvious. Similarly, I moved the Core-specific annotation
> >>>>> data from @Plugin to @Configurable since they only apply there. Now
> >>>>> that the category is in the @Namespace annotation, @Plugin only has a
> >>>>> single value() string to set which is the name of the plugin (which
> >>>>> will default to using the simple class name if the plugin name is an
> >>>>> empty string; I've removed explicit plugin names on classes whose
> >>>>> simple names already match their plugin names). Note that you can
> >>>>> still define namespace alias annotations for different namespaces
> >>>>> (I've only made them for Core, Lookup, and TypeConverter namespaces
> so
> >>>>> far).
> >>>>>
> >>>>> Some next steps to make the system more consistent:
> >>>>> * @ConditionalOnFoo annotations for injectable bundles (i.e., the
> >>>>> equivalent to a @Configuration class in Spring)
> >>>>> * Ability to inject PluginType<T> for plugin namespaces that
> shouldn't
> >>>>> eagerly load available plugins
> >>>>> * Further cleanup and potential supporting APIs to apply inversion of
> >>>>> control rather than calling Injector APIs
> >>>>> * Figuring out how Injector may interact with or otherwise set up
> >>>>> things like PropertySource services or other log4j-api services. This
> >>>>> might be doable via a sort of "DI promise" API where the DI system
> >>>>> will complete the promise or some variation of lazy async loading
> >>>>>
> >>>>> From there, hopefully any remaining limitations or code smells will
> be
> >>>>> obvious enough to fix up before 3.0.0.
> >>>>>
> >>>>>> On Thu, May 19, 2022 at 3:06 PM Matt Sicker <boa...@gmail.com>
> wrote:
> >>>>>>
> >>>>>> I’d like to add more ConditionalOn annotations for the @Factory
> >>> methods,
> >>>>> or at least something like “if no binding exists for this key
> already,
> >>> here
> >>>>> it is” which is analogous to @ConditionalOnMissingBean in Spring.
> That
> >>>>> would make the DefaultCallback code easier to write without using the
> >>>>> Injector API directly. That will also make a good example for users
> to
> >>> copy
> >>>>> from when making their own InjectorCallback customizations.
> >>>>>>
> >>>>>> I do need to look more closely at the API module to see how
> injection
> >>>>> can work without pulling DI APIs up. Might need to define some
> >>>>> ServiceLoader stuff there like an ordering annotation.
> >>>>>>
> >>>>>> —
> >>>>>> Matt Sicker
> >>>>>>
> >>>>>> On May 19, 2022, at 10:47, Ralph Goers <ralph.go...@dslextreme.com>
> >>>>> wrote:
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>> On May 19, 2022, at 12:15 AM, Volkan Yazıcı <vol...@yazi.ci>
> wrote:
> >>>>>>
> >>>>>>
> >>>>>> In the last couple of weeks, I have been interrogating Matt on why
> and
> >>>>> how
> >>>>>>
> >>>>>> of the 3.x plugin infra. This inevitably led to some code
> archaeology.
> >>> I
> >>>>>>
> >>>>>> will try to share my take out of this exercise.
> >>>>>>
> >>>>>>
> >>>>>> 1.x required users to type the fully-qualified class names of
> >>> components
> >>>>>>
> >>>>>> (appenders, layouts, etc.) in either configuration files or
> properties:
> >>>>>>
> >>>>>>
> >>>>>> log4j.appender.A1=org.apache.log4j.ConsoleAppender
> >>>>>>
> >>>>>> log4j.appender.A1.layout=org.apache.log4j.PatternLayout
> >>>>>>
> >>>>>>
> >>>>>> Plugins were first introduced in 2.x to address this concern:
> >>>>>>
> >>>>>>
> >>>>>> It wasn’t a concern. It was an annoyance.
> >>>>>>
> >>>>>> providing a
> >>>>>>
> >>>>>> mechanism to alias components. Later on it also got used to glue the
> >>> rest
> >>>>>>
> >>>>>> of the configuration components together. Consequently maintainers
> >>>>> started
> >>>>>>
> >>>>>> exercising this throughout the entire code base, whenever something
> >>>>> needed
> >>>>>>
> >>>>>> to be indirectly injected somewhere.
> >>>>>>
> >>>>>>
> >>>>>> Only where we thought it made sense. Some things use system
> properties,
> >>>>>> some things use ServiceLoader.
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>> That was a great epiphany and I am in love with the plugins! It
> feels
> >>>>> like
> >>>>>>
> >>>>>> Log4j is composed of simple beans beautifully practicing
> >>>>>>
> >>>>>> separation-of-concerns while running on a marvellous Spring-like
> >>>>>>
> >>>>>> fully-fledged dependency injection (DI) framework... almost... sort
> >>> of...
> >>>>>>
> >>>>>> The reality is sadly a little bit different than that. In
> particular, I
> >>>>> see
> >>>>>>
> >>>>>> two major issues:
> >>>>>>
> >>>>>>
> >>>>>> 1. Missing `@ConditionalOn*` support
> >>>>>>
> >>>>>>
> >>>>>> I am certain I added the ability to do this in 3.0. I added
> constraint
> >>>>> checking
> >>>>>> at the class level where previously it was only available on
> >>> parameters.
> >>>>> That
> >>>>>> said, we currently only have RequiredClass and RequiredProperty
> >>>>> validators.
> >>>>>>
> >>>>>> 2. Static access to DI
> >>>>>>
> >>>>>>
> >>>>>> I guess Matt is already working on issue #1. He is trying to make
> sure
> >>>>>>
> >>>>>> `@Required` et al. annotations are executed the same way at every
> >>>>> injection
> >>>>>>
> >>>>>> site.
> >>>>>>
> >>>>>>
> >>>>>> What do I mean with the static access to DI? In a `@Bean`-annotated
> >>>>> method
> >>>>>>
> >>>>>> of a Spring application, do you create your own `ApplicationContext`
> >>>>>>
> >>>>>> instance and access other beans from that? Do you statically access
> >>>>>>
> >>>>>> `ApplicationContext.getBean("foo")`? Certainly not! Though these two
> >>>>>>
> >>>>>> examples are what we exactly do in Log4j. We create single-use
> >>>>>>
> >>>>>> `PluginManager` instances and use it to collect plugins. We call
> >>>>>>
> >>>>>> `DI.createInjector().getInstance("foo")`. What we should be rather
> >>> doing
> >>>>> is
> >>>>>>
> >>>>>> to inject the `ApplicationContext` and/or the beans we are
> interested
> >>> in,
> >>>>>>
> >>>>>> that is, in Log4j terms, inject the `Injector` and/or the plugins we
> >>> are
> >>>>>>
> >>>>>> interested in.
> >>>>>>
> >>>>>>
> >>>>>> Thoughts?
> >>>>>>
> >>>>>>
> >>>>>> What you are suggesting is that the injector should just be another
> >>> bean
> >>>>>> that can be injected. I have no problem with that.
> >>>>>>
> >>>>>> I should mention that I asked Matt to look into an issue I have with
> >>> the
> >>>>> Spring
> >>>>>> support. Spring needs access to its Environment. We currently save
> that
> >>>>> in
> >>>>>> the LoggerContex. However, the SpringPropertySource has a
> circularity
> >>>>> problem.
> >>>>>> PropertySources are created before anything else in Log4j, including
> >>> the
> >>>>> Injector.
> >>>>>> The Spring support currently uses an EnvironmentHolder singleton.
> I’d
> >>>>> really like
> >>>>>> to do a PR for Spring to move the Spring Boot code there but I am
> not
> >>>>> comfortable
> >>>>>> doing it with the EnvironmentHolder.
> >>>>>>
> >>>>>> What happens how is that the EnvironmentHolder checks to see if
> Log4j
> >>>>> has initialized.
> >>>>>> If it hasn’t it returns null. If it has then it accesses the
> >>>>> LoggerContext to get the Environment.
> >>>>>> This just feels clunky. What I would prefer is to have the
> >>>>> PropertySource be injected
> >>>>>> with the Environment when it becomes available. In essence, deferred
> >>>>> injection.
> >>>>>>
> >>>>>> Ralph
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>>
> >>>>>
> >>>
>
