I have created a jdbc_cve branch in qos-ch/reload4j repo based on PR 26
Please check 26 once again. I made it to contain two commits: 1) revert "jdbcappender removal" 2) cve fix. That enables minimizing and isolating changes related to the CVE fix. -----
1) reformat using javaConventions.xml
While I have no problems with doing that, reformatting **between** CVE-related fixes (mine and your activateOptions) makes review harder. For instance, as one of the changes you **killed** JDBCAppender#removes field which silently breaks backward compatibility. If you absolutely want reformat, then I would suggest: * "revert removal jdbc" * "reformat" * "fix CVE"
2) JdbcPatterParser is created only once in JdbcAppender.activateOptions and then reused.
I did not know activateOptions existed. Picking options in activateOptions
3) Modified JdbcPatternParserTest to make them a bit easier to follow, at least for me
While I do not care how the three tests are written, I strongly believe ParserState is way harder for both **read** and **write** test. It overcomplicates things (e.g. equals/hashcode boilerplate which is one for place for error). My test is 25 lines, your test is 100 lines, while effectively they have exactly the same test coverage. As you introduced ParserState, the test creation became a manual process. You need to carefully create ParserState instance, add quotes, etc. Then, the error message would look different from what the code looks like. That means a failing test would be harder to understand. My approach was to compare strings, so I can easily add new tests by just adding input and leaving the expected value blank. Then I would just grab the "expected" output from the test failure and use it as the expected value in the test assertion. Then, I see you've removed assert message. That is really sad as it would make it complicated to analyze the failure. Here's an example: My approach: assertParameterizedSql( "JdbcPatternParser{sql=insert into t(message) values ('abc'),args=[]}", "insert into t(message) values ('%m''%m')" ); Failure: org.junit.ComparisonFailure: parser.setPattern(...).toString() for insert into t(message) values ('%m''%m') Expected :JdbcPatternParser{sql=insert into t(message) values ('abc'),args=[]} Actual :JdbcPatternParser{sql=insert into t(message) values (?),args=[%m'%m]} ---- Your approach: public void testSingleQuotesAndSpaces2() { ParserState expected = new ParserState("insert into t(message) values ('abc')"); otherAssert("insert into t(message) values ('%m''%m')", expected); Failure: Expected :ParserState [expected=insert into t(message) values ('abc'), args=[]] Actual :ParserState [expected=insert into t(message) values (?), args=[%m'%m]] Note how you don't know **WHY** something is expected, and you can't proofread the failure to tell if that is legit or not. The assertion message should be there to tell what was the input, so the one who runs into test failure can understand it. Final 2c: in your branch, JdbcPatternParserTest mixes tabs and spaces, which is a frustrating thing for a **new** class. Vladimir