galovics commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r883538073
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}",
LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed =
Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+ Page<GenericResultsetData> reportData =
this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(),
null,
+ new ReportMapper(sqlStringBuilder));
+ if (isPaginationAllowed) {
+ sqlStringBuilder =
retrieveGenericResultsetWithPagination(sqlStringBuilder, queryParams);
}
+ result =
this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ int pageSize =
this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
- final String sql = getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport);
+ final long elapsed = System.currentTimeMillis() - startTime;
+ LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time:
{}", name, type, elapsed);
Review Comment:
Info is way too verbose here. Debug/trace please.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/data/GenericResultsetData.java:
##########
@@ -53,6 +61,11 @@ public String getColTypeOfColumnNamed(final String
columnName) {
return colType;
}
+ public static GenericResultsetData setTotalItemsAndRecordsPerPage(final
GenericResultsetData genericResultsetData, final int totalItems,
Review Comment:
This is a very bad pattern. This method is not setting anything, it's
constructing a brand new object. Why isn't the constructor above enough for
everything?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}",
LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed =
Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+ Page<GenericResultsetData> reportData =
this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(),
null,
+ new ReportMapper(sqlStringBuilder));
+ if (isPaginationAllowed) {
+ sqlStringBuilder =
retrieveGenericResultsetWithPagination(sqlStringBuilder, queryParams);
}
+ result =
this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ int pageSize =
this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
- final String sql = getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport);
+ final long elapsed = System.currentTimeMillis() - startTime;
+ LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time:
{}", name, type, elapsed);
Review Comment:
LogParameterEscapeUtil is also missing here.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -51,24 +59,31 @@
import
org.apache.fineract.infrastructure.documentmanagement.contentrepository.FileSystemContentRepository;
import
org.apache.fineract.infrastructure.security.service.PlatformSecurityContext;
import
org.apache.fineract.infrastructure.security.service.SqlInjectionPreventerService;
-import
org.apache.fineract.infrastructure.security.utils.LogParameterEscapeUtil;
import org.apache.fineract.useradministration.domain.AppUser;
import org.owasp.esapi.ESAPI;
import org.owasp.esapi.codecs.UnixCodec;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
import org.springframework.jdbc.core.JdbcTemplate;
import org.springframework.jdbc.core.RowMapper;
import org.springframework.jdbc.support.rowset.SqlRowSet;
import org.springframework.stereotype.Service;
@Service
-@Slf4j
+
@RequiredArgsConstructor
public class ReadReportingServiceImpl implements ReadReportingService {
+ private static final String ORDER_BY_REGEX_PATTERN = "^[0-9]*$";
+ private static final Logger LOG =
LoggerFactory.getLogger(ReadReportingServiceImpl.class);
Review Comment:
Why was the `@Slf4j` annotation replaced here?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}",
LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
Review Comment:
I don't see the reason to use a StringBuilder here. Clarify please.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}",
LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed =
Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+ Page<GenericResultsetData> reportData =
this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(),
null,
+ new ReportMapper(sqlStringBuilder));
Review Comment:
The ReportMapper is not doing anything, why do we have it?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}",
LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed =
Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+ Page<GenericResultsetData> reportData =
this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(),
null,
Review Comment:
Why is this needed, again?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -213,6 +251,21 @@ private String getSql(final String name, final String
type) {
throw new ReportNotFoundException(encodedName);
}
+ private static final class ReportMapper implements
RowMapper<GenericResultsetData> {
Review Comment:
This is superflous. Not doing anything.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}",
LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed =
Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+ Page<GenericResultsetData> reportData =
this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(),
null,
+ new ReportMapper(sqlStringBuilder));
+ if (isPaginationAllowed) {
+ sqlStringBuilder =
retrieveGenericResultsetWithPagination(sqlStringBuilder, queryParams);
}
+ result =
this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ int pageSize =
this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
- final String sql = getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport);
+ final long elapsed = System.currentTimeMillis() - startTime;
+ LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time:
{}", name, type, elapsed);
+ return GenericResultsetData.setTotalItemsAndRecordsPerPage(result,
reportData.getTotalFilteredRecords(), pageSize);
+ }
- final GenericResultsetData result =
this.genericDataService.fillGenericResultSet(sql);
+ public StringBuilder retrieveGenericResultsetWithPagination(final
StringBuilder sqlStringBuilder,
Review Comment:
This method is lying. It's not retrieving a generic resultset with
pagination. It's constructing the SQL for it.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}",
LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
Review Comment:
Also, you removed the LogParameterEscapeUtil call from here. Please redo it.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}",
LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
Review Comment:
Info is way too verbose here. Debug/trace please.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -151,21 +166,45 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final boolean isSelfServiceUserReport) {
final long startTime = System.currentTimeMillis();
- if (log.isDebugEnabled()) {
- log.debug("STARTING REPORT: {} Type: {}",
LogParameterEscapeUtil.escapeLogParameter(name),
- LogParameterEscapeUtil.escapeLogParameter(type));
+ LOG.info("STARTING REPORT: {} Type: {}", name, type);
+ StringBuilder sqlStringBuilder = new StringBuilder(200);
+ sqlStringBuilder.append(getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport));
+ final GenericResultsetData result;
+ boolean isPaginationAllowed =
Boolean.parseBoolean(queryParams.get(ReportingConstants.IS_PAGINATION_ALLOWED));
+ Page<GenericResultsetData> reportData =
this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(),
null,
+ new ReportMapper(sqlStringBuilder));
+ if (isPaginationAllowed) {
+ sqlStringBuilder =
retrieveGenericResultsetWithPagination(sqlStringBuilder, queryParams);
}
+ result =
this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ int pageSize =
this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
- final String sql = getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport);
+ final long elapsed = System.currentTimeMillis() - startTime;
+ LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time:
{}", name, type, elapsed);
+ return GenericResultsetData.setTotalItemsAndRecordsPerPage(result,
reportData.getTotalFilteredRecords(), pageSize);
+ }
- final GenericResultsetData result =
this.genericDataService.fillGenericResultSet(sql);
+ public StringBuilder retrieveGenericResultsetWithPagination(final
StringBuilder sqlStringBuilder,
+ final Map<String, String> queryParams) {
+ retrieveGenericResultsetWithPaginationDataValidator(queryParams);
+ int pageSize =
this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
+ int pageNo =
Integer.parseInt(queryParams.get(ReportingConstants.OFFSET));
+
+ pageNo = pageNo * pageSize;
+ sqlStringBuilder.append(" order by
").append(queryParams.get(PAGINATION_ORDER_BY));
+ sqlStringBuilder.append(" ");
Review Comment:
Very very bad pattern. You're making this method to have side-effects while
also returning the object. Decide on one and do it accordingly. Preferably
return the Stirng.
--
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.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
