-
Notifications
You must be signed in to change notification settings - Fork 1.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix handling of LoggerContextAware
lookups
#2313
Conversation
@ppkarwasz, thanks for considering me for the review. Unfortunately I am not much familiar with the parts you touch, hence I doubt if my review will be meaningful. I trust your judgement and suggest to proceed as you wish. |
public void setConfiguration(final Configuration configuration) { | ||
super.setConfiguration(configuration); | ||
// Propagate | ||
for (final StrLookup lookup : strLookupMap.values()) { | ||
if (lookup instanceof ConfigurationAware) { | ||
((ConfigurationAware) lookup).setConfiguration(configuration); | ||
} | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vy,
This is the part I wanted to consult you on. IMHO it is bad practice when methods called set*
do anything else than set a single value.
I think in this case the usage is acceptable, compared to:
logging-log4j2/log4j-core/src/main/java/org/apache/logging/log4j/core/lookup/StrSubstitutor.java
Lines 1490 to 1495 in f05864d
public void setVariableResolver(final StrLookup variableResolver) { | |
if (variableResolver instanceof ConfigurationAware && this.configuration != null) { | |
((ConfigurationAware) variableResolver).setConfiguration(this.configuration); | |
} | |
this.variableResolver = variableResolver; | |
} |
where the side effect of the call seems surprising.
Nevertheless I am not going to modify the second example, maybe sometimes it is necessary.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I find the *Aware
design pattern a footgun. As is visibly the case here, it is not clear where one should check the instance type and on which values. The more such a contract is used, the more the probability that an instanceof
check is missing.
I sadly need to agree that your usage is acceptable given current state of the things.
Due to the changes in #2278 `LoggerContextAware` lookups stopped working in `2.23.0`. This PR: * fixes the NPE in `Interpolator` that occurs if `Interpolator#setLoggerContext` was **not** called after instantiation. * Calls `Interpolator#setConfiguration` and `Interpolator#setLoggerContext` wherever it is possible. * Changes the way `Interpolator` propagates `Configuration` and `LoggerContext` to child lookups. Previously this occurred at each evaluation, now it occurs only in the setters. Closes #2309.
9cdb359
to
395a8f4
Compare
Due to the changes in #2278
LoggerContextAware
lookups stopped working in2.23.0
.This PR:
Interpolator
that occurs ifInterpolator#setLoggerContext
was not called after instantiation.Interpolator#setConfiguration
andInterpolator#setLoggerContext
wherever it is possible.Interpolator
propagatesConfiguration
andLoggerContext
to child lookups. Previously this occurred at each evaluation, now it occurs only in the setters.Closes #2309.