-
-
Notifications
You must be signed in to change notification settings - Fork 1.7k
Add getConfiguration method for multiple URIs #3921
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
base: 2.x
Are you sure you want to change the base?
Add getConfiguration method for multiple URIs #3921
Conversation
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java
Outdated
Show resolved
Hide resolved
9377356
to
22ba22c
Compare
Thanks for the review, @vy. I've updated the PR. If this looks good, I'll proceed with adding tests and changelog. |
@yybmion, yes, please proceed with tests and a changelog. |
...j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationFactoryTest.java
Show resolved
Hide resolved
...j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationFactoryTest.java
Outdated
Show resolved
Hide resolved
...j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationFactoryTest.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/json/package-info.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/properties/package-info.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/xml/package-info.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/yaml/package-info.java
Outdated
Show resolved
Hide resolved
src/changelog/.2.x.x/add_getConfiguration_method_for_mulitiple_URIs.xml
Outdated
Show resolved
Hide resolved
aeda617
to
40b26ea
Compare
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 for (final URI uri : uris) {
if (uri == null) {
throw new ConfigurationException("URI list contains null element");
}
... |
There was a problem hiding this 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?
log4j-core/src/main/java/org/apache/logging/log4j/core/config/json/package-info.java
Show resolved
Hide resolved
6af187e
to
e91c25b
Compare
@yybmion, CI builds are failing. Would you mind reading the Log4j Development Guide and resolving all build failures, please? |
e91c25b
to
93819f2
Compare
@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. |
There was a problem hiding this 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
, thelog4j2.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 (seeparseConfigLocations
), 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.
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java
Outdated
Show resolved
Hide resolved
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java
Outdated
Show resolved
Hide resolved
...j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationFactoryTest.java
Outdated
Show resolved
Hide resolved
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: Lines 373 to 478 in 8a3fb53
|
|
||
/** | ||
* {@return a {@link Configuration} created using provided configuration location {@link URI}s} | ||
* If the provided list of {@code URI}s is null or empty, {@code getConfiguration(loggerContext, name, (URI) null)} will be returned. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we explain more explicitly the behavior of an empty list?
* If the provided list of {@code URI}s is null or empty, {@code getConfiguration(loggerContext, name, (URI) null)} will be returned. | |
* <p>If the provided list of {@code URI}s is empty, the configuration factory | |
* attempts to load an implementation-dependent set of default locations. | |
* If no configuration can be found, a {@link ConfigurationException} is thrown.</p> |
* | ||
* @param loggerContext a logger context, may be null | ||
* @param name a configuration name, may be null | ||
* @param configLocations configuration location {@code URI}s, may be null or empty |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit: I’d prefer not to allow null
for a new API and instead require callers to pass an empty list.
Hi @vy and @ppkarwasz, Thanks for the feedback! Before I finalize this PR, I'd like to clarify the scope of follow-up work. Based on the discussion, I see two distinct improvement areas: 1. Factory refactoring (removing duplicate code)
Delegate composite configuration logic in 2. Exception-throwing consistency
Align all I'm currently working on two changes. Should I handle them in separate PRs? |
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
./mvnw spotless:check
./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)
📝 Changelog (select one)
src/changelog/.2.x.x
. (See Changelog Entry File Guide).