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.

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

feat: Add filtering capabilities in events change log [DHIS2-18012] #19268

Merged
merged 5 commits into from
Nov 25, 2024
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -69,8 +69,7 @@ public Page<EventChangeLog> getEventChangeLog(
// check existence and access
eventService.getEvent(event);

return hibernateEventChangeLogStore.getEventChangeLogs(
event, operationParams.getOrder(), pageParams);
return hibernateEventChangeLogStore.getEventChangeLogs(event, operationParams, pageParams);
}

@Transactional
Expand Down Expand Up @@ -165,6 +164,11 @@ public Set<String> getOrderableFields() {
return hibernateEventChangeLogStore.getOrderableFields();
}

@Override
public Set<String> getFilterableFields() {
return hibernateEventChangeLogStore.getFilterableFields();
}

private <T> void logIfChanged(
String propertyName,
Function<Event, T> valueExtractor,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,10 +27,12 @@
*/
package org.hisp.dhis.tracker.export.event;

import java.util.Map;
import lombok.AccessLevel;
import lombok.AllArgsConstructor;
import lombok.Builder;
import lombok.Getter;
import org.hisp.dhis.common.QueryFilter;
import org.hisp.dhis.common.SortDirection;
import org.hisp.dhis.tracker.export.Order;

Expand All @@ -40,20 +42,30 @@
public class EventChangeLogOperationParams {

private Order order;
private Map.Entry<String, QueryFilter> filterEntry;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it makes sense to make this field a Pair?
Right now I don't understand why we have a Map.Entry here but in the builder we have a filterMap method.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I just didn't want to use an external library, but I agree conceptually probably makes more sense to use Pair instead of Map.Entry.


public static class EventChangeLogOperationParamsBuilder {

// Do not remove this unused method. This hides the order field from the builder which Lombok
// does not support. The repeated order field and private order method prevent access to order
// via the builder.
// Order should be added via the orderBy builder methods.
// Do not remove these unused methods. They hide the order and filter fields from the builder
// which Lombok
// does not support.
// They should be added via their respective orderBy and filterBy builder methods.
private EventChangeLogOperationParamsBuilder order(Order order) {
return this;
}

private EventChangeLogOperationParamsBuilder filterMap(Map<String, QueryFilter> filterMap) {
Fixed Show fixed Hide fixed
return this;
}

public EventChangeLogOperationParamsBuilder orderBy(String field, SortDirection direction) {
this.order = new Order(field, direction);
return this;
}

public EventChangeLogOperationParamsBuilder filterBy(String field, QueryFilter filter) {
this.filterEntry = Map.entry(field, filter);
return this;
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -85,9 +85,17 @@ void addPropertyChangeLog(

/**
* Fields the {@link #getEventChangeLog(UID, EventChangeLogOperationParams, PageParams)} can order
* event change logs by. Ordering by fields other than these is considered a programmer error.
* event change logs by. Ordering by fields other than these, is considered a programmer error.
* Validation of user provided field names should occur before calling {@link
* #getEventChangeLog(UID, EventChangeLogOperationParams, PageParams)}.
*/
Set<String> getOrderableFields();

/**
* Fields the {@link #getEventChangeLog(UID, EventChangeLogOperationParams, PageParams)} can
* filter event change logs by. Filtering by fields other than these, is considered a programmer
* error. Validation of user provided field names should occur before calling {@link
* #getEventChangeLog(UID, EventChangeLogOperationParams, PageParams)}.
*/
Set<String> getFilterableFields();
}
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,9 @@
import java.util.Map;
import java.util.Set;
import javax.annotation.Nonnull;
import javax.annotation.Nullable;
import org.hibernate.Session;
import org.hisp.dhis.changelog.ChangeLogType;
import org.hisp.dhis.common.QueryFilter;
import org.hisp.dhis.common.SortDirection;
import org.hisp.dhis.common.UID;
import org.hisp.dhis.dataelement.DataElement;
Expand Down Expand Up @@ -73,6 +73,12 @@
entry("dataElement", COLUMN_CHANGELOG_DATA_ELEMENT),
entry("property", COLUMN_CHANGELOG_PROPERTY));

private static final Map<String, String> FILTERABLE_FIELDS =
muilpp marked this conversation as resolved.
Show resolved Hide resolved
Map.ofEntries(
entry("username", COLUMN_CHANGELOG_USER),
entry("dataElement", COLUMN_CHANGELOG_DATA_ELEMENT),
entry("property", COLUMN_CHANGELOG_PROPERTY));

private final EntityManager entityManager;

public HibernateEventChangeLogStore(EntityManager entityManager) {
Expand All @@ -84,11 +90,14 @@
}

public Page<EventChangeLog> getEventChangeLogs(
@Nonnull UID event, @Nullable Order order, @Nonnull PageParams pageParams) {
@Nonnull UID event,
@Nonnull EventChangeLogOperationParams operationParams,
@Nonnull PageParams pageParams) {

Map.Entry<String, QueryFilter> filterEntry = operationParams.getFilterEntry();

String hql =
String.format(
"""
"""
select ecl.event,
ecl.dataElement,
ecl.eventProperty,
Expand All @@ -105,15 +114,23 @@
left join ecl.dataElement d
left join ecl.createdBy u
where e.uid = :eventUid
order by %s
"""
.formatted(sortExpressions(order)));
""";

if (filterEntry != null) {
hql += String.format(" and %s = :filterValue ", FILTERABLE_FIELDS.get(filterEntry.getKey()));
}

hql += String.format("order by %s".formatted(sortExpressions(operationParams.getOrder())));
Dismissed Show dismissed Hide dismissed

Query query = entityManager.createQuery(hql);
query.setParameter("eventUid", event.getValue());
query.setFirstResult((pageParams.getPage() - 1) * pageParams.getPageSize());
query.setMaxResults(pageParams.getPageSize() + 1);

if (filterEntry != null) {
query.setParameter("filterValue", filterEntry.getValue().getFilter());
}

List<Object[]> results = query.getResultList();
List<EventChangeLog> eventChangeLogs =
results.stream()
Expand Down Expand Up @@ -189,4 +206,8 @@
public Set<String> getOrderableFields() {
return ORDERABLE_FIELDS.keySet();
}

public Set<String> getFilterableFields() {
return FILTERABLE_FIELDS.keySet();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
*/
package org.hisp.dhis.tracker.export.event;

import static org.hisp.dhis.test.utils.Assertions.assertContainsOnly;
import static org.hisp.dhis.tracker.Assertions.assertNoErrors;
import static org.junit.jupiter.api.Assertions.assertAll;
import static org.junit.jupiter.api.Assertions.assertEquals;
Expand All @@ -35,8 +36,12 @@
import java.io.IOException;
import java.time.Instant;
import java.util.List;
import java.util.Set;
import java.util.stream.Collectors;
import java.util.stream.Stream;
import org.hisp.dhis.common.IdentifiableObjectManager;
import org.hisp.dhis.common.QueryFilter;
import org.hisp.dhis.common.QueryOperator;
import org.hisp.dhis.common.SortDirection;
import org.hisp.dhis.common.UID;
import org.hisp.dhis.feedback.ForbiddenException;
Expand All @@ -59,7 +64,7 @@
import org.junit.jupiter.params.provider.MethodSource;
import org.springframework.beans.factory.annotation.Autowired;

class OrderEventChangeLogTest extends TrackerTest {
class OrderAndFilterEventChangeLogTest extends TrackerTest {

@Autowired private EventChangeLogService eventChangeLogService;

Expand Down Expand Up @@ -260,6 +265,67 @@ void shouldSortChangeLogsWhenOrderingByPropertyDesc()
() -> assertPropertyCreate("geometry", "(-11.419700, 8.103900)", changeLogs.get(4)));
}

@Test
void shouldFilterChangeLogsWhenFilteringByUser() throws ForbiddenException, NotFoundException {
EventChangeLogOperationParams params =
EventChangeLogOperationParams.builder()
.filterBy("username", new QueryFilter(QueryOperator.EQ, importUser.getUsername()))
.build();

Page<EventChangeLog> changeLogs =
eventChangeLogService.getEventChangeLog(UID.of("QRYjLTiJTrA"), params, defaultPageParams);

Set<String> changeLogUsers =
changeLogs.getItems().stream()
.map(cl -> cl.getCreatedBy().getUsername())
.collect(Collectors.toSet());
assertContainsOnly(List.of(importUser.getUsername()), changeLogUsers);
}

@Test
void shouldFilterChangeLogsWhenFilteringByDataElement()
throws ForbiddenException, NotFoundException {
Event event = getEvent("kWjSezkXHVp");
String dataElement = getFirstDataElement(event);
EventChangeLogOperationParams params =
EventChangeLogOperationParams.builder()
.filterBy("dataElement", new QueryFilter(QueryOperator.EQ, dataElement))
.build();

Page<EventChangeLog> changeLogs =
eventChangeLogService.getEventChangeLog(UID.of(event.getUid()), params, defaultPageParams);

Set<String> changeLogDataElements =
changeLogs.getItems().stream()
.map(cl -> cl.getDataElement().getUid())
.collect(Collectors.toSet());
assertContainsOnly(List.of(dataElement), changeLogDataElements);
}

private Stream<Arguments> provideEventProperties() {
return Stream.of(
Arguments.of("occurredAt"), Arguments.of("scheduledAt"), Arguments.of("geometry"));
}

@ParameterizedTest
@MethodSource("provideEventProperties")
void shouldFilterChangeLogsWhenFilteringByOccurredAt(String filterValue)
throws ForbiddenException, NotFoundException {
EventChangeLogOperationParams params =
EventChangeLogOperationParams.builder()
.filterBy("property", new QueryFilter(QueryOperator.EQ, filterValue))
.build();

Page<EventChangeLog> changeLogs =
eventChangeLogService.getEventChangeLog(UID.of("QRYjLTiJTrA"), params, defaultPageParams);

Set<String> changeLogOccurredAtProperties =
changeLogs.getItems().stream()
.map(EventChangeLog::getEventProperty)
.collect(Collectors.toSet());
assertContainsOnly(List.of(filterValue), changeLogOccurredAtProperties);
}

private void updateDataValue(String event, String dataElementUid, String newValue) {
trackerObjects.getEvents().stream()
.filter(e -> e.getEvent().getValue().equalsIgnoreCase(event))
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -223,6 +223,24 @@ void shouldGetEventChangeLogInAscOrder() {
"scheduledAt", "2023-01-10 00:00:00.000", eventPropertyChangeLogs));
}

@Test
void shouldGetEventChangeLogsWhenFilteringByProperty() {
JsonList<JsonEventChangeLog> changeLogs =
GET("/tracker/events/{id}/changeLogs?filter=property:eq:occurredAt", event.getUid())
.content(HttpStatus.OK)
.getList("changeLogs", JsonEventChangeLog.class);
List<JsonEventChangeLog> eventPropertyChangeLogs =
changeLogs.stream()
.filter(log -> log.getChange().getEventProperty().getProperty() != null)
.toList();

assertAll(
() -> assertHasSize(1, eventPropertyChangeLogs),
() ->
assertPropertyCreateExists(
"occurredAt", "2023-01-10 00:00:00.000", eventPropertyChangeLogs));
}

@Test
void shouldGetChangeLogPagerWithNextElementWhenMultipleElementsImportedAndFirstPageRequested() {
JsonPage changeLogs =
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,4 +51,6 @@ public class ChangeLogRequestParams implements FieldsRequestParam {
private List<FieldPath> fields = FieldFilterParser.parse(DEFAULT_FIELDS_PARAM);

private List<OrderCriteria> order = new ArrayList<>();

private String filter;
}
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ private RequestParamsValidator() {

private static final char ESCAPE = '/';

private static final String SUPPORTED_CHANGELOG_FILTER_OPERATOR = "eq";

/**
* Negative lookahead to avoid wrong split of comma-separated list of filters when one or more
* filter value contain comma. It skips comma escaped by slash
Expand Down Expand Up @@ -271,6 +273,39 @@ public static void validateOrderParams(List<OrderCriteria> order, Set<String> su
validateOrderParamsContainNoDuplicates(order, errorSuffix);
}

/**
* Validate the {@code filter} request parameter in change log tracker exporters. Allowed filter
* values are {@code supportedFieldNames}. Only one field name at a time can be specified. If the
* endpoint supports UIDs use {@link #parseFilters(String)}.
*/
public static void validateFilter(String filter, Set<String> supportedFieldNames)
throws BadRequestException {
if (filter == null) {
return;
}

String[] split = filter.split(":");

if (split.length != 3) {
throw new BadRequestException(
"Invalid filter => %s. Expected format is [field]:eq:[value]." + filter);
muilpp marked this conversation as resolved.
Show resolved Hide resolved
}

if (!supportedFieldNames.contains(split[0])) {
throw new BadRequestException(
String.format(
"Invalid filter field. Supported fields are '%s'. Only one of them can be specified at a time",
String.join(", ", supportedFieldNames.stream().sorted().toList())));
}

if (!split[1].equalsIgnoreCase(SUPPORTED_CHANGELOG_FILTER_OPERATOR)) {
throw new BadRequestException(
String.format(
"Invalid filter operator. The only supported operator is '%s'.",
SUPPORTED_CHANGELOG_FILTER_OPERATOR));
}
}

/**
* Parse given {@code input} string representing a filter for an object referenced by a UID like a
* tracked entity attribute or data element. Refer to {@link #parseSanitizedFilters(Map, String)}}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -27,11 +27,14 @@
*/
package org.hisp.dhis.webapi.controller.tracker.export.event;

import static org.hisp.dhis.webapi.controller.tracker.export.RequestParamsValidator.validateFilter;
import static org.hisp.dhis.webapi.controller.tracker.export.RequestParamsValidator.validateOrderParams;
import static org.hisp.dhis.webapi.controller.tracker.export.RequestParamsValidator.validatePaginationBounds;

import java.util.List;
import java.util.Set;
import org.hisp.dhis.common.QueryFilter;
import org.hisp.dhis.common.QueryOperator;
import org.hisp.dhis.feedback.BadRequestException;
import org.hisp.dhis.tracker.export.event.EventChangeLogOperationParams;
import org.hisp.dhis.tracker.export.event.EventChangeLogOperationParams.EventChangeLogOperationParamsBuilder;
Expand All @@ -51,13 +54,17 @@ private ChangeLogRequestParamsMapper() {
* done in {@link EventMapper} is not necessary for change logs.
*/
static EventChangeLogOperationParams map(
Set<String> orderableFields, ChangeLogRequestParams requestParams)
Set<String> orderableFields,
Set<String> filterableFields,
ChangeLogRequestParams requestParams)
throws BadRequestException {
validatePaginationBounds(requestParams.getPage(), requestParams.getPageSize());
validateOrderParams(requestParams.getOrder(), orderableFields);
validateFilter(requestParams.getFilter(), filterableFields);

EventChangeLogOperationParamsBuilder builder = EventChangeLogOperationParams.builder();
mapOrderParam(builder, requestParams.getOrder());
mapFilterParam(builder, requestParams.getFilter());
return builder.build();
}

Expand All @@ -69,4 +76,11 @@ private static void mapOrderParam(

orders.forEach(order -> builder.orderBy(order.getField(), order.getDirection()));
}

private static void mapFilterParam(EventChangeLogOperationParamsBuilder builder, String filter) {
if (filter != null) {
String[] split = filter.split(":");
builder.filterBy(split[0], new QueryFilter(QueryOperator.EQ, split[2]));
}
}
}
Loading
Loading