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 <[email protected]> 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 <[email protected]> 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 <[email protected]> > wrote: > > > > > > > > On May 19, 2022, at 12:15 AM, Volkan Yazıcı <[email protected]> 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 > > > > > > > > > > > > >
