ppkarwasz commented on PR #3839: URL: https://github.com/apache/logging-log4j2/pull/3839#issuecomment-3233181160
> @ppkarwasz, since there is no established default that one can claim as a stable API, I'm inclined to throw `IAE`, or `NPE` (when arg is null) on failures. WDYT? I think the bigger issue is **the nullability and return behavior**, which is inconsistent across the existing `getConfiguration(...)` methods: ```java public Configuration getConfiguration(LoggerContext ctx, ConfigurationSource source); public Configuration getConfiguration(LoggerContext ctx, String name, URI location); public Configuration getConfiguration(LoggerContext ctx, String name, URI location, ClassLoader loader); ``` * All three can return `null` in some cases. * But the second (and therefore the third) will return a `DefaultConfiguration` in the most common failure case. That mix of `null` vs `DefaultConfiguration` is problematic: users will almost always guard for one but not the other. For example, Spring Boot’s `Log4j2LoggingSystem` never checks for `null`. ## My proposal ### Return behavior * Clearly document that these methods **can and do** return `null`. * Treat the “fallback to `DefaultConfiguration`” behavior as **deprecated**, with the intent to remove it in a future major release. This gives consumers a single, consistent contract to rely on. ### Parameter nullability For arguments, I’d keep the current semantics and only be strict where it adds value: * `LoggerContext`: optional. It’s mostly passed through to rare plugins (interpolator lookups, `LoggerConfig` default `includeLocation`), so it can remain nullable. * `String name`: optional. Used as a profile hint (`log4j2-spring.xml` vs `log4j2.xml`), so nullable is fine. * `ConfigurationSource`: should not be optional, but if you pass `null` today, you get `null`—so no need to change that. * `URI`: optional. It’s just a hint about where the config came from. * `List<URI>` (new overload): here I’d draw the line. Passing `null` is meaningless when an empty list is a valid “no configs” signal. I’d propose we explicitly throw `NPE` if the list itself is `null`. This way we converge towards: * **Return contract:** always `null` on failure (with legacy `DefaultConfiguration` as deprecated). * **Parameter contract:** permissive for existing args, strict (`NPE`) only for the new `List<URI>`. -- 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]
