Skip to content

Commit

Permalink
Fix: Fix the table population problem when the skip outliers switch i…
Browse files Browse the repository at this point in the history
…s activated/deactivated (#16037)

* fix: Table population

* continuous analytics

* post code review impl

* todo for e2e test
  • Loading branch information
d-bernat authored Dec 28, 2023
1 parent 19e1ff2 commit 64c54cf
Show file tree
Hide file tree
Showing 6 changed files with 55 additions and 4 deletions.
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
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -86,6 +89,8 @@ public class AnalyticsOutlierService {

private final CurrentUserService currentUserService;

private final TableInfoReader tableInfoReader;

/**
* Transform the incoming request into api response (json).
*
Expand Down Expand Up @@ -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;

Expand Down
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
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;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -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")
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -74,13 +74,15 @@ public class AnalyticsOutlierDetectionController {
value = RESOURCE_PATH + "/explain",
produces = {APPLICATION_JSON_VALUE, "application/javascript"})
public @ResponseBody Grid getExplainOutliersJson(OutlierQueryParams query) {
outlierService.checkAnalyticsTableForOutliers();
OutlierRequest request = getFromQuery(query, true);

return outlierService.getOutliersPerformanceMetrics(request);
}

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

Grid grid = outlierService.getOutliers(request);
Expand All @@ -95,6 +97,7 @@ public Grid getOutliersJson(OutlierQueryParams queryParams) {
@GetMapping(value = RESOURCE_PATH + ".csv")
public void getOutliersCsv(OutlierQueryParams queryParams, HttpServletResponse response)
throws IOException {
outlierService.checkAnalyticsTableForOutliers();
OutlierRequest request = getFromQuery(queryParams, false);
contextUtils.configureResponse(response, CONTENT_TYPE_CSV, NO_CACHE, "outlierdata.csv", true);

Expand All @@ -104,6 +107,7 @@ public void getOutliersCsv(OutlierQueryParams queryParams, HttpServletResponse r
@GetMapping(value = RESOURCE_PATH + ".xml")
public void getOutliersXml(OutlierQueryParams queryParams, HttpServletResponse response)
throws IOException {
outlierService.checkAnalyticsTableForOutliers();
OutlierRequest request = getFromQuery(queryParams, false);
contextUtils.configureResponse(response, CONTENT_TYPE_XML, NO_CACHE);

Expand All @@ -113,6 +117,7 @@ public void getOutliersXml(OutlierQueryParams queryParams, HttpServletResponse r
@GetMapping(value = RESOURCE_PATH + ".xls")
public void getOutliersXls(OutlierQueryParams queryParams, HttpServletResponse response)
throws IOException {
outlierService.checkAnalyticsTableForOutliers();
OutlierRequest request = getFromQuery(queryParams, false);
contextUtils.configureResponse(response, CONTENT_TYPE_EXCEL, NO_CACHE, "outlierdata.xls", true);

Expand All @@ -122,6 +127,7 @@ public void getOutliersXls(OutlierQueryParams queryParams, HttpServletResponse r
@GetMapping(value = RESOURCE_PATH + ".html")
public void getOutliersHtml(OutlierQueryParams queryParams, HttpServletResponse response)
throws IOException {
outlierService.checkAnalyticsTableForOutliers();
OutlierRequest request = getFromQuery(queryParams, false);

contextUtils.configureResponse(response, CONTENT_TYPE_HTML, NO_CACHE);
Expand All @@ -132,6 +138,7 @@ public void getOutliersHtml(OutlierQueryParams queryParams, HttpServletResponse
@GetMapping(value = RESOURCE_PATH + ".html+css")
public void getOutliersHtmlCss(OutlierQueryParams queryParams, HttpServletResponse response)
throws IOException {
outlierService.checkAnalyticsTableForOutliers();
OutlierRequest request = getFromQuery(queryParams, false);
contextUtils.configureResponse(response, CONTENT_TYPE_HTML, NO_CACHE);

Expand Down

0 comments on commit 64c54cf

Please sign in to comment.