[reload4j] the jdbc_cve branch

[Vladimir Sitnikov]

>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
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mailman.qos.ch/pipermail/reload4j/attachments/20220120/26c8f2f8/attachment.html>