Update StringMatchFilter builder API (#3509)#3510
Update StringMatchFilter builder API (#3509)#3510JWT007 wants to merge 4 commits intoapache:mainfrom
Conversation
* Guard against NPEs * added private constructor to builder * added getText() accessor * add JVerify nullability annotations * added detailed javadoc with implementation details * optimized AbstractLifeCycle and AbstractFilter "equalsImpl" implementations * added changelog
Generated by gradle/develocity-actions |
JWT007
added
bug
| }) | ||
| void testFilterBuilderFailsWithExceptionOnNullText() { | ||
| StringMatchFilter.Builder stringMatchFilterBuilder = StringMatchFilter.newBuilder(); | ||
| Assertions.assertThrows(IllegalArgumentException.class, () -> stringMatchFilterBuilder.setText(null)); |
There was a problem hiding this comment.
This is certainly debatable, but I would rather expect NPE to be thrown here.
The way I see it, there are two levels of validation:
- The parameter is annotated as
@NonNulland we expect the programmer to check for that. In this case NPE seems appropriate. - There is a more involved validation process that requires a non-empty string. In this case
IllegalArgumentExceptionseems appropriate.
There was a problem hiding this comment.
this is only because I used the Log4j Assert which throws IllegalArgumentException.
public Builder setText(final String text) {
this.text = Assert.requireNonEmpty(text, "The 'text' argument must not be null or empty.");
return this;
}
I could of course add an extra Objects.requiresNonNull if you think that is better?
There was a problem hiding this comment.
Maybe we can modify Assert.requireNonEmpty? What do you think?
It is mostly a question of convention (and we have none), but I think that it is worth differentiating the null and empty cases, since there are plenty of tools that do static analysis on nulls and the caller could use one.
| void testConfigurationWithTextNEG(final Configuration configuration) { | ||
| final Filter filter = configuration.getFilter(); | ||
| assertNull(filter, "The filter should be null."); | ||
| } |
There was a problem hiding this comment.
This would rather belong in a unit test for PluginBuilder or similar.
The fact that the user misspells StringMatchFilter or another name, doesn't seem relevant.
There was a problem hiding this comment.
this simply tests that a misconfigured (or not at all) fails on build and returns null I think.
A "" invalid name would throw in an entirely different locattion I think - unresolved plugin type.
There was a problem hiding this comment.
Unless I am mistaken this tests that a misspelled plugin name (StringMatchFfilter) will be ignored.
Since the plugin name is misspelled PluginBuilder will not find the plugin and return null. No code in StringMatchFilter.java will be executed.
| public Result filter( | ||
| final Logger logger, final Level level, final Marker marker, final Object msg, final Throwable t) { | ||
| return filter(logger.getMessageFactory().newMessage(msg).getFormattedMessage()); | ||
| public Result filter(final LogEvent event) { | ||
| Objects.requireNonNull(event, "The 'event' argument must not be null."); | ||
| return filter(event.getMessage().getFormattedMessage()); |
There was a problem hiding this comment.
Please don't sort methods. It makes it much harder to review the code.
There was a problem hiding this comment.
Agreed with @ppkarwasz. Please strive to avoid cosmetic changes on the existing code in PRs. Feel free to submit a follow-up PR/commit for these kind of changes.
3 participants
#3509
Fix StringMatchFilter Validation