[GitHub] [fineract] josemakara2 commented on a change in pull request #1671: Use prepared statements instead of string concatenated SQL everywhere - PART 1 (FINERACT-854)

[GitBox <gi...@apache.org>]

josemakara2 commented on a change in pull request #1671:
URL: https://github.com/apache/fineract/pull/1671#discussion_r611042926



##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/email/service/EmailReadPlatformServiceImpl.java
##########
@@ -130,46 +130,41 @@ public EmailData retrieveOne(final Long resourceId) {
     @Override
     public Collection<EmailData> retrieveAllPending(final SearchParameters searchParameters) {
         final String sqlPlusLimit = (searchParameters.getLimit() > 0) ? " limit 0, " + searchParameters.getLimit() : "";
-        final String sql = "select " + this.emailRowMapper.schema() + " where emo.status_enum = "
-                + EmailMessageStatusType.PENDING.getValue() + sqlPlusLimit;
+        final String sql = "select " + this.emailRowMapper.schema() + " where emo.status_enum = " + sqlPlusLimit;

Review comment:
       My bad. Good find, to fix to `where emo.status_enum = ?`

##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/campaigns/email/service/EmailReadPlatformServiceImpl.java
##########
@@ -130,46 +130,41 @@ public EmailData retrieveOne(final Long resourceId) {
     @Override
     public Collection<EmailData> retrieveAllPending(final SearchParameters searchParameters) {
         final String sqlPlusLimit = (searchParameters.getLimit() > 0) ? " limit 0, " + searchParameters.getLimit() : "";
-        final String sql = "select " + this.emailRowMapper.schema() + " where emo.status_enum = "
-                + EmailMessageStatusType.PENDING.getValue() + sqlPlusLimit;
+        final String sql = "select " + this.emailRowMapper.schema() + " where emo.status_enum = " + sqlPlusLimit;
 
-        return this.jdbcTemplate.query(sql, this.emailRowMapper, new Object[] {});
+        return this.jdbcTemplate.query(sql, this.emailRowMapper, new Object[] { EmailMessageStatusType.PENDING.getValue() });
     }
 
     @Override
     public Collection<EmailData> retrieveAllSent(final SearchParameters searchParameters) {
         final String sqlPlusLimit = (searchParameters.getLimit() > 0) ? " limit 0, " + searchParameters.getLimit() : "";
-        final String sql = "select " + this.emailRowMapper.schema() + " where emo.status_enum = " + EmailMessageStatusType.SENT.getValue()
-                + sqlPlusLimit;
+        final String sql = "select " + this.emailRowMapper.schema() + " where emo.status_enum = ?" + sqlPlusLimit;
 
-        return this.jdbcTemplate.query(sql, this.emailRowMapper, new Object[] {});
+        return this.jdbcTemplate.query(sql, this.emailRowMapper, new Object[] { EmailMessageStatusType.SENT.getValue() });
     }
 
     @Override
     public List<Long> retrieveExternalIdsOfAllSent(final Integer limit) {
         final String sqlPlusLimit = (limit > 0) ? " limit 0, " + limit : "";
-        final String sql = "select external_id from " + this.emailRowMapper.tableName() + " where status_enum = "
-                + EmailMessageStatusType.SENT.getValue() + sqlPlusLimit;
+        final String sql = "select external_id from " + this.emailRowMapper.tableName() + " where status_enum = " + sqlPlusLimit;

Review comment:
       To fix to ` where status_enum =?`

##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/api/RunreportsApiResource.java
##########
@@ -105,6 +109,10 @@ public Response runReport(@PathParam("reportName") @Parameter(description = "rep
         // Pass through isSelfServiceUserReport so that ReportingProcessService implementations can use it
         queryParams.putSingle(IS_SELF_SERVICE_USER_REPORT_PARAMETER, Boolean.toString(isSelfServiceUserReport));
 
+        if (parameterType) {
+            return datatableReportingProcessService.processRequest(reportName, queryParams);
+        }
+

Review comment:
       I had toyed with the idea of splitting it, checked the code review guidelines and there was no mention of one PR commit one task one fix. Since it wasn't a code dump, I decided to fix it in this PR because it was blocking me from testing on a _related_ change on community-app..
   
   I agree though with idea of one PR commit, one fix. 
   - I will create a separate jira re Fix runreports
   - Remove it from here
   - And assign to you for review
   




-- 
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.

For queries about this service, please contact Infrastructure at:
users@infra.apache.org