Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix: Fix the table population problem when the skip outliers switch is activated/deactivated #16037

Merged
merged 5 commits into from
Dec 28, 2023
Merged

Fix: Fix the table population problem when the skip outliers switch is activated/deactivated #16037

Show file tree
Hide file tree
Changes from 3 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
8f2ea7f
fix: Table population
d-bernat Dec 27, 2023
ccf5927
continuous analytics
d-bernat Dec 28, 2023
7463152
Merge branch 'master' into 2.41-DHIS2-16375
d-bernat Dec 28, 2023
b68507c
post code review impl
d-bernat Dec 28, 2023
a346b29
todo for e2e test
d-bernat Dec 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions dhis-2/dhis-api/src/main/java/org/hisp/dhis/feedback/ErrorCode.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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}`"),
Expand Down
8 changes: 4 additions & 4 deletions ...vice-analytics/src/main/java/org/hisp/dhis/analytics/table/JdbcAnalyticsTableManager.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,9 +163,9 @@ public AnalyticsTableType getAnalyticsTableType() {
public List<AnalyticsTable> 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();
}
Expand Down Expand Up @@ -432,8 +432,8 @@ private String getApprovalJoinClause(Integer year) {
return StringUtils.EMPTY;
}

private List<AnalyticsTableColumn> getDimensionColumns() {
return getDimensionColumns(null, null);
private List<AnalyticsTableColumn> getDimensionColumns(AnalyticsTableUpdateParams params) {
return getDimensionColumns(null, params);
}

private List<AnalyticsTableColumn> getDimensionColumns(
Expand Down
19 changes: 19 additions & 0 deletions ...s/src/main/java/org/hisp/dhis/analytics/table/scheduling/ContinuousAnalyticsTableJob.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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;
Expand All @@ -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);

Expand Down Expand Up @@ -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;
}
}
19 changes: 19 additions & 0 deletions ...org/hisp/dhis/webapi/controller/outlierdetection/AnalyticsOutlierDetectionController.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -37,14 +37,18 @@
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;
import org.hisp.dhis.analytics.outlier.data.OutlierRequestValidator;
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;
Expand All @@ -68,19 +72,22 @@ 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);
}

@GetMapping(value = RESOURCE_PATH, produces = APPLICATION_JSON_VALUE)
public Grid getOutliersJson(OutlierQueryParams queryParams) {
checkAnalyticsTableForOutliers();
OutlierRequest request = getFromQuery(queryParams, false);

Grid grid = outlierService.getOutliers(request);
Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);

Expand All @@ -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);
Expand All @@ -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);

Expand All @@ -144,4 +156,11 @@ private OutlierRequest getFromQuery(OutlierQueryParams queryParams, boolean anal

return request;
}

private void checkAnalyticsTableForOutliers() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method should be moved to the class AnalyticsOutlierService, with a proper Javadoc.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would be nice to have a e2e to test this error. Please see the class NoAnalyticsTablesErrorsScenariosTest. You can add a new test there to simulate this exception and assert the response/error.

Copy link
Contributor Author

@d-bernat d-bernat Dec 28, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

e2e is coming, there are no e2e for outliers at all now

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Okay, if you prefer to add it as part of all others e2e for Outliers is fine. Just please add a TODO in the class NoAnalyticsTablesErrorsScenariosTest, so we do not forget this scenario.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

if (tableInfoReader.getInfo("analytics").getColumns().stream()
.noneMatch("sourceid"::equalsIgnoreCase)) {
throw new IllegalQueryException(new ErrorMessage(ErrorCode.E7180));
}
}
}
Loading