MaxFreedomPollard opened a new pull request, #4304:
URL: https://github.com/apache/logging-log4j2/pull/4304
## What breaks
`SmtpAppender.createAppender` never returns an appender. It throws
`NullPointerException: name` out of the `AbstractAppender` constructor, and
none of the mail settings passed to it reach the `MailManager`. The deprecated
factory is still public API, so anything that calls it programmatically fails
at configuration time.
## Cause
Commit 30e9563f (`[LOG4J2-3362] Adds a SmtpManager compatible with Jakarta
EE 9`, first released in 2.18.0) rewrote the body of `createAppender` to
delegate to `SmtpAppender.newBuilder()`. The delegation forwards seven of the
eighteen parameters and drops the rest, at
`log4j-core/src/main/java/org/apache/logging/log4j/core/appender/SmtpAppender.java:358`:
```java
return SmtpAppender.newBuilder()
.setIgnoreExceptions(Booleans.parseBoolean(ignore, true))
.setSmtpPort(AbstractAppender.parseInt(smtpPortStr, 0))
.setSmtpDebug(Boolean.parseBoolean(smtpDebug))
.setBufferSize(bufferSizeStr == null ? DEFAULT_BUFFER_SIZE :
Integers.parseInt(bufferSizeStr))
.setLayout(layout)
.setFilter(filter)
.setConfiguration(config != null ? config : new
DefaultConfiguration())
.build();
```
`name`, `to`, `cc`, `bcc`, `from`, `replyTo`, `subject`, `smtpProtocol`,
`smtpHost`, `smtpUsername` and `smtpPassword` are never passed on.
`Builder.build()` therefore reaches `new SmtpAppender(getName(), ...)` with a
null name, and `AbstractAppender` does `this.name =
Objects.requireNonNull(name, "name")` at
`log4j-core/src/main/java/org/apache/logging/log4j/core/appender/AbstractAppender.java:231`.
The `if (name == null)` guard at the top of `createAppender` passes, because
the caller did supply a name; it just goes nowhere.
The body before that commit built the `SmtpManager` directly from all of
those arguments, so this is a regression in the refactor and not a limitation
of the deprecated entry point. The plugin system is unaffected:
`createAppender` carries no `@PluginFactory`, so XML and properties
configurations go through `newBuilder()`.
## Fix
Forward the eleven missing attributes to the builder. Passing `smtpProtocol`
straight through is safe because `Builder.build()` already resets a null or
empty protocol to `smtp`, which is what the builder field defaults to.
## Testing
`SmtpAppenderTest#testCreateAppenderForwardsMailAttributes` calls the
deprecated factory with every attribute set and asserts the appender name, then
compares the resulting `MailManager` name against the manager name of an
equivalent `newBuilder()` chain. `MailManager.createManagerName` encodes to,
cc, bcc, from, replyTo, subject, protocol, host, port, username and debug into
that name, so equal names mean every attribute arrived.
```
export JAVA_HOME=<Temurin 17>
./mvnw -pl log4j-api-java9,log4j-core-java9,log4j-core-test -am install
-DskipTests
./mvnw -pl log4j-core-test test -Dtest=SmtpAppenderTest
-Dsurefire.failIfNoSpecifiedTests=false
```
Without the change, on `2.x` at ded9666: `Tests run: 6, Failures: 0, Errors:
1` with `SmtpAppenderTest.testCreateAppenderForwardsMailAttributes ...
NullPointerException: name`. With the change: `Tests run: 6, Failures: 0,
Errors: 0, Skipped: 0`.
`./mvnw -pl log4j-core-test -am verify` passes, and `spotless:apply` on
`log4j-core` and `log4j-core-test` leaves both files unchanged.
## Checklist
* Base your changes on `2.x` branch if you are targeting Log4j 2; use `main`
otherwise
* `./mvnw verify` succeeds ([the build
instructions](https://logging.apache.org/log4j/2.x/development.html#building))
* Non-trivial changes contain an entry file in the `src/changelog/.2.x.x`
directory
* Tests are provided
--
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]
