Improve validation of `PathCondition`s

[ppkarwasz]

Description

As reported in this StackOverflow question if the configuration does not contain all required properties, at least the IfLastModified fails with a NullPointerException instead of a proper error message.

We should probably check all components in org.apache.logging.log4j.core.appender.rolling for similar issues.

[lukaszspyra]

Hi,
I came across this issue and looked through the comments on the closed PR . I would like to contribute, however I have question regarding the previous conversation:

There are two ways to instantiate a Log4j2 component like IfLastModified:

• you can call IfLastModified#createAgeCondition in your code. We don't recommend such a programmatic approach, so if you pass null as age parameter, IMHO you deserve the NPE with a standard message,
• you can declare it in your configuration. In such a case the component is instantiated by PluginBuilder that validates all constraints and logs the validation errors appropriately.

From what I noticed, at least in case of IfLastModified#createAgeCondition there is no builder defined, so in both ways to instantiate it (pragmatic and PluginBuilder) it would fall to the same factory method. Adding @Required on the param will result in logging error in any way (followed by NPE from Objects.requiredNonNull() of private c-tor), there would be no distinction.

Is it acceptable solution?

[ppkarwasz]

Hi @lukaszspyra,

Yes, adding @Required in the right places is what I had in mind. This will cause the PluginBuilder to log an error and return null.

Logging an error and returning null is the way Log4j 2.x deals with configuration errors: we don't want to disrupt the application's logic. That's why I am not a big fan of Objects.requireNonNull assertions: PluginBuilder will not trigger them and programmatic configuration should check the preconditions before calling a factory method or builder.

[lukaszspyra]

Thanks @ppkarwasz it helped me a lot. Will go through remaining components in org.apache.logging.log4j.core.appender.rolling and create PR when completed.

[lukaszspyra]

Hi @ppkarwasz ,
Just finished working on the issue, can you have a look at PR please?

[ppkarwasz]

Fixed by #1546