Skip to content

Add API to list supported configuration file extensions #3839

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: 2.x
Choose a base branch
from

Conversation

yybmion
Copy link

@yybmion yybmion commented Jul 21, 2025

Summary

Add ConfigurationFactory.getActiveFileExtensions() and getSupportedExtensions() methods to allow external frameworks like Spring Boot to determine which configuration file formats are currently supported at runtime.

  • Add abstract getSupportedExtensions() method to ConfigurationFactory
  • Add static getActiveFileExtensions() method to aggregate results
  • Implement getSupportedExtensions() in all ConfigurationFactory subclasses
  • Add test coverage for new functionality

Example Usage

List<String> supportedExtensions = ConfigurationFactory.getActiveFileExtensions();
// Returns: ["xml", "json", "jsn", "properties"] (if Jackson available)
// Returns: ["xml", "properties"] (if Jackson missing)

closes #3775

Checklist

Before we can review and merge your changes, please go through the checklist below. If you're still working on some items, feel free to submit your pull request as a draft—our CI will help guide you through the remaining steps.

✅ Required checks

  • License: I confirm that my changes are submitted under the Apache License, Version 2.0.

  • Commit signatures: All commits are signed and verifiable. (See GitHub Docs on Commit Signature Verification).

  • Code formatting: The code is formatted according to the project’s style guide.

    How to check and fix formatting
    • To check formatting: ./mvnw spotless:check
    • To fix formatting: ./mvnw spotless:apply

    See the build instructions for details.

  • Build & Test: I verified that the project builds and all unit tests pass.

    How to build the project

    Run: ./mvnw verify

    See the build instructions for details.

🧪 Tests (select one)

  • I have added or updated tests to cover my changes.
  • No additional tests are needed for this change.

📝 Changelog (select one)

  • I added a changelog entry in src/changelog/.2.x.x. (See Changelog Entry File Guide).
  • This is a trivial change and does not require a changelog entry.

Copy link

github-actions bot commented Jul 21, 2025

Job Requested goals Build Tool Version Build Outcome Build Scan®
build-macos-latest clean install 3.9.8 Build Scan PUBLISHED
build-ubuntu-latest clean install 3.9.8 Build Scan PUBLISHED
build-windows-latest clean install 3.9.8 Build Scan PUBLISHED
Generated by gradle/develocity-actions

@yybmion
Copy link
Author

yybmion commented Aug 19, 2025

Hi @ppkarwasz , Just a gentle reminder — PTAL when you have time.

@vy
Copy link
Member

vy commented Aug 20, 2025

@ppkarwasz, would you mind reminding me why we decided to introduce a new API such that

  1. It duplicates quite some logic (e.g., in JsonConfigurationFactory, extensions and suffixes are semantically identical)
  2. Supported configuration file extensions could be obtained from ConfigurationFactory.getFactories() by filtering on isActive() and collecting getSupportedTypes() if only getFactories() would have existed

Put another way, why don't we add a static getFactories() to CF and be done with it?

@ppkarwasz
Copy link
Contributor

  1. It duplicates quite some logic (e.g., in JsonConfigurationFactory, extensions and suffixes are semantically identical).

Totally agree. Let’s put a default implementation in ConfigurationFactory so most factories don’t have to repeat themselves. Something like:

@Override
public List<String> getSupportedExtensions() {
    final String[] types = getSupportedTypes(); // protected, legacy API
    // Preserve legacy semantics: only advertise extensions when active and types are present.
    return isActive() && types != null && types.length > 0
            ? Arrays.asList(types)
            : Collections.emptyList();
}
  1. Supported configuration file extensions could be obtained from ConfigurationFactory.getFactories() by filtering on isActive() and collecting getSupportedTypes() if only getFactories() would have existed. Put another way, why don't we add a static getFactories() to CF and be done with it?

I prefer the dedicated getSupportedExtensions() API for a few reasons:

  • isActive() and getSupportedTypes() are protected, not public, so they’re not available to consumers; exposing them (or a raw getFactories()) would leak internal details.
  • 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.
  • 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.
  • Conceptually, ConfigurationFactory should look like a singleton service; how it multiplexes concrete factories is an implementation detail. Exposing getFactories() would couple users to that detail, whereas getSupportedExtensions() gives them exactly what they need without exposing internals.

Add getSupportedExtensions()
methods to allow external frameworks like Spring Boot to determine which
configuration file formats are currently supported at runtime.

- Add abstract getSupportedExtensions() method to ConfigurationFactory
- Implement getSupportedExtensions() in all ConfigurationFactory subclasses
- Add comprehensive test

closes apache#3775
@vy
Copy link
Member

vy commented Aug 21, 2025

  1. It duplicates quite some logic (e.g., in JsonConfigurationFactory, extensions and suffixes are semantically identical).

Totally agree. Let’s put a default implementation in ConfigurationFactory so most factories don’t have to repeat themselves.

Please don't – you will still end up having two methods doing the same thing.

I prefer the dedicated getSupportedExtensions() API for a few reasons:

  • 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.

  • 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 CFs to dynamically determine their active status.

  • 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.

  • Conceptually, ConfigurationFactory should look like a singleton service; how it multiplexes concrete factories is an implementation detail. Exposing getFactories() would couple users to that detail, whereas getSupportedExtensions() gives them exactly what they need without exposing internals.

CF::getSupportedFileExtensions only hoists CF::getSupportedTypes for a handful of users. OTOH, CF::instances would enable several more use cases with less API impact.

@vy
Copy link
Member

vy commented Aug 21, 2025

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.

On Tuesday, in a meeting where @ppkarwasz and I were present, after hearing that one of the biggest Log4j users using programmatic configuration extensively, I am very reluctant to marry the concept of files with configurations.

@ppkarwasz
Copy link
Contributor

  • 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 CFs to dynamically determine their active status.

The inconsistency is in the way a factory communicates that it is inactive:

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, 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, 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 URLs 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:

    !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:

    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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: To triage
Development

Successfully merging this pull request may close these issues.

Add API to List Supported Configuration File Locations
3 participants