ppkarwasz commented on code in PR #3921:
URL: https://github.com/apache/logging-log4j2/pull/3921#discussion_r2419018780
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java:
##########
@@ -333,6 +334,52 @@ public Configuration getConfiguration(
return getConfiguration(loggerContext, name, configLocation);
}
+ /**
+ * Creates a Configuration from multiple configuration URIs.
+ * If multiple URIs are successfully loaded, they will be combined into a
CompositeConfiguration.
+ *
+ * @param loggerContext the logger context (may be null)
+ * @param name the configuration name (may be null)
+ * @param uris the list of configuration URIs (must not be null or empty)
+ * @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
+ * from any of the provided URIs
+ * @since 2.26.0
+ */
+ public Configuration getConfiguration(final LoggerContext loggerContext,
final String name, final List<URI> uris) {
+
+ Objects.requireNonNull(uris, "uris parameter cannot be null");
+
+ if (uris.isEmpty()) {
+ throw new IllegalArgumentException("URI list cannot be empty");
+ }
+
+ final List<AbstractConfiguration> configurations = new ArrayList<>();
+
+ for (final URI uri : uris) {
+
+ if (uri == null) {
+ throw new ConfigurationException("URI list contains null
element");
+ }
Review Comment:
*Nit*: `IllegalArgumentException` seems more appropriate here.
`ConfigurationException` suggests an unexpected runtime failure, whereas a
null-check is a precondition the caller should do himself.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java:
##########
@@ -333,6 +334,52 @@ public Configuration getConfiguration(
return getConfiguration(loggerContext, name, configLocation);
}
+ /**
+ * Creates a Configuration from multiple configuration URIs.
+ * If multiple URIs are successfully loaded, they will be combined into a
CompositeConfiguration.
+ *
+ * @param loggerContext the logger context (may be null)
+ * @param name the configuration name (may be null)
+ * @param uris the list of configuration URIs (must not be null or empty)
+ * @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
Review Comment:
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.
##########
log4j-core-test/src/test/java/org/apache/logging/log4j/core/config/ConfigurationFactoryTest.java:
##########
@@ -130,4 +136,63 @@ void properties(final LoggerContext context) throws
IOException {
final Path logFile = loggingPath.resolve("test-properties.log");
checkFileLogger(context, logFile);
}
+
+ @Test
+ void testGetConfigurationWithNullUris() {
+ final ConfigurationFactory factory =
ConfigurationFactory.getInstance();
+ try (final LoggerContext context = new LoggerContext("test")) {
+ assertThrows(NullPointerException.class, () ->
factory.getConfiguration(context, "test", (List<URI>) null));
+ }
+ }
+
+ @Test
+ void testGetConfigurationWithEmptyUris() {
+ final ConfigurationFactory factory =
ConfigurationFactory.getInstance();
+ try (final LoggerContext context = new LoggerContext("test")) {
+ assertThrows(
+ IllegalArgumentException.class,
+ () -> factory.getConfiguration(context, "test",
Collections.emptyList()));
+ }
+ }
+
+ @Test
+ void testGetConfigurationWithNullInList() {
+ final ConfigurationFactory factory =
ConfigurationFactory.getInstance();
+ try (final LoggerContext context = new LoggerContext("test")) {
+ final List<URI> listWithNull = Collections.singletonList(null);
+ assertThrows(ConfigurationException.class, () ->
factory.getConfiguration(context, "test", listWithNull));
+ }
+ }
+
+ @Test
+ void testGetConfigurationWithSingleUri() throws Exception {
+ final ConfigurationFactory factory =
ConfigurationFactory.getInstance();
Review Comment:
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)`.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java:
##########
@@ -333,6 +334,52 @@ public Configuration getConfiguration(
return getConfiguration(loggerContext, name, configLocation);
}
+ /**
+ * Creates a Configuration from multiple configuration URIs.
+ * If multiple URIs are successfully loaded, they will be combined into a
CompositeConfiguration.
+ *
+ * @param loggerContext the logger context (may be null)
+ * @param name the configuration name (may be null)
+ * @param uris the list of configuration URIs (must not be null or empty)
+ * @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
+ * from any of the provided URIs
+ * @since 2.26.0
+ */
+ public Configuration getConfiguration(final LoggerContext loggerContext,
final String name, final List<URI> uris) {
+
+ Objects.requireNonNull(uris, "uris parameter cannot be null");
+
+ if (uris.isEmpty()) {
+ throw new IllegalArgumentException("URI list cannot be empty");
+ }
Review Comment:
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.
##########
log4j-core/src/main/java/org/apache/logging/log4j/core/config/ConfigurationFactory.java:
##########
@@ -333,6 +334,52 @@ public Configuration getConfiguration(
return getConfiguration(loggerContext, name, configLocation);
}
+ /**
+ * Creates a Configuration from multiple configuration URIs.
+ * If multiple URIs are successfully loaded, they will be combined into a
CompositeConfiguration.
+ *
+ * @param loggerContext the logger context (may be null)
+ * @param name the configuration name (may be null)
+ * @param uris the list of configuration URIs (must not be null or empty)
+ * @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
+ * from any of the provided URIs
+ * @since 2.26.0
+ */
+ public Configuration getConfiguration(final LoggerContext loggerContext,
final String name, final List<URI> uris) {
+
+ Objects.requireNonNull(uris, "uris parameter cannot be null");
+
+ if (uris.isEmpty()) {
+ throw new IllegalArgumentException("URI list cannot be empty");
+ }
+
+ final List<AbstractConfiguration> configurations = new ArrayList<>();
+
+ for (final URI uri : uris) {
+
+ if (uri == null) {
+ throw new ConfigurationException("URI list contains null
element");
+ }
+
+ final Configuration config = getConfiguration(loggerContext, name,
uri);
+
+ if (config == null) {
+ throw new ConfigurationException("Failed to create
configuration from: " + uri);
+ }
+
+ if (!(config instanceof AbstractConfiguration)) {
+ throw new ConfigurationException("Configuration at " + uri + "
is not an AbstractConfiguration");
+ }
Review Comment:
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.
--
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]
