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