[GitHub] [fineract] ptuomola 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>]

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



##########
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:
       Isn't there a '?' missing here? ie 'where emo.status_enum = ?'. Or am I missing something and ? gets appended elsewhere?
   
   Also I don't think this is vulnerable for SQL injection as we are apending the value directly from the enum - but of course it makes sense to use parameters rather than string concatenation. 

##########
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:
       IMHO this should be a separate PR as it is not related to the topic / JIRA of this PR... can we split it out to a separate one with reference to a right JIRA? Otherwise it will be very confusing afterwards to work out what was changed where...

##########
File path: fineract-provider/src/main/java/org/apache/fineract/infrastructure/security/service/JdbcTenantDetailsService.java
##########
@@ -49,7 +49,9 @@ public JdbcTenantDetailsService(@Qualifier("hikariTenantDataSource") final DataS
 
     private static final class TenantMapper implements RowMapper<FineractPlatformTenant> {
 
-        private final StringBuilder sqlBuilder = new StringBuilder("t.id, ts.id as connectionId , ")//
+        private final String tenantIdentifier;

Review comment:
       This is starting to break the RowMapper pattern a bit... IMHO the purpose of RowMapper is to map one record of ResultSet to corresponding DTO. It should not care about what was the query used to get the result set - i.e. you should be able to reuse the same RowMapper in anything that queries a specific table. That's why the schema() method should return just the schema of the object, not the where condition as well.
   
   Was there a reason why the where condition was moved to the schema()? Why not stick to the pattern and have the schema() return the schema for the object, and the calling method specify the where condition?

##########
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:
       Same here - isn't this missing a ?

##########
File path: fineract-provider/src/main/resources/sql/migrations/core_db/V365__reportCategoryList-FINERACT-1306.sql
##########
@@ -18,5 +18,10 @@
 --
 
 -- two tables added: ReportCategoryList and FullReportList (FINERACT-1306)
-INSERT INTO stretchy_report (report_name, report_type, report_category, report_sql, description, core_report, use_report)VALUES ("ReportCategoryList", 'Table', '(NULL)', '(NULL)', '(NULL)', 1, 1);
-INSERT INTO stretchy_report (report_name, report_type, report_category, report_sql, description, core_report, use_report)VALUES ("FullReportList", 'Table', '(NULL)', '(NULL)', '(NULL)', 1, 1);
+INSERT INTO stretchy_report (report_name, report_type, report_category, report_sql, description, core_report, use_report)

Review comment:
       I think this duplicates another PR that was fixing the issue with RunReports... my suggestion would be to pull this to a separate PR rather than combine with the SQL parameterisation. 




-- 
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