ppkarwasz commented on PR #3839: URL: https://github.com/apache/logging-log4j2/pull/3839#issuecomment-3211804379
> > * Implementations are **inconsistent** today: some factories always report active but conditionally restrict types (e.g., Log4j 1 XML returns `xml` only when a property is set), while others return a constant type set but flip `isActive()` based on optional deps. A single public method lets us **normalize** that behavior and present a stable contract. > > What you're describing is not a problem, but a behavior and it is all good, AFAICT. It is up to the `CF` what to return from `isActive()`. It makes sense `CF.getFactories()` filtered on `isActive()` yields a different outcome when a user has enabled/disabled Log4j 1 support. As a matter of fact, this is a powerful feature that allows `CF`s to dynamically determine their active status. The inconsistency is in the way a factory communicates that it is inactive: - If [`o.a.log4j.config.PropertiesConfigurationFactory`](https://github.com/apache/logging-log4j2/blob/2.x/log4j-1.2-api/src/main/java/org/apache/log4j/config/PropertiesConfigurationFactory.java) is **not** active, then `isActive()` return `true`, but `getSupportedTypes()` returns `null`. - On the other hand if [`YamlConfigurationFactory`](https://github.com/apache/logging-log4j2/blob/2.x/log4j-core/src/main/java/org/apache/logging/log4j/core/config/yaml/YamlConfigurationFactory.java) is **not** active, then `isActive()` returns `false`, while `getSupportedTypes()` returns `[".yml", ".yaml"]`. My point is that, as they are now implemented, it does **not** make sense to make these methods public. > Another concern I have regarding adding a new, as a matter of fact, 2nd after `getSupportedTypes()`, _"supported file extensions"_ concept to `CF` is that it reinforces the idea of a `CF` must be associated with a _file_, whereas it actually is not necessarily. I agree completely: a `ConfigurationFactory` **should not** be inherently tied to a file. Unfortunately, the current discovery mechanism assumes the presence of a file resource. A good example is [`SpringBootConfigurationFactory`](https://github.com/spring-projects/spring-boot/blob/main/core/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/SpringBootConfigurationFactory.java), which only works if a dummy `log4j2.springboot` file is present. > > * `isActive()` and `getSupportedTypes()` are **protected**, not public, so they’re not available to consumers; exposing them (or a raw `getFactories()`) would leak internal details. > > Agreed. Let's make `isActive()` public. > [...] > > * In Log4j 3, `isActive()` isn’t needed anymore: optional Maven **modules** provide support for additional formats, not per-factory optional dependencies, so I would propose to remove `isActive()` from Log4j 3. > > That is a good idea. I don’t think we should make either `isActive()` or `getSupportedTypes()` public. * **`isActive()`**: aside from its inconsistent semantics today (which we could technically fix), we’ve already agreed it should disappear in Log4j Core 3 where the concept no longer applies. Therefore we can not propose its usage to Spring Boot if we want them to handle both Log4j Core 2 and 3. * **`getSupportedTypes()`**: as you noted, this reinforces the idea that every `ConfigurationFactory` must be file-based, which is not always true. Exposing it publicly would just entrench that misconception further. ### Alternative proposal Looking again at [`Log4j2LoggingSystem`](https://github.com/spring-projects/spring-boot/blob/main/core/spring-boot/src/main/java/org/springframework/boot/logging/log4j2/Log4J2LoggingSystem.java), I realized that the `getSupportedExtensions` method I suggested is really addressing an *XY problem*: I proposed expanding our API surface only to accommodate how Spring Boot **currently** interacts with `ConfigurationFactory`. If we take a step back, Spring Boot doesn’t actually need to know about multiple factories, or how we handle extensions and precedence. What it needs boils down to: 1. **User-specified config (`logging.config`, plus optional `logging.log4j2.config.override`):** Spring Boot should resolve those locations to `URL`s and call `ConfigurationFactory.getConfiguration(LoggerContext, String, URI)`. If overrides are provided, they need a way to combine them. That suggests introducing a dedicated method like `getConfiguration(LoggerContext, String, List<URI>)` so they don't need to touch our “private” API. 2. **No explicit config provided:** Today, Spring Boot checks for “standard” configuration files. In reality, they could just verify whether Log4j Core has already been initialized with a non-default configuration: ```java !config.getConfigurationSource().equals(ConfigurationSource.NULL) ``` If it’s still the default config, they know they should look for alternatives. 3. **Spring-specific defaults (e.g., `log4j2-spring.xml`):** This is where they currently rely on extensions again. But they could instead let Log4j Core resolve it via the `name` parameter: ```java ConfigurationFactory.getInstance().getConfiguration(ctx, "-spring", null); ``` 4. **Fallback:** If all of the above fail, they fall back to their standard configuration file. **TL;DR:** Spring Boot does not actually need to know what extensions we support. With the existing `ConfigurationFactory.getConfiguration` API (plus potentially a small addition for composite configs), they can cover all their use cases. The only real gap is an ergonomic way to build composite configurations; everything else can stay encapsulated inside Log4j Core. @yybmion, apologies that your PR turned into a broader discussion about `ConfigurationFactory`’s shortcomings. That said, having this concrete example was valuable: it helped us see that the path we were considering (exposing supported extensions) may not be the right one after all. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
