logging-log4j2

logging-log4j2 copied to clipboard

Proposed fix for LOG4J2-2332

Hey, first of, thanks for the contribution! Personally, I'm excited to see someone noticed the validation API in the first place. :D

Anyways, I certainly like this idea. In fact, this actually solves a long standing annoyance I've had with the same issue in the past.

Thank you for your contribution.

I am not a fan of the name "NoUnresolvedVariables" because of the two negatives "No" and the "Un" in "Unresolved". Can we come up with a simpler name? Maybe "ValidateVariables"?

I also wonder if the annotation should have an optional log level so that not ALL validation failures end up as an ERROR event.

Maybe @RequiresVariables or something like that?

And all the validation annotations so far have all been used to err a plugin factory which seems like a config error to me. As a user, I'd like to see errors related to where I messed up most of all.

@RequiresVariables is not quite right either IMO, @RequiresResolvedVariables might be better.

Why not just an attribute of the existing plugin? Also, I can't tell from the patch but will this cause Log4j configuration to fail or just the component? I am assuming we would want this to apply to a lot more parameters on a lot of plugins, not just the ones changed here.

The way validation constraints work is that it fails the plugin only (will return null along with logging the error). It was originally developed to automate all the null/isEmpty() checks everywhere via the @Required annotation and we've added more since then.

Anyways, it could be implemented an attribute of @Required in that case. For backwards compatibility reasons, I wouldn't make that the default state to avoid breaking existing configs.

@jvz I debugged my way through and stumbled over it while doing so. I actually wondered why some @Required tags are not there for some of the file loggers "file" attribute, while they are there in other cases.

Personally I do not really care for whatever name you guys decide on, it is just the functionality I need. My point of view is just that it sucks to run a java program with priviledges that allow you to create directories and end up creating directories all over the place where you do not actually want them with really weird names.

Regarding backwards compatibility I do not really know if this is a reason to take into account since this is as you said yourself actually a configuration issue that goes around unnoticed for now and should actually be reported/validated before calling mkdirs on it. It might just bring up configuration files that were actually broken all time long and should have been fixed anyway. But those are just my few cents on this topic.

If you decide to make this non-default behaviour: Is there a way I can make it default behavior without using a configuration file? Because I supply my customers with a framework that uses log4j2 as it's logging utility and they can supply configuration files I do not have any control over and I want to prevent them messing up their directories unintentionally.

I would have been happy enough if I could have intercepted the mkdirs call somehow and cancel it, since I am validating the appenders target directory after the context initialization, but it is too late at this point already.

Btw. could you guys help me with the travis issues? There are a lot of exceptions I am seeing in there and I do not know which are intentional and which I actually caused and which are causing travis to fail the build. If you give me a few hints I will gladly look into them.

I have also enabled edits by maintainers, so feel free to push fixes/changes to my fork/this pull request.

Travis is broken due to some integration tests that don't seem to work well in CI builds currently, don't worry too much about it affecting your PR. I manually verify a PR builds before merging to master as it is.

As for making it default behavior, I personally would prefer to adopt this new behavior as well, though I'm not sure how it would affect existing configurations. If it's not adopted as default behavior, perhaps a system property addition to globally override that setting?

Edit: there's some developer docs about validators here.

This shouldn't need to be a system property. it can be an attribute of the configuration element.

We should consider if the presence of unresolved variables should:

• Log a DEBUG, WARN, ERROR, or FATAL message.
• Fail fast or log all unresolved variables.
• Fail the the whole config.

@garydgregory Agreed. That was what I was questioning in my first comment.

I think it should at least be error level, since during context initialization by default only StatusLogger is used and only level error or higher is reported to stderr(correct me if I am wrong please).

As for the rest I will leave it uncommented since you guys probably know better how you handled similar problems in the past.

@Lemongrass3110 @jvz @garydgregory After all this time I am not sure where this stands. It now has conflicts that need to be resolved. The PR is over 2 years old and we either need to make the changes needed to merge it or close it.

@Lemongrass3110 @jvz @garydgregory After all this time I am not sure where this stands. It now has conflicts that need to be resolved. The PR is over 2 years old and we either need to make the changes needed to merge it or close it.

@rgoers the Log4J team has to decide on how they want to resolve this, as you can read here:

We should consider if the presence of unresolved variables should:

* Log a DEBUG, WARN, ERROR, or FATAL message.

* Fail fast or log all unresolved variables.

* Fail the the whole config.

I cannot force any decisions onto you.

@rgoers I tried to resolve the conflicts as good as possible. Do you want me to move the new classes from org.apache.logging.log4j.core.config.plugins.validation.* to org.apache.logging.log4j.plugins.validation.* as well?

Can you maybe check the Travis CI status? I saw that master was also failing right now, so I am not really sure, if the problem is because of my changes or simply related to master.

Leaving this open as we may want to look at it again after I get the master build straightened out.

This was closed automatically by Github because we renamed the release-2.x branch to 2.x. Feel free to resubmit to the 2.x branch.

@ppkarwasz sorry but after almost 5 years and no decision from your team I will not waste any time on this anymore.