Skip to content

Conversation

yybmion
Copy link

@yybmion yybmion commented Sep 8, 2025

Summary

Adds a new public API to ConfigurationFactory for creating configurations
from multiple URIs, automatically combining them into a CompositeConfiguration when needed.

Motivation

External frameworks like Spring Boot currently need to understand Log4j's internal implementation to create composite configurations. This new API encapsulates that complexity.

Addresses feedback from #3839

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

@yybmion yybmion force-pushed the add_getConfiguration_for_multiple_URIs branch from 9377356 to 22ba22c Compare September 26, 2025 09:28
@yybmion
Copy link
Author

yybmion commented Sep 26, 2025

Thanks for the review, @vy. I've updated the PR.

If this looks good, I'll proceed with adding tests and changelog.

@vy
Copy link
Member

vy commented Sep 26, 2025

If this looks good, I'll proceed with adding tests and changelog.

@yybmion, yes, please proceed with tests and a changelog.

@yybmion
Copy link
Author

yybmion commented Sep 26, 2025

@yybmion, yes, please proceed with tests and a changelog.

@vy , Added tests and changelog as requested.

@yybmion yybmion force-pushed the add_getConfiguration_for_multiple_URIs branch from aeda617 to 40b26ea Compare October 3, 2025 14:12
@yybmion
Copy link
Author

yybmion commented Oct 3, 2025

Thanks for the review @vy, Changes applied as requested.

Added a null element check in the URI list to throw a ConfigurationException when any element is null. This ensures that testGetConfigurationWithNullInList passes.

for (final URI uri : uris) {

     if (uri == null) {
          throw new ConfigurationException("URI list contains null element");
   }
...

Copy link
Member

@vy vy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yybmion, thanks so much for your patience. I am happy with the last state of the PR.

@ppkarwasz, do you any remarks?

Copy link

github-actions bot commented Oct 3, 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 yybmion force-pushed the add_getConfiguration_for_multiple_URIs branch from 6af187e to e91c25b Compare October 3, 2025 17:41
@yybmion
Copy link
Author

yybmion commented Oct 3, 2025

@vy, I'm sorry about the confusion. Previously, I bumped the versions of those packages in the last PR (#3839) after making some changes, but I forgot to revert them. I've now restored them to their original values.

@vy
Copy link
Member

vy commented Oct 10, 2025

@yybmion, CI builds are failing. Would you mind reading the Log4j Development Guide and resolving all build failures, please?

@yybmion yybmion force-pushed the add_getConfiguration_for_multiple_URIs branch from e91c25b to 93819f2 Compare October 10, 2025 08:41
@yybmion
Copy link
Author

yybmion commented Oct 10, 2025

@yybmion, just a sec, why do we need these version bumps? We did not touch to these packages.

@vy I think the version bumps are actually needed because the new public method in ConfigurationFactory is inherited by all its subclasses (JsonConfigurationFactory, XmlConfigurationFactory, etc.), which adds the method to those packages' public API.

This is causing the build failure. I've updated the versions back to 2.26.0.

Copy link
Contributor

@ppkarwasz ppkarwasz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @yybmion,

Thanks for the PR, and apologies for the delayed feedback: I haven’t had as much time to spend on Log4j recently as I’d like.

I like the direction of your change, but there are still a few edge cases we need to address.

In particular, I expected the PR to integrate the newly introduced method into the existing code.

For example, ConfigurationFactory.Factory#getConfiguration(LoggerContext, String, URI) could benefit from using it: we currently have a couple of legacy mechanisms for passing multiple URIs through a single URI parameter:

  • If uri == null, the log4j2.configurationFile property is treated as a comma-separated list of file paths or URIs.
  • If uri != null, there’s a legacy (and admittedly hacky) method of cramming two URIs into one string (see parseConfigLocations), which unfortunately still needs to be supported.

These mechanisms are handled in getConfiguration(LoggerContext, String, URI), but I think we should delegate them to the newly introduced method.

* @return a Configuration created from the provided URIs
* @throws NullPointerException if uris is null
* @throws IllegalArgumentException if uris is empty
* @throws ConfigurationException if no valid configuration could be created
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change introduces an inconsistency between two similarly named methods:

  • getConfiguration(LoggerContext, String, URI): returns null on error
  • getConfiguration(LoggerContext, String, List<URI>): throws ConfigurationException on error

Personally, I prefer the exception-throwing approach since it forces the caller to handle the error explicitly.
That said, we could instead align with the existing “StatusLogger + return null” pattern here.

If we keep the latter approach, then LoggerContext.start(Configuration) and LoggerContext.reconfigure(Configuration) should defensively null-check the Configuration argument to ensure that the call ignores null as argument.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We have discussed this earlier, and agreed that, since this is a new API, we will go with the exception-throwing approach. @ppkarwasz you can see your thumbs up in the discussion: #3839 (comment)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're right that we discussed on dev@logging that we should not use partially constructed configurations.

However, my concern here is slightly different: we now have two overloads with fundamentally different error-handling conventions. Java deprecated the File API partly because it behaved inconsistently across methods and this proposal would introduce a similar inconsistency between these overloads.

I see three possible approaches:

  1. Align all methods to throw on error.
    This approach would make error handling explicit and predictable. Currently, getConfiguration can already throw when an invalid properties configuration is encountered. However, most other factories behave differently: they either return null when the file is missing or the factory is inactive, or return a partially constructed (broken) configuration when a parsing error occurs.
    Unifying everything under the exception-throwing model would be cleaner, but we should proceed carefully, as it could introduce backward-compatibility issues for existing users.

  2. Align all methods to return null on error.
    This is more consistent with current behavior, though not ideal: some callers expect a non-null configuration even when URI is null.

  3. Introduce a new method, e.g. getRequiredConfiguration, which explicitly throws on failure.
    This would let us offer both behaviors without breaking existing expectations.

P.S.: A “thumbs up” on a PR announcement doesn’t necessarily mean approval: it just means I’ve seen it and plan to review when I can.

Comment on lines +355 to +357
if (uris.isEmpty()) {
throw new IllegalArgumentException("URI list cannot be empty");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If the list is empty, I’d delegate to getConfiguration(loggerContext, name, (URI) null). This allows the factory to either auto-detect a configuration or return null.

Also note: the name parameter only matters in the empty-list case. When a non-null URI is provided, getConfiguration(LoggerContext, String, URI) ignores name entirely.
So, if we decide to forbid empty lists, the name parameter becomes unnecessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with delegating to getConfiguration(loggerContext, name, (URI) null), but don't return null please.

Comment on lines +363 to +365
if (uri == null) {
throw new ConfigurationException("URI list contains null element");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: IllegalArgumentException seems more appropriate here. ConfigurationException suggests an unexpected runtime failure, whereas a null-check is a precondition the caller should do himself.

Comment on lines +373 to +375
if (!(config instanceof AbstractConfiguration)) {
throw new ConfigurationException("Configuration at " + uri + " is not an AbstractConfiguration");
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This check is only necessary when uris has more than one element and we need to build a CompositeConfiguration.

If uris is a singleton, we can skip the check and just return the configuration directly.

Comment on lines +168 to +169
void testGetConfigurationWithSingleUri() throws Exception {
final ConfigurationFactory factory = ConfigurationFactory.getInstance();
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since the goal is to test the basic implementation, we could use a mock that delegates to the real getConfiguration(LoggerContext, String, List<URI>) method, while mocking only getConfiguration(LoggerContext, String, URI).

@ppkarwasz
Copy link
Contributor

I realize my comments go a bit beyond the immediate scope of this PR, but the section of code below has become a real maintenance headache. It would be great if we could take the opportunity to clean it up once and for all:

public Configuration getConfiguration(
final LoggerContext loggerContext, final String name, final URI configLocation) {
if (configLocation == null) {
final String configLocationStr = this.substitutor.replace(
PropertiesUtil.getProperties().getStringProperty(CONFIGURATION_FILE_PROPERTY));
if (configLocationStr != null) {
final String[] sources = parseConfigLocations(configLocationStr);
if (sources.length > 1) {
final List<AbstractConfiguration> configs = new ArrayList<>();
for (final String sourceLocation : sources) {
final Configuration config = getConfiguration(loggerContext, sourceLocation.trim());
if (config != null) {
if (config instanceof AbstractConfiguration) {
configs.add((AbstractConfiguration) config);
} else {
LOGGER.error("Failed to created configuration at {}", sourceLocation);
return null;
}
} else {
LOGGER.warn("Unable to create configuration for {}, ignoring", sourceLocation);
}
}
if (configs.size() > 1) {
return new CompositeConfiguration(configs);
} else if (configs.size() == 1) {
return configs.get(0);
}
}
return getConfiguration(loggerContext, configLocationStr);
}
final String log4j1ConfigStr = this.substitutor.replace(
PropertiesUtil.getProperties().getStringProperty(LOG4J1_CONFIGURATION_FILE_PROPERTY));
if (log4j1ConfigStr != null) {
System.setProperty(LOG4J1_EXPERIMENTAL, "true");
return getConfiguration(LOG4J1_VERSION, loggerContext, log4j1ConfigStr);
}
for (final ConfigurationFactory factory : getFactories()) {
final String[] types = factory.getSupportedTypes();
if (types != null) {
for (final String type : types) {
if (type.equals(ALL_TYPES)) {
final Configuration config =
factory.getConfiguration(loggerContext, name, configLocation);
if (config != null) {
return config;
}
}
}
}
}
} else {
final String[] sources = parseConfigLocations(configLocation);
if (sources.length > 1) {
final List<AbstractConfiguration> configs = new ArrayList<>();
for (final String sourceLocation : sources) {
final Configuration config = getConfiguration(loggerContext, sourceLocation.trim());
if (config instanceof AbstractConfiguration) {
configs.add((AbstractConfiguration) config);
} else {
LOGGER.error("Failed to created configuration at {}", sourceLocation);
return null;
}
}
return new CompositeConfiguration(configs);
}
// configLocation != null
final String configLocationStr = configLocation.toString();
for (final ConfigurationFactory factory : getFactories()) {
final String[] types = factory.getSupportedTypes();
if (types != null) {
for (final String type : types) {
if (type.equals(ALL_TYPES) || configLocationStr.endsWith(type)) {
final Configuration config =
factory.getConfiguration(loggerContext, name, configLocation);
if (config != null) {
return config;
}
}
}
}
}
}
Configuration config = getConfiguration(loggerContext, true, name);
if (config == null) {
config = getConfiguration(loggerContext, true, null);
if (config == null) {
config = getConfiguration(loggerContext, false, name);
if (config == null) {
config = getConfiguration(loggerContext, false, null);
}
}
}
if (config != null) {
return config;
}
LOGGER.warn(
"No Log4j 2 configuration file found. "
+ "Using default configuration (logging only errors to the console), "
+ "or user programmatically provided configurations. "
+ "Set system property 'log4j2.debug' "
+ "to show Log4j 2 internal initialization logging. "
+ "See https://logging.apache.org/log4j/2.x/manual/configuration.html for instructions on how to configure Log4j 2");
return new DefaultConfiguration();
}

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.

3 participants