<div dir="ltr"><div dir="ltr">>Not silently as we would have amply publicized this.</div><div dir="ltr"><div><br></div><div>I strongly suggest keeping the current APIs if possible (keep old fields, types, etc).</div><div>It does not help much if we drop JDBCAppender#removes field, and the removal definitely breaks apps.</div><div>So why remove it?</div><div><br></div><div>I intentionally avoided making extra cleanup changes in PR26 so we can agree on the approach and merge it.</div><div>I assumed the cleanup can go afterward if needed.</div><div><br></div><div>Frankly speaking, I'm inclined that there might be a good case for several JDBC appenders.</div><div><br></div><div>For instance:</div><div>-jdbc-tableappender.jar</div><div>The users would be able to configure table name and column names.</div><div>The appender would write to the table and that would be it.</div><div><br></div><div>-jdbc-freesql.jar</div><div>The user configures SQL, and the appender uses it via prepared statement.</div><div>This is less secure than -jdbc-tableappender, because an attacker would be able</div><div>to execute an arbitrary SQL if they can edit the configuration file.</div><div><br></div><div>The same thing might apply to other appenders (and logback? ;) ).</div><div>If that is something appealing to you, then imagine we can move the current JDBCAppender to</div><div>reload4j-jdbc-legacy.jar and we could let it die there. We don't need to refactor and modernize it then.</div><div><br></div></div><div dir="ltr">>cure mode might be quite hard to defend in the current</div>>climate, even as an opt-in feature.<div><br></div><div>Frankly speaking, I assumed that there might be systems that</div><div>use the current JDBCAppender in a secure manner (e.g. their format message does not include user-provided input),</div><div>so I assumed it might be not that bad to just leave the fallback.</div><div><br></div><div>On the other hand, if their format string is not supported by the new parser, adding quotes should be enough,</div><div>so it might be just fine to drop insecure mode.</div><div><br></div><div>There's a possibility that people used "table rotation" via insert into logs%d{yyyy}(...).</div><div>This case is not supported by my fix, and I think it would be hard to support in a secure manner.</div><div><br></div><div>>You must construct the expected result by hand. This implies that you</div>>know what you are doing when you write the test. That is kind of the point<div><br></div><div>Frankly speaking, I think the added ParserState boilerplate (equals, hashcode, etc) negates all that.</div><div>getCopyOfpatternStringRepresentationList looks crazy (naming is hard), so I would prefer simpler designs,</div><div>especially for an internal (package-private) class like JdbcPatternParser.</div><div>Then, JdbcPatternParser#toString() assertion ensured the object would look sane in the debugger.</div><div><br></div><div>---</div><div><br></div><div>>I was not aware of that. It must be the mixed tab mode which is the<br>>default. Would "only spaces" be better?<br></div><div><br></div><div>That is why suggested .editorconfig and its indent_style = space.</div><div>Reviewing changes is harder when the code layout is broken,</div><div>and it is often broken when tabs and spaces are mixed in the same file.</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>