galovics commented on code in PR #2310:
URL: https://github.com/apache/fineract/pull/2310#discussion_r864571483
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/GenericDataServiceImpl.java:
##########
@@ -88,7 +88,7 @@ public GenericResultsetData fillGenericResultSet(final String
sql) {
resultsetDataRows.add(resultsetDataRow);
}
- return new GenericResultsetData(columnHeaders, resultsetDataRows);
+ return new GenericResultsetData(columnHeaders, resultsetDataRows,
0, 0);
Review Comment:
I mean 0, 0 makes sense here? I don't think so.
The total size of the result should be the size of the resultsetDataRows,
isn't it? Same for the recordsPerPage since this is not a paginated API.
Let's try to be consistent with the data we're trying to represent.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -161,16 +167,48 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
-
- final String sql = getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport);
-
- final GenericResultsetData result =
this.genericDataService.fillGenericResultSet(sql);
+ final 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));
+
+ if (isPaginationAllowed) {
+ result = retrieveGenericResultsetWithPagination(sqlStringBuilder,
queryParams);
Review Comment:
Let's not pass a mutable StringBuilder as a parameter.
The code should be looking like this:
```
String sql = ... // base SQL
...
if (pagination) {
sql = retrievePaginatedSql(sql)
}
...
return fillGenericResultset(sql)
```
Or similar to this.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java:
##########
@@ -1315,7 +1315,7 @@ public GenericResultsetData
retrieveDataTableGenericResultSet(final String dataT
final List<ResultsetRowData> result =
fillDatatableResultSetDataRows(sql);
- return new GenericResultsetData(columnHeaders, result);
+ return new GenericResultsetData(columnHeaders, result, 0, 0);
Review Comment:
Same as for the other.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -161,16 +167,48 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
-
- final String sql = getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport);
-
- final GenericResultsetData result =
this.genericDataService.fillGenericResultSet(sql);
+ final 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));
+
+ if (isPaginationAllowed) {
+ result = retrieveGenericResultsetWithPagination(sqlStringBuilder,
queryParams);
+ } else {
+ result =
this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ }
final long elapsed = System.currentTimeMillis() - startTime;
LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time:
{}", name, type, elapsed);
return result;
}
+ public GenericResultsetData retrieveGenericResultsetWithPagination(final
StringBuilder sqlStringBuilder,
+ final Map<String, String> queryParams) {
+ final GenericResultsetData result;
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+
+ final DataValidatorBuilder baseDataValidator = new
DataValidatorBuilder(dataValidationErrors);
+
baseDataValidator.reset().parameter(PAGE_NO).value(queryParams.get(PAGE_NO)).notNull().throwValidationErrors();
+
+
baseDataValidator.reset().parameter(PAGINATION_ORDER_BY).value(queryParams.get(PAGINATION_ORDER_BY)).ignoreIfNull()
+
.matchesRegularExpression(ORDER_BY_REGEX_PATTERN).throwValidationErrors();
+ int pageSize =
this.configurationDomainService.reportsPaginationNumberOfItemsPerPage();
+
+ Page<GenericResultsetData> reportData =
this.paginationHelper.fetchPage(this.jdbcTemplate, sqlStringBuilder.toString(),
null,
+ new ReportMapper(sqlStringBuilder));
+ int pageNo =
Integer.parseInt(queryParams.get(ReportingConstants.PAGE_NO));
+
+ pageNo = pageNo * pageSize;
+ sqlStringBuilder.append(" order by
").append(queryParams.get(PAGINATION_ORDER_BY));
+ sqlStringBuilder.append(" ");
+ sqlStringBuilder.append(sqlGenerator.limit(pageSize, pageNo));
+ result =
this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ result.setTotalItems(reportData.getTotalFilteredRecords());
Review Comment:
I don't like this. Manipulating this after it's already constructed seems to
be a big code smell. Why don't we set these during construction time?
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -61,23 +71,19 @@
import org.springframework.stereotype.Service;
@Service
+@AllArgsConstructor(onConstructor = @__(@Autowired))
Review Comment:
RequiredArgsConstructor makes more sense here plus there's no need to put
the Autowired annotation onto the generated constructor since Spring 4.3+
supports constructor dependency injection without it.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadWriteNonCoreDataServiceImpl.java:
##########
@@ -1337,7 +1337,7 @@ private GenericResultsetData
retrieveDataTableGenericResultSetForUpdate(final St
final List<ResultsetRowData> result =
fillDatatableResultSetDataRows(sql);
- return new GenericResultsetData(columnHeaders, result);
+ return new GenericResultsetData(columnHeaders, result, 0, 0);
Review Comment:
Same as for the other.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -161,16 +167,48 @@ public GenericResultsetData
retrieveGenericResultset(final String name, final St
final long startTime = System.currentTimeMillis();
LOG.info("STARTING REPORT: {} Type: {}", name, type);
-
- final String sql = getSQLtoRun(name, type, queryParams,
isSelfServiceUserReport);
-
- final GenericResultsetData result =
this.genericDataService.fillGenericResultSet(sql);
+ final 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));
+
+ if (isPaginationAllowed) {
+ result = retrieveGenericResultsetWithPagination(sqlStringBuilder,
queryParams);
+ } else {
+ result =
this.genericDataService.fillGenericResultSet(sqlStringBuilder.toString());
+ }
final long elapsed = System.currentTimeMillis() - startTime;
LOG.info("FINISHING Report/Request Name: {} - {} Elapsed Time:
{}", name, type, elapsed);
return result;
}
+ public GenericResultsetData retrieveGenericResultsetWithPagination(final
StringBuilder sqlStringBuilder,
+ final Map<String, String> queryParams) {
+ final GenericResultsetData result;
+ final List<ApiParameterError> dataValidationErrors = new ArrayList<>();
+
+ final DataValidatorBuilder baseDataValidator = new
DataValidatorBuilder(dataValidationErrors);
Review Comment:
Please extract the validation to a separate method/class to improve
readability.
##########
fineract-provider/src/main/java/org/apache/fineract/infrastructure/dataqueries/service/ReadReportingServiceImpl.java:
##########
@@ -61,23 +71,19 @@
import org.springframework.stereotype.Service;
@Service
+@AllArgsConstructor(onConstructor = @__(@Autowired))
public class ReadReportingServiceImpl implements ReadReportingService {
+ private static final String ORDER_BY_REGEX_PATTERN = "^[0-9]*$";
Review Comment:
I just realized this. Why do we want to sort only based on the number of the
column? This can be very difficult to maintain. Imagine people start building
on this and suddenly we change the SQL that we run and we switch up the column
order. Functionally nothing will break without ordering but if you use
ordering, it's not gonna sort on the expected columns.
Why don't we switch this into providing the column name an order by that?
If this is something you really want to keep (column number ordering) then
please add test cases to cover every single column ordering, otherwise we'll
regress in the future.
##########
integration-tests/src/test/java/org/apache/fineract/integrationtests/StretchyReportTest.java:
##########
@@ -0,0 +1,108 @@
+/**
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * http://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.apache.fineract.integrationtests;
+
+import static org.junit.jupiter.api.Assertions.assertEquals;
+
+import io.restassured.builder.RequestSpecBuilder;
+import io.restassured.builder.ResponseSpecBuilder;
+import io.restassured.http.ContentType;
+import io.restassured.specification.RequestSpecification;
+import io.restassured.specification.ResponseSpecification;
+import java.util.ArrayList;
+import java.util.HashMap;
+import java.util.LinkedHashMap;
+import
org.apache.fineract.integrationtest.stretchyreports.StretchyReportHelper;
+import org.apache.fineract.integrationtests.common.ClientHelper;
+import org.apache.fineract.integrationtests.common.CommonConstants;
+import org.apache.fineract.integrationtests.common.GlobalConfigurationHelper;
+import org.apache.fineract.integrationtests.common.Utils;
+import org.junit.jupiter.api.Assertions;
+import org.junit.jupiter.api.BeforeEach;
+import org.junit.jupiter.api.Test;
+import org.slf4j.Logger;
+import org.slf4j.LoggerFactory;
+
+public class StretchyReportTest {
+
+ private static final Logger LOG =
LoggerFactory.getLogger(StretchyReportTest.class);
+ private RequestSpecification requestSpec;
+ private ResponseSpecification responseSpec;
+ private StretchyReportHelper stretchyReportHelper;
+ private static final String STRETCHY_GET_REPORT_URL =
"/fineract-provider/api/v1/reports";
+ private static final String STRETCHY_REPORT_URL =
"/fineract-provider/api/v1/runreports";
+
+ @BeforeEach
+ public void setup() {
+ Utils.initializeRESTAssured();
+ this.requestSpec = new
RequestSpecBuilder().setContentType(ContentType.JSON).build();
+ this.requestSpec.header("Authorization", "Basic " +
Utils.loginIntoServerAndGetBase64EncodedAuthenticationKey());
+ this.responseSpec = new
ResponseSpecBuilder().expectStatusCode(200).build();
+ }
+
+ @Test
+ public void testReportPagination() {
+ this.stretchyReportHelper = new StretchyReportHelper(this.requestSpec,
this.responseSpec);
+
+ final ResponseSpecification errorResponse = new
ResponseSpecBuilder().expectStatusCode(400).build();
+ StretchyReportHelper validationErrorHelper = new
StretchyReportHelper(this.requestSpec, errorResponse);
+ for (int i = 0; i < 20; i++) {
+ final Integer clientID =
ClientHelper.createClient(this.requestSpec, this.responseSpec);
+ ClientHelper.verifyClientCreatedOnServer(this.requestSpec,
this.responseSpec, clientID);
+ }
+ String url = STRETCHY_REPORT_URL + "/" + "Client Listing" + "?" +
Utils.TENANT_IDENTIFIER + "&R_officeId=1";
+ LinkedHashMap reportData =
this.stretchyReportHelper.getStretchyReportDetail(this.requestSpec,
this.responseSpec, url, "");
+ ArrayList<String> rdata = (ArrayList<String>) reportData.get("data");
+ Integer reportDataSize = rdata.size();
+ Assertions.assertNotNull(reportDataSize);
+
+ Boolean isPaginationAllowed = true;
+ if (isPaginationAllowed) {
Review Comment:
Okay but then why the if? The test-case is clearly for paginationAllowed
only but then why the check?
--
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]
