From 8f2ea7fa151ce4d9d093aade4d0ab29cce7929d2 Mon Sep 17 00:00:00 2001 From: d-bernat Date: Wed, 27 Dec 2023 10:50:51 +0100 Subject: [PATCH 1/4] fix: Table population --- .../dhis/analytics/table/JdbcAnalyticsTableManager.java | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/table/JdbcAnalyticsTableManager.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/table/JdbcAnalyticsTableManager.java index dc98e3b9a2b0..3107fd46b2a8 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/table/JdbcAnalyticsTableManager.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/table/JdbcAnalyticsTableManager.java @@ -163,9 +163,9 @@ public AnalyticsTableType getAnalyticsTableType() { public List getAnalyticsTables(AnalyticsTableUpdateParams params) { AnalyticsTable table = params.isLatestUpdate() - ? getLatestAnalyticsTable(params, getDimensionColumns(), getValueColumns()) + ? getLatestAnalyticsTable(params, getDimensionColumns(params), getValueColumns()) : getRegularAnalyticsTable( - params, getDataYears(params), getDimensionColumns(), getValueColumns()); + params, getDataYears(params), getDimensionColumns(params), getValueColumns()); return table.hasPartitionTables() ? List.of(table) : List.of(); } @@ -432,8 +432,8 @@ private String getApprovalJoinClause(Integer year) { return StringUtils.EMPTY; } - private List getDimensionColumns() { - return getDimensionColumns(null, null); + private List getDimensionColumns(AnalyticsTableUpdateParams params) { + return getDimensionColumns(null, params); } private List getDimensionColumns( From ccf5927b060693c5889d2cbcc5b31917a30c2413 Mon Sep 17 00:00:00 2001 From: d-bernat Date: Thu, 28 Dec 2023 10:45:41 +0100 Subject: [PATCH 2/4] continuous analytics --- .../org/hisp/dhis/feedback/ErrorCode.java | 5 +++++ .../ContinuousAnalyticsTableJob.java | 19 +++++++++++++++++++ .../AnalyticsOutlierDetectionController.java | 19 +++++++++++++++++++ 3 files changed, 43 insertions(+) diff --git a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/feedback/ErrorCode.java b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/feedback/ErrorCode.java index 7248f510574b..70c0141358d2 100644 --- a/dhis-2/dhis-api/src/main/java/org/hisp/dhis/feedback/ErrorCode.java +++ b/dhis-2/dhis-api/src/main/java/org/hisp/dhis/feedback/ErrorCode.java @@ -438,6 +438,11 @@ public enum ErrorCode { "Query failed because a referenced table does not exist. Please ensure analytics job was run"), E7145("Query failed because of a syntax error"), + /* Analytics outliers */ + + E7180( + "The analytics outliers data does not exist. Please ensure analytics job was run and did not skip the outliers"), + /* Event analytics */ E7200(Constants.AT_LEAST_ONE_ORGANISATION_UNIT_MUST_BE_SPECIFIED), E7201("Dimensions cannot be specified more than once: `{0}`"), diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/table/scheduling/ContinuousAnalyticsTableJob.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/table/scheduling/ContinuousAnalyticsTableJob.java index a5b7d6c146c5..3dc7c0f9297e 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/table/scheduling/ContinuousAnalyticsTableJob.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/table/scheduling/ContinuousAnalyticsTableJob.java @@ -36,6 +36,7 @@ import org.apache.commons.lang3.ObjectUtils; import org.hisp.dhis.analytics.AnalyticsTableGenerator; import org.hisp.dhis.analytics.AnalyticsTableUpdateParams; +import org.hisp.dhis.analytics.common.TableInfoReader; import org.hisp.dhis.scheduling.Job; import org.hisp.dhis.scheduling.JobConfiguration; import org.hisp.dhis.scheduling.JobProgress; @@ -69,6 +70,8 @@ public class ContinuousAnalyticsTableJob implements Job { private final SystemSettingManager systemSettingManager; + private final TableInfoReader tableInfoReader; + @Override public JobType getJobType() { return JobType.CONTINUOUS_ANALYTICS_TABLE; @@ -79,6 +82,12 @@ public void execute(JobConfiguration jobConfiguration, JobProgress progress) { ContinuousAnalyticsJobParameters parameters = (ContinuousAnalyticsJobParameters) jobConfiguration.getJobParameters(); + if (!checkJobOutliersConsistency(parameters)) { + log.info( + "Updating the analytics table is currently not feasible. The incoming job parameters do not align with the existing outliers data structure, preventing a successful update."); + return; + } + Integer fullUpdateHourOfDay = ObjectUtils.firstNonNull(parameters.getFullUpdateHourOfDay(), DEFAULT_HOUR_OF_DAY); @@ -132,4 +141,14 @@ public void execute(JobConfiguration jobConfiguration, JobProgress progress) { analyticsTableGenerator.generateTables(params, progress); } } + + private boolean checkJobOutliersConsistency(ContinuousAnalyticsJobParameters parameters) { + boolean analyticsTableWithOutliers = + tableInfoReader.getInfo("analytics").getColumns().stream() + .anyMatch("sourceid"::equalsIgnoreCase); + boolean outliersRequired = !parameters.getSkipOutliers(); + + return outliersRequired && analyticsTableWithOutliers + || !outliersRequired && !analyticsTableWithOutliers; + } } diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/outlierdetection/AnalyticsOutlierDetectionController.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/outlierdetection/AnalyticsOutlierDetectionController.java index 35ae6b6763fc..c69f065782f5 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/outlierdetection/AnalyticsOutlierDetectionController.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/outlierdetection/AnalyticsOutlierDetectionController.java @@ -37,6 +37,7 @@ import java.io.IOException; import javax.servlet.http.HttpServletResponse; import lombok.AllArgsConstructor; +import org.hisp.dhis.analytics.common.TableInfoReader; import org.hisp.dhis.analytics.outlier.data.OutlierQueryParams; import org.hisp.dhis.analytics.outlier.data.OutlierQueryParser; import org.hisp.dhis.analytics.outlier.data.OutlierRequest; @@ -44,7 +45,10 @@ import org.hisp.dhis.analytics.outlier.service.AnalyticsOutlierService; import org.hisp.dhis.common.DhisApiVersion; import org.hisp.dhis.common.Grid; +import org.hisp.dhis.common.IllegalQueryException; import org.hisp.dhis.common.OpenApi; +import org.hisp.dhis.feedback.ErrorCode; +import org.hisp.dhis.feedback.ErrorMessage; import org.hisp.dhis.webapi.mvc.annotation.ApiVersion; import org.hisp.dhis.webapi.utils.ContextUtils; import org.springframework.security.access.prepost.PreAuthorize; @@ -68,12 +72,14 @@ public class AnalyticsOutlierDetectionController { private final ContextUtils contextUtils; private final OutlierQueryParser queryParser; private final OutlierRequestValidator validator; + private final TableInfoReader tableInfoReader; @PreAuthorize("hasRole('ALL') or hasRole('F_PERFORM_ANALYTICS_EXPLAIN')") @GetMapping( value = RESOURCE_PATH + "/explain", produces = {APPLICATION_JSON_VALUE, "application/javascript"}) public @ResponseBody Grid getExplainOutliersJson(OutlierQueryParams query) { + checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(query, true); return outlierService.getOutliersPerformanceMetrics(request); @@ -81,6 +87,7 @@ public class AnalyticsOutlierDetectionController { @GetMapping(value = RESOURCE_PATH, produces = APPLICATION_JSON_VALUE) public Grid getOutliersJson(OutlierQueryParams queryParams) { + checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(queryParams, false); Grid grid = outlierService.getOutliers(request); @@ -95,6 +102,7 @@ public Grid getOutliersJson(OutlierQueryParams queryParams) { @GetMapping(value = RESOURCE_PATH + ".csv") public void getOutliersCsv(OutlierQueryParams queryParams, HttpServletResponse response) throws IOException { + checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(queryParams, false); contextUtils.configureResponse(response, CONTENT_TYPE_CSV, NO_CACHE, "outlierdata.csv", true); @@ -104,6 +112,7 @@ public void getOutliersCsv(OutlierQueryParams queryParams, HttpServletResponse r @GetMapping(value = RESOURCE_PATH + ".xml") public void getOutliersXml(OutlierQueryParams queryParams, HttpServletResponse response) throws IOException { + checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(queryParams, false); contextUtils.configureResponse(response, CONTENT_TYPE_XML, NO_CACHE); @@ -113,6 +122,7 @@ public void getOutliersXml(OutlierQueryParams queryParams, HttpServletResponse r @GetMapping(value = RESOURCE_PATH + ".xls") public void getOutliersXls(OutlierQueryParams queryParams, HttpServletResponse response) throws IOException { + checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(queryParams, false); contextUtils.configureResponse(response, CONTENT_TYPE_EXCEL, NO_CACHE, "outlierdata.xls", true); @@ -122,6 +132,7 @@ public void getOutliersXls(OutlierQueryParams queryParams, HttpServletResponse r @GetMapping(value = RESOURCE_PATH + ".html") public void getOutliersHtml(OutlierQueryParams queryParams, HttpServletResponse response) throws IOException { + checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(queryParams, false); contextUtils.configureResponse(response, CONTENT_TYPE_HTML, NO_CACHE); @@ -132,6 +143,7 @@ public void getOutliersHtml(OutlierQueryParams queryParams, HttpServletResponse @GetMapping(value = RESOURCE_PATH + ".html+css") public void getOutliersHtmlCss(OutlierQueryParams queryParams, HttpServletResponse response) throws IOException { + checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(queryParams, false); contextUtils.configureResponse(response, CONTENT_TYPE_HTML, NO_CACHE); @@ -144,4 +156,11 @@ private OutlierRequest getFromQuery(OutlierQueryParams queryParams, boolean anal return request; } + + private void checkAnalyticsTableForOutliers() { + if (tableInfoReader.getInfo("analytics").getColumns().stream() + .noneMatch("sourceid"::equalsIgnoreCase)) { + throw new IllegalQueryException(new ErrorMessage(ErrorCode.E7180)); + } + } } From b68507c2170a492c8210af99fcfd9b6a16cbcf32 Mon Sep 17 00:00:00 2001 From: d-bernat Date: Thu, 28 Dec 2023 11:33:04 +0100 Subject: [PATCH 3/4] post code review impl --- .../service/AnalyticsOutlierService.java | 18 +++++++++++++ .../AnalyticsOutlierDetectionController.java | 26 +++++-------------- 2 files changed, 25 insertions(+), 19 deletions(-) diff --git a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/outlier/service/AnalyticsOutlierService.java b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/outlier/service/AnalyticsOutlierService.java index 1719b736a04d..17b8e9c7d190 100644 --- a/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/outlier/service/AnalyticsOutlierService.java +++ b/dhis-2/dhis-services/dhis-service-analytics/src/main/java/org/hisp/dhis/analytics/outlier/service/AnalyticsOutlierService.java @@ -59,12 +59,15 @@ import lombok.AllArgsConstructor; import lombok.extern.slf4j.Slf4j; import org.hisp.dhis.analytics.cache.OutliersCache; +import org.hisp.dhis.analytics.common.TableInfoReader; import org.hisp.dhis.analytics.outlier.data.Outlier; import org.hisp.dhis.analytics.outlier.data.OutlierRequest; import org.hisp.dhis.common.ExecutionPlan; import org.hisp.dhis.common.Grid; import org.hisp.dhis.common.GridHeader; import org.hisp.dhis.common.IllegalQueryException; +import org.hisp.dhis.feedback.ErrorCode; +import org.hisp.dhis.feedback.ErrorMessage; import org.hisp.dhis.organisationunit.OrganisationUnit; import org.hisp.dhis.organisationunit.OrganisationUnitService; import org.hisp.dhis.system.grid.GridUtils; @@ -86,6 +89,8 @@ public class AnalyticsOutlierService { private final CurrentUserService currentUserService; + private final TableInfoReader tableInfoReader; + /** * Transform the incoming request into api response (json). * @@ -163,6 +168,19 @@ public void getOutliersAsHtmlCss(OutlierRequest request, Writer writer) GridUtils.toHtmlCss(getOutliers(request), writer); } + /** + * The inclusion of outliers is an optional aspect of the analytics table. The outliers API entry + * point can generate a proper response only when outliers are exported along with the analytics + * table. The 'sourceid' column serves as a reliable indicator for successful outliers export. Its + * absence implies that the outliers were not exported. + */ + public void checkAnalyticsTableForOutliers() { + if (tableInfoReader.getInfo("analytics").getColumns().stream() + .noneMatch("sourceid"::equalsIgnoreCase)) { + throw new IllegalQueryException(new ErrorMessage(ErrorCode.E7180)); + } + } + private void setHeaders(Grid grid, OutlierRequest request) { boolean isModifiedZScore = request.getAlgorithm() == MOD_Z_SCORE; diff --git a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/outlierdetection/AnalyticsOutlierDetectionController.java b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/outlierdetection/AnalyticsOutlierDetectionController.java index c69f065782f5..01114462ddd6 100644 --- a/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/outlierdetection/AnalyticsOutlierDetectionController.java +++ b/dhis-2/dhis-web-api/src/main/java/org/hisp/dhis/webapi/controller/outlierdetection/AnalyticsOutlierDetectionController.java @@ -37,7 +37,6 @@ import java.io.IOException; import javax.servlet.http.HttpServletResponse; import lombok.AllArgsConstructor; -import org.hisp.dhis.analytics.common.TableInfoReader; import org.hisp.dhis.analytics.outlier.data.OutlierQueryParams; import org.hisp.dhis.analytics.outlier.data.OutlierQueryParser; import org.hisp.dhis.analytics.outlier.data.OutlierRequest; @@ -45,10 +44,7 @@ import org.hisp.dhis.analytics.outlier.service.AnalyticsOutlierService; import org.hisp.dhis.common.DhisApiVersion; import org.hisp.dhis.common.Grid; -import org.hisp.dhis.common.IllegalQueryException; import org.hisp.dhis.common.OpenApi; -import org.hisp.dhis.feedback.ErrorCode; -import org.hisp.dhis.feedback.ErrorMessage; import org.hisp.dhis.webapi.mvc.annotation.ApiVersion; import org.hisp.dhis.webapi.utils.ContextUtils; import org.springframework.security.access.prepost.PreAuthorize; @@ -72,14 +68,13 @@ public class AnalyticsOutlierDetectionController { private final ContextUtils contextUtils; private final OutlierQueryParser queryParser; private final OutlierRequestValidator validator; - private final TableInfoReader tableInfoReader; @PreAuthorize("hasRole('ALL') or hasRole('F_PERFORM_ANALYTICS_EXPLAIN')") @GetMapping( value = RESOURCE_PATH + "/explain", produces = {APPLICATION_JSON_VALUE, "application/javascript"}) public @ResponseBody Grid getExplainOutliersJson(OutlierQueryParams query) { - checkAnalyticsTableForOutliers(); + outlierService.checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(query, true); return outlierService.getOutliersPerformanceMetrics(request); @@ -87,7 +82,7 @@ public class AnalyticsOutlierDetectionController { @GetMapping(value = RESOURCE_PATH, produces = APPLICATION_JSON_VALUE) public Grid getOutliersJson(OutlierQueryParams queryParams) { - checkAnalyticsTableForOutliers(); + outlierService.checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(queryParams, false); Grid grid = outlierService.getOutliers(request); @@ -102,7 +97,7 @@ public Grid getOutliersJson(OutlierQueryParams queryParams) { @GetMapping(value = RESOURCE_PATH + ".csv") public void getOutliersCsv(OutlierQueryParams queryParams, HttpServletResponse response) throws IOException { - checkAnalyticsTableForOutliers(); + outlierService.checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(queryParams, false); contextUtils.configureResponse(response, CONTENT_TYPE_CSV, NO_CACHE, "outlierdata.csv", true); @@ -112,7 +107,7 @@ public void getOutliersCsv(OutlierQueryParams queryParams, HttpServletResponse r @GetMapping(value = RESOURCE_PATH + ".xml") public void getOutliersXml(OutlierQueryParams queryParams, HttpServletResponse response) throws IOException { - checkAnalyticsTableForOutliers(); + outlierService.checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(queryParams, false); contextUtils.configureResponse(response, CONTENT_TYPE_XML, NO_CACHE); @@ -122,7 +117,7 @@ public void getOutliersXml(OutlierQueryParams queryParams, HttpServletResponse r @GetMapping(value = RESOURCE_PATH + ".xls") public void getOutliersXls(OutlierQueryParams queryParams, HttpServletResponse response) throws IOException { - checkAnalyticsTableForOutliers(); + outlierService.checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(queryParams, false); contextUtils.configureResponse(response, CONTENT_TYPE_EXCEL, NO_CACHE, "outlierdata.xls", true); @@ -132,7 +127,7 @@ public void getOutliersXls(OutlierQueryParams queryParams, HttpServletResponse r @GetMapping(value = RESOURCE_PATH + ".html") public void getOutliersHtml(OutlierQueryParams queryParams, HttpServletResponse response) throws IOException { - checkAnalyticsTableForOutliers(); + outlierService.checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(queryParams, false); contextUtils.configureResponse(response, CONTENT_TYPE_HTML, NO_CACHE); @@ -143,7 +138,7 @@ public void getOutliersHtml(OutlierQueryParams queryParams, HttpServletResponse @GetMapping(value = RESOURCE_PATH + ".html+css") public void getOutliersHtmlCss(OutlierQueryParams queryParams, HttpServletResponse response) throws IOException { - checkAnalyticsTableForOutliers(); + outlierService.checkAnalyticsTableForOutliers(); OutlierRequest request = getFromQuery(queryParams, false); contextUtils.configureResponse(response, CONTENT_TYPE_HTML, NO_CACHE); @@ -156,11 +151,4 @@ private OutlierRequest getFromQuery(OutlierQueryParams queryParams, boolean anal return request; } - - private void checkAnalyticsTableForOutliers() { - if (tableInfoReader.getInfo("analytics").getColumns().stream() - .noneMatch("sourceid"::equalsIgnoreCase)) { - throw new IllegalQueryException(new ErrorMessage(ErrorCode.E7180)); - } - } } From a346b2925e17cb5901062b8c9f44bc07cfdfa621 Mon Sep 17 00:00:00 2001 From: d-bernat Date: Thu, 28 Dec 2023 12:30:40 +0100 Subject: [PATCH 4/4] todo for e2e test --- .../dhis/analytics/NoAnalyticsTablesErrorsScenariosTest.java | 2 ++ 1 file changed, 2 insertions(+) diff --git a/dhis-2/dhis-test-e2e/src/test/java/org/hisp/dhis/analytics/NoAnalyticsTablesErrorsScenariosTest.java b/dhis-2/dhis-test-e2e/src/test/java/org/hisp/dhis/analytics/NoAnalyticsTablesErrorsScenariosTest.java index de8551762fc7..5527299146db 100644 --- a/dhis-2/dhis-test-e2e/src/test/java/org/hisp/dhis/analytics/NoAnalyticsTablesErrorsScenariosTest.java +++ b/dhis-2/dhis-test-e2e/src/test/java/org/hisp/dhis/analytics/NoAnalyticsTablesErrorsScenariosTest.java @@ -49,6 +49,8 @@ * missing analytics tables. For this reason they have to run first (before analytics tables are * created), hence @Order(1). */ + +//TODO do not forget add e2e for DHIS2-16375 @Order(1) @ExtendWith(ConfigurationExtension.class) @Tag("analytics")