-
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
Removed @PluginAttribute 'patternFlags' argument from @PluginFactory for RegexFilter. (#3086) #3512
base: 2.x
Are you sure you want to change the base?
Conversation
+ made AbstractFiltter.AbstractFilterBuilder onMatch/onMismatch fields protected + added AbstractFilter(AbstractFilterBuilder) constructor + added RegexFilter.Builder implementation + added RegexFilter(Builder) constructor + moved RegexFilter Pattern compile into constructor + added fields to persist configuration propertties + getters (regexExpression, patternFlags) + changed private constructor to accept builder as argument + renamed private method 'targetMessageTest' to more approprriate 'getMessageTextByType' + added Javadoc + grouped deprecations
+ added validation checks to RegexFilter + added JVerify nullability annotations to RegexFilter + updated javadoc + replaced deprecated usages of CompositeFilter#getFilters with CompositeFilter#getFiltersArray in AbstractFilterableTest
New branch based on 2.x branch - replacing PR #3463 |
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.
Thanks so much for the hard work @JWT007! Dropped some remarks.
@Override | ||
public Result filter(final LogEvent event) { | ||
final String text = targetMessageTest(event.getMessage()); | ||
return filter(text); | ||
Objects.requireNonNull(event, "The 'event' argument must not be null."); |
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.
Objects.requireNonNull(event, "The 'event' argument must not be null."); | |
Objects.requireNonNull(event, "event"); |
* The regular expression to match. | ||
*/ | ||
@PluginBuilderAttribute | ||
@Required(message = "No 'regex' provided for RegexFilter") |
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.
Please don't provide a failure message for cases where the failure system should be providing the necessary context.
@Required(message = "No 'regex' provided for RegexFilter") | |
@Required |
/** {@inheritDoc} */ | ||
@Override | ||
public boolean isValid() { | ||
return (Strings.isNotEmpty(this.regex)); |
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.
return (Strings.isNotEmpty(this.regex)); | |
return Strings.isNotEmpty(this.regex); |
public @Nullable RegexFilter build() { | ||
|
||
// validate the "regex" attribute | ||
if (Strings.isEmpty(this.regex)) { | ||
LOGGER.error("Unable to create RegexFilter: The 'regex' attribute be set to a non-empty String."); | ||
return null; | ||
} | ||
|
||
// build with *safety* to not throw exceptions | ||
try { | ||
return new RegexFilter(this); | ||
} catch (final Exception ex) { | ||
LOGGER.error("Unable to create RegexFilter. {}", ex.getMessage(), ex); | ||
return null; | ||
} | ||
} |
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.
We had a dev@
discussion about this, and the conclusion is to throw exception on failures while building components. Would you mind updating this build()
logic accordingly, please?
* @deprecated pattern flags have been deprecated - they can just be included in the regex-expression. | ||
*/ | ||
@Deprecated | ||
private static final int DEFAULT_PATTERN_FLAGS = 0; |
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.
AFAIU, this constant is only used in toPatternFlags()
, could you move it into that method, please?
@Deprecated | ||
public String[] getPatternFlags() { | ||
return this.patternFlags.clone(); | ||
} |
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.
We're adding a new deprecated method? Why don't we simply not add it?
Boolean.TRUE.equals(useRawMsg), Pattern.compile(regex, toPatternFlags(patternFlags)), match, mismatch); | ||
|
||
// LOG4J-3086 - pattern-flags can be embedded in RegEx expression | ||
Objects.requireNonNull(regex, "The 'regex' argument must not be null."); |
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.
Objects.requireNonNull(regex, "The 'regex' argument must not be null."); | |
Objects.requireNonNull(regex, "regex"); |
(#3086)
Removed @PluginAttribute 'patternFlags' argument from https://github.com/pluginfactory for RegexFilter.
Actually created new https://github.com/pluginfactory without the argument and deprecated old 'createFilter' method.
See ticket for discussion of the reasoning - short version - pattern flags can be passed as part of RegEx expression.
Also guarded the "PatternCompile" method against excepttions - logging and returning null if the pattern fails to compile.
Recreated from 2.x and superceding PR #3463