New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fixes to ogc-server-statistics to pass all tests #2392
Fixes to ogc-server-statistics to pass all tests #2392
Conversation
- Move repeated hardcoded values for statistics table and column names to a constants in a LogColumns utility. - Use try-with-resources when dealing with JDBC prepared statements and resultsets. - Do now swallow exception cause when throwing DataCommandException
5b0f929
to
b6cef72
Compare
pStmt.execute("DELETE FROM ogcstatistics.OGC_SERVICES_LOG"); | ||
|
||
try (Statement pStmt = this.connection.createStatement()){ | ||
pStmt.execute(String.format("DELETE FROM %s", QUALIFIED_TABLE_NAME)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm aware that QUALIFIED_TABLE_NAME is not an user input, but String.format() to format SQL queries is prone to sql injections
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Right. In this case it's safe as QUALIFIED_TABLE_NAME
is a constant to avoid possible typos. It would also make it safe in the event the table name changed (e.g. it has the schema hardcoded, might that change eventually?).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No plan to change this.
Honestly, this looks safe enough for me.
try { | ||
this.connection.setAutoCommit(true); | ||
} catch (SQLException e1) { | ||
e1.printStackTrace(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should be logged instead of putting it on stdout ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes, just amended it.
...erver-statistics/src/main/java/org/georchestra/ogcservstatistics/log4j/OGCServiceParser.java
Show resolved
Hide resolved
Most failed because OGCServiceParser couldn't parse the OGC service name, especially out of the HTTP POST requests log messages. Fixed using regular expressions. The only test passing was OGCServiceParserTest, but lacked test cases for the failing messages, which were added.
b6cef72
to
6227a61
Compare
This pull request fixes #2076
There were two major errors:
RetrieveAllCommand
had a hardcoded column namesecrole
instead ofroles
, which directly prevented all online tests from running.After fixing that, most tests failed because OGCServiceParser couldn't parse the service name, especially out of the HTTP POST requests log messages. Fixed using regular expressions.
The only test passing was OGCServiceParserTest, but test cases for the failing messages, which were added.