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:
[email protected]
