NIFI-15224 Add Pre-Query and Post-Query to PutDatabaseRecord#10536
NIFI-15224 Add Pre-Query and Post-Query to PutDatabaseRecord#10536spects wants to merge 1 commit intoapache:mainfrom
Conversation
exceptionfactory
left a comment
exceptionfactory
left a comment
There was a problem hiding this comment.
Thanks for proposing this improvement @spects.
The general approach looks good, aligning with the feature implemented in ExecuteSQL.
I noted a few minor implementation recommendations.
| * Executes given queries using pre-defined connection. | ||
| * Returns null on success, or a query string if failed. | ||
| */ | ||
| private Pair<String, SQLException> executeConfigStatements(final Connection con, final List<String> configQueries) { |
There was a problem hiding this comment.
Catching and returning the exception, just to throw it from the calling method, does not seem like the best approach. I recommend simply declaring throws SQLException on this method and not returning anything.
| .description("A semicolon-delimited list of queries executed before the main SQL query is executed. " + | ||
| "For example, set session properties before main query. " + | ||
| "It's possible to include semicolons in the statements themselves by escaping them with a backslash ('\\;'). " + | ||
| "Results/outputs from these queries will be suppressed if there are no errors.") |
There was a problem hiding this comment.
A multiline string can be used instead of string concatenation to build the description.
| assertTrue(rs.next()); | ||
| assertEquals(1, rs.getInt(1)); | ||
| assertEquals("rec1", rs.getString(2)); | ||
| assertEquals(101, rs.getInt(3)); | ||
| assertEquals(jdbcDate1.toString(), rs.getDate(4).toString()); | ||
| assertTrue(rs.next()); | ||
| assertEquals(2, rs.getInt(1)); | ||
| assertEquals("rec2", rs.getString(2)); | ||
| assertEquals(102, rs.getInt(3)); | ||
| assertEquals(jdbcDate2.toString(), rs.getDate(4).toString()); | ||
| assertTrue(rs.next()); | ||
| assertEquals(3, rs.getInt(1)); | ||
| assertEquals("rec3", rs.getString(2)); | ||
| assertEquals(103, rs.getInt(3)); | ||
| assertNull(rs.getDate(4)); | ||
| assertTrue(rs.next()); | ||
| assertEquals(4, rs.getInt(1)); | ||
| assertEquals("rec4", rs.getString(2)); | ||
| assertEquals(104, rs.getInt(3)); | ||
| assertNull(rs.getDate(4)); | ||
| assertTrue(rs.next()); | ||
| assertEquals(5, rs.getInt(1)); | ||
| assertNull(rs.getString(2)); | ||
| assertEquals(105, rs.getInt(3)); | ||
| assertNull(rs.getDate(4)); | ||
| assertFalse(rs.next()); |
There was a problem hiding this comment.
These assertions appear to be duplicated in the other test method. Recommend creating a shared private assertResultsFound() method or similar to delegate the evaluation.
|
Thanks again for your work on this addition @spects. In order to address the feedback issues and move this forward, I submitted a new pull request and listed you as a co-author on the commit. |
3 participants
Summary
NIFI-15224
Implement SQL Pre-Query and SQL Post-Query in PutDatabaseRecord. Implementation draws from AbstractExecuteSQL's implementation.
Tracking
Please complete the following tracking steps prior to pull request creation.
Issue Tracking
Pull Request Tracking
NIFI-00000NIFI-00000Pull Request Formatting
mainbranchVerification
Please indicate the verification steps performed prior to pull request creation.
Build
./mvnw clean install -P contrib-checkLicensing
LICENSEandNOTICEfilesDocumentation