You are viewing a plain text version of this content. The canonical link for it is here.
Posted to notifications@logging.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2023/01/30 12:38:01 UTC
[GitHub] [logging-log4j2] ppkarwasz opened a new issue, #1231: Improve validation of `PathCondition`s
ppkarwasz opened a new issue, #1231:
URL: https://github.com/apache/logging-log4j2/issues/1231
## Description
As reported in [this StackOverflow question](https://stackoverflow.com/q/75091192/11748454) 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.
--
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: notifications-unsubscribe@logging.apache.org.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Improve validation of `PathCondition`s (logging-log4j2)
Posted by "lukaszspyra (via GitHub)" <gi...@apache.org>.
lukaszspyra commented on issue #1231:
URL: https://github.com/apache/logging-log4j2/issues/1231#issuecomment-1627481253
Hi @ppkarwasz ,
Just finished working on the issue, can you have a look at [PR](https://github.com/apache/logging-log4j2/pull/1546) please?
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Improve validation of `PathCondition`s (logging-log4j2)
Posted by "lukaszspyra (via GitHub)" <gi...@apache.org>.
lukaszspyra commented on issue #1231:
URL: https://github.com/apache/logging-log4j2/issues/1231#issuecomment-1581059851
Hi,
I came across this issue and looked through the comments on the [closed PR ]( #1276 ). 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](https://logging.apache.org/log4j/2.x/log4j-core/apidocs/org/apache/logging/log4j/core/config/plugins/util/PluginBuilder.html) 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?
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Improve validation of `PathCondition`s (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz closed issue #1231: Improve validation of `PathCondition`s
URL: https://github.com/apache/logging-log4j2/issues/1231
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Improve validation of `PathCondition`s (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on issue #1231:
URL: https://github.com/apache/logging-log4j2/issues/1231#issuecomment-1645047463
Fixed by #1546
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Improve validation of `PathCondition`s (logging-log4j2)
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on issue #1231:
URL: https://github.com/apache/logging-log4j2/issues/1231#issuecomment-1581303568
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.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [I] Improve validation of `PathCondition`s (logging-log4j2)
Posted by "lukaszspyra (via GitHub)" <gi...@apache.org>.
lukaszspyra commented on issue #1231:
URL: https://github.com/apache/logging-log4j2/issues/1231#issuecomment-1594241586
Thanks @ppkarwasz it helped me a lot. Will go through remaining components in `org.apache.logging.log4j.core.appender.rolling` and create PR when compeleted.
--
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: notifications-unsubscribe@logging.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org