ppkarwasz commented on code in PR #3921:
URL: https://github.com/apache/logging-log4j2/pull/3921#discussion_r2422619072
##########
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:
You're right that we discussed on
[`dev@logging`](https://lists.apache.org/thread/h2oydyk6xld47ljttqvflbt4530o73vw)
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.
--
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]
