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

FM2-653 #556

Merged
merged 12 commits into from
Jan 24, 2025
Merged

FM2-653 #556

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
12 commits
Select commit Hold shift + click to select a range
5b94f18
FM2-653: Add support for searching for Locations by ancestor
mogoodrich Jan 9, 2025
c46ba07
FM2-653: Add support for searching for Locations by ancestor
mogoodrich Jan 9, 2025
e5b00bd
FM2-653: Add support for searching for Locations by ancestor
mogoodrich Jan 9, 2025
9319b83
FM2-653: Add support for searching for Locations by ancestor
mogoodrich Jan 9, 2025
90ec5dd
FM2-653: Add support for searching for Locations by ancestor
mogoodrich Jan 9, 2025
29a5cb0
FM2-653: Add support for searching for Locations by ancestor
mogoodrich Jan 17, 2025
47d35d7
FM2-653: Add support for searching for Locations by ancestor
mogoodrich Jan 17, 2025
19723f0
FM2-653: Add support for searching for Locations by ancestor
mogoodrich Jan 17, 2025
b1efbfb
FM2-653: Add support for searching for Locations by ancestor
mogoodrich Jan 24, 2025
be996fe
FM2-653: Add support for searching for Locations by ancestor
mogoodrich Jan 24, 2025
370d392
Merge branch 'master' into FM2-653
mogoodrich Jan 24, 2025
aafa52a
FM2-653: Add support for searching for Locations by ancestor
mogoodrich Jan 24, 2025
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
2 changes: 2 additions & 0 deletions api/src/main/java/org/openmrs/module/fhir2/FhirConstants.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -245,6 +245,8 @@ private FhirConstants() {

public static final String LOCATION_REFERENCE_SEARCH_HANDLER = "location.reference.search.handler";

public static final String LOCATION_ANCESTOR_SEARCH_HANDLER = "location.ancestor.search.handler";

public static final String DOSAGE_FORM_SEARCH_HANDLER = "dosage.form.search.handler";

public static final String INGREDIENT_SEARCH_HANDLER = "ingredient.search.handler";
Expand Down
28 changes: 20 additions & 8 deletions api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/BaseDao.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -558,8 +558,11 @@ protected Optional<Criterion> handleGender(@Nonnull String propertyName, TokenAn
});
}

protected Optional<Criterion> handleLocationReference(@Nonnull String locationAlias,
ReferenceAndListParam locationReference) {
protected Optional<Criterion> handleLocationReference(ReferenceAndListParam locationReference) {
return handleLocationReference(null, locationReference);
}

protected Optional<Criterion> handleLocationReference(String locationAlias, ReferenceAndListParam locationReference) {
if (locationReference == null) {
return Optional.empty();
}
Expand All @@ -568,18 +571,27 @@ protected Optional<Criterion> handleLocationReference(@Nonnull String locationAl
if (token.getChain() != null) {
switch (token.getChain()) {
case Location.SP_NAME:
return propertyLike(String.format("%s.name", locationAlias), token.getValue());
return propertyLike(locationAlias != null ? String.format("%s.name", locationAlias) : "name",
token.getValue());
case Location.SP_ADDRESS_CITY:
return propertyLike(String.format("%s.cityVillage", locationAlias), token.getValue());
return propertyLike(
locationAlias != null ? String.format("%s.cityVillage", locationAlias) : "cityVillage",
token.getValue());
case Location.SP_ADDRESS_STATE:
return propertyLike(String.format("%s.stateProvince", locationAlias), token.getValue());
return propertyLike(
locationAlias != null ? String.format("%s.stateProvince", locationAlias) : "stateProvince",
token.getValue());
case Location.SP_ADDRESS_POSTALCODE:
return propertyLike(String.format("%s.postalCode", locationAlias), token.getValue());
return propertyLike(
locationAlias != null ? String.format("%s.postalCode", locationAlias) : "postalCode",
token.getValue());
case Location.SP_ADDRESS_COUNTRY:
return propertyLike(String.format("%s.country", locationAlias), token.getValue());
return propertyLike(locationAlias != null ? String.format("%s.country", locationAlias) : "country",
token.getValue());
}
} else {
return Optional.of(eq(String.format("%s.uuid", locationAlias), token.getValue()));
return Optional
.of(eq(locationAlias != null ? String.format("%s.uuid", locationAlias) : "uuid", token.getValue()));
}

return Optional.empty();
Expand Down
28 changes: 28 additions & 0 deletions api/src/main/java/org/openmrs/module/fhir2/api/dao/impl/FhirLocationDaoImpl.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,9 +10,11 @@
package org.openmrs.module.fhir2.api.dao.impl;

import static org.hibernate.criterion.Restrictions.eq;
import static org.hibernate.criterion.Restrictions.or;

import javax.annotation.Nonnull;

import java.util.ArrayList;
import java.util.List;
import java.util.Optional;

Expand All @@ -22,6 +24,7 @@
import lombok.AccessLevel;
import lombok.Setter;
import org.hibernate.Criteria;
import org.hibernate.criterion.Criterion;
import org.hibernate.sql.JoinType;
import org.openmrs.Location;
import org.openmrs.LocationAttribute;
Expand All @@ -38,6 +41,8 @@
@Setter(AccessLevel.PACKAGE)
public class FhirLocationDaoImpl extends BaseFhirDao<Location> implements FhirLocationDao {

private static final int SUPPORTED_LOCATION_HIERARCHY_SEARCH_DEPTH = 9;
Copy link
Member Author

Choose a reason for hiding this comment

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

Basically, I use a succession of joins to go back up the ancestor tree. A bit annoying/hacky, but from a practical matter, seems unlikely there would ever be a hierarchy of more than 9 levels. We could make this a configurable parameter if we wanted.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe we turn this into a GP? That way an implementation can adjust it to the actual depth of their location hierarchy.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough, I will do that (and since it's configurable, set a more normal default of like 5)


@Autowired
LocationService locationService;

Expand All @@ -64,6 +69,10 @@ protected void setupSearchParams(Criteria criteria, SearchParameterMap theParams
entry.getValue()
.forEach(param -> handleParentLocation(criteria, (ReferenceAndListParam) param.getParam()));
break;
case FhirConstants.LOCATION_ANCESTOR_SEARCH_HANDLER:
entry.getValue()
.forEach(param -> handleAncestorLocation(criteria, (ReferenceAndListParam) param.getParam()));
break;
case FhirConstants.TAG_SEARCH_HANDLER:
entry.getValue().forEach(param -> handleTag(criteria, (TokenAndListParam) param.getParam()));
break;
Expand All @@ -84,6 +93,25 @@ public List<LocationAttribute> getActiveAttributesByLocationAndAttributeTypeUuid
.list();
}

private void handleAncestorLocation(Criteria criteria, ReferenceAndListParam ancestor) {

List<Criterion> elementOrAncestorEqualsReferenceCriteria = new ArrayList<>();
Optional<Criterion> elementEqualsReferenceCriterion = handleLocationReference(ancestor); // note: "partof:below" is inclusive of the element itself
elementEqualsReferenceCriterion.ifPresent(elementOrAncestorEqualsReferenceCriteria::add);

// we need to add a join to the parentLocation for each level of hierarchy we want to search, and add a "equals" criterion for each level
int depth = 1;
while (depth <= SUPPORTED_LOCATION_HIERARCHY_SEARCH_DEPTH) {
Optional<Criterion> ancestorEqualsReferenceCriterion = handleLocationReference("ancestor" + depth, ancestor);
ancestorEqualsReferenceCriterion.ifPresent(elementOrAncestorEqualsReferenceCriteria::add);
criteria.createAlias(depth == 1 ? "parentLocation" : "ancestor" + (depth - 1) + ".parentLocation",
"ancestor" + depth, JoinType.LEFT_OUTER_JOIN);
depth++;
}

criteria.add(or(elementOrAncestorEqualsReferenceCriteria.toArray(new Criterion[0])));
}

private void handleName(Criteria criteria, StringAndListParam namePattern) {
handleAndListParam(namePattern, (name) -> propertyLike("name", name)).ifPresent(criteria::add);
}
Expand Down
10 changes: 7 additions & 3 deletions api/src/main/java/org/openmrs/module/fhir2/api/search/param/LocationSearchParams.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,11 +42,13 @@ public class LocationSearchParams extends BaseResourceSearchParams {

private ReferenceAndListParam parent;

private ReferenceAndListParam ancestor;

@Builder
public LocationSearchParams(StringAndListParam name, StringAndListParam city, StringAndListParam country,
StringAndListParam postalCode, StringAndListParam state, TokenAndListParam tag, ReferenceAndListParam parent,
TokenAndListParam id, DateRangeParam lastUpdated, SortSpec sort, HashSet<Include> includes,
HashSet<Include> revIncludes) {
ReferenceAndListParam ancestor, TokenAndListParam id, DateRangeParam lastUpdated, SortSpec sort,
HashSet<Include> includes, HashSet<Include> revIncludes) {

super(id, lastUpdated, sort, includes, revIncludes);

Expand All @@ -57,6 +59,7 @@ public LocationSearchParams(StringAndListParam name, StringAndListParam city, St
this.state = state;
this.tag = tag;
this.parent = parent;
this.ancestor = ancestor;
}

@Override
Expand All @@ -67,7 +70,8 @@ public SearchParameterMap toSearchParameterMap() {
.addParameter(FhirConstants.POSTALCODE_SEARCH_HANDLER, getPostalCode())
.addParameter(FhirConstants.STATE_SEARCH_HANDLER, getState())
.addParameter(FhirConstants.TAG_SEARCH_HANDLER, getTag())
.addParameter(FhirConstants.LOCATION_REFERENCE_SEARCH_HANDLER, getParent());
.addParameter(FhirConstants.LOCATION_REFERENCE_SEARCH_HANDLER, getParent())
.addParameter(FhirConstants.LOCATION_ANCESTOR_SEARCH_HANDLER, getAncestor());
}

}
5 changes: 4 additions & 1 deletion api/src/main/java/org/openmrs/module/fhir2/providers/r3/LocationFhirResourceProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ public IBundleProvider searchLocations(@OptionalParam(name = Location.SP_NAME) S
@OptionalParam(name = Location.SP_PARTOF, chainWhitelist = { "", Location.SP_NAME, Location.SP_ADDRESS_CITY,
Location.SP_ADDRESS_STATE, Location.SP_ADDRESS_COUNTRY,
Location.SP_ADDRESS_POSTALCODE }, targetTypes = Location.class) ReferenceAndListParam parent,
@OptionalParam(name = "below", chainWhitelist = { "", Location.SP_NAME, Location.SP_ADDRESS_CITY,
Copy link
Member Author

Choose a reason for hiding this comment

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

So it sounds like @ibacher this extra parameter should not be here, and this new functionality should just be part of the "SP_PARTOF" parameter above? I was having a problem with that, see my follow on comment and screenshot in the test case below.

Copy link
Member Author

Choose a reason for hiding this comment

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

The tl;dr is that if I change this to name="partof:below" any parameter passed in that way gets assigned to both the ReferenceAndListParam ancestor and ReferenceAndListParam parent.

And if I remove the "below" parameter, I'm not quite sure how to parse the "parent" ReferenceAndListParam to tell if this should be a standard "partof" vs a "partof:below"

Copy link
Member

Choose a reason for hiding this comment

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

this extra parameter should not be here, and this new functionality should just be part of the "SP_PARTOF" parameter above?

Ideally, yes.

And if I remove the "below" parameter, I'm not quite sure how to parse the "parent" ReferenceAndListParam to tell if this should be a standard "partof" vs a "partof:below"

That's very problematic. The problem is that partof=123abc&partof:below=456def is technically valid and I'm not quite sure what HAPI would do with that...

Location.SP_ADDRESS_STATE, Location.SP_ADDRESS_COUNTRY,
Location.SP_ADDRESS_POSTALCODE }, targetTypes = Location.class) ReferenceAndListParam ancestor,
@OptionalParam(name = Location.SP_RES_ID) TokenAndListParam id,
@OptionalParam(name = "_lastUpdated") DateRangeParam lastUpdated,
@IncludeParam(allow = { "Location:" + Location.SP_PARTOF }) HashSet<Include> includes,
Expand All @@ -129,6 +132,6 @@ public IBundleProvider searchLocations(@OptionalParam(name = Location.SP_NAME) S
}

return new SearchQueryBundleProviderR3Wrapper(locationService.searchForLocations(new LocationSearchParams(name, city,
country, postalCode, state, tag, parent, id, lastUpdated, sort, includes, revIncludes)));
country, postalCode, state, tag, parent, ancestor, id, lastUpdated, sort, includes, revIncludes)));
}
}
5 changes: 4 additions & 1 deletion api/src/main/java/org/openmrs/module/fhir2/providers/r4/LocationFhirResourceProvider.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,9 @@ public IBundleProvider searchLocations(@OptionalParam(name = Location.SP_NAME) S
@OptionalParam(name = Location.SP_PARTOF, chainWhitelist = { "", Location.SP_NAME, Location.SP_ADDRESS_CITY,
Location.SP_ADDRESS_STATE, Location.SP_ADDRESS_COUNTRY,
Location.SP_ADDRESS_POSTALCODE }, targetTypes = Location.class) ReferenceAndListParam parent,
@OptionalParam(name = "below", chainWhitelist = { "", Location.SP_NAME, Location.SP_ADDRESS_CITY,
Location.SP_ADDRESS_STATE, Location.SP_ADDRESS_COUNTRY,
Location.SP_ADDRESS_POSTALCODE }, targetTypes = Location.class) ReferenceAndListParam ancestor,
@OptionalParam(name = Location.SP_RES_ID) TokenAndListParam id,
@OptionalParam(name = "_lastUpdated") DateRangeParam lastUpdated,
@IncludeParam(allow = { "Location:" + Location.SP_PARTOF }) HashSet<Include> includes,
Expand All @@ -136,6 +139,6 @@ public IBundleProvider searchLocations(@OptionalParam(name = Location.SP_NAME) S
}

return (fhirLocationService.searchForLocations(new LocationSearchParams(name, city, country, postalCode, state, tag,
parent, id, lastUpdated, sort, includes, revIncludes)));
parent, ancestor, id, lastUpdated, sort, includes, revIncludes)));
}
}
2 changes: 1 addition & 1 deletion api/src/test/java/org/openmrs/module/fhir2/api/impl/FhirLocationServiceImplTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ public void searchForLocations_shouldReturnLocationsByParameters() {
when(locationDao.getSearchResults(any())).thenReturn(locations);

IBundleProvider results = fhirLocationService.searchForLocations(
new LocationSearchParams(null, null, null, null, null, null, null, null, null, null, null, null));
new LocationSearchParams(null, null, null, null, null, null, null, null, null, null, null, null, null));

assertThat(results, notNullValue());

Expand Down
159 changes: 159 additions & 0 deletions api/src/test/java/org/openmrs/module/fhir2/api/search/LocationSearchQueryTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -98,6 +98,18 @@ public class LocationSearchQueryTest extends BaseModuleContextSensitiveTest {

private static final String LOCATION_PARENT_NAME = "Test location 5";

private static final String LOCATION_ANCESTOR_TEST_UUID = "76cd2d30-2411-44ef-84ea-8b7473256a6a";

private static final String LOCATION_ANCESTOR_TEST_NAME = "Test location 4";

private static final String LOCATION_ANCESTOR_TEST_CITY = "Scituate";

private static final String LOCATION_ANCESTOR_TEST_COUNTRY = "USA";

private static final String LOCATION_ANCESTOR_TEST_POSTAL_CODE = "02066";

private static final String LOCATION_ANCESTOR_TEST_STATE = "MA";

private static final String DATE_CREATED = "2005-01-01";

private static final String DATE_CHANGED = "2010-03-31";
Expand Down Expand Up @@ -752,6 +764,153 @@ public void searchForLocations_shouldSortLocationsByCityAsRequested() {
}
}

@Test
public void searchForLocations_shouldReturnCorrectLocationsByAncestorUUID() {

ReferenceAndListParam ancestorLocation = new ReferenceAndListParam().addAnd(
new ReferenceOrListParam().add(new ReferenceParam().setValue(LOCATION_ANCESTOR_TEST_UUID).setChain(null)));

SearchParameterMap theParams = new SearchParameterMap().addParameter(FhirConstants.LOCATION_ANCESTOR_SEARCH_HANDLER,
ancestorLocation);

IBundleProvider locations = search(theParams);

assertThat(locations, notNullValue());
assertThat(locations.size(), equalTo(5));

List<Location> resultList = get(locations);

assertThat(resultList, hasSize(equalTo(5)));
List<String> locationNames = resultList.stream().map(Location::getName).collect(Collectors.toList());
assertThat(locationNames, hasItem("Test location 4")); // element search for ("below" search should be inclusive)
assertThat(locationNames, hasItem("Test location 6")); // child element
assertThat(locationNames, hasItem("Test location 8")); // child element
assertThat(locationNames, hasItem("Test location 11")); // grandchild element
assertThat(locationNames, hasItem("Test location 12")); // great grandchild element
}

@Test
public void searchForLocations_shouldReturnCorrectLocationsByAncestorName() {

ReferenceAndListParam ancestorLocation = new ReferenceAndListParam().addAnd(
new ReferenceOrListParam().add(new ReferenceParam().setValue(LOCATION_ANCESTOR_TEST_NAME).setChain("name")));

SearchParameterMap theParams = new SearchParameterMap().addParameter(FhirConstants.LOCATION_ANCESTOR_SEARCH_HANDLER,
ancestorLocation);

IBundleProvider locations = search(theParams);

assertThat(locations, notNullValue());
assertThat(locations.size(), equalTo(5));

List<Location> resultList = get(locations);

assertThat(resultList, hasSize(equalTo(5)));
List<String> locationNames = resultList.stream().map(Location::getName).collect(Collectors.toList());
assertThat(locationNames, hasItem("Test location 4")); // element search for ("below" search should be inclusive)
assertThat(locationNames, hasItem("Test location 6")); // child element
assertThat(locationNames, hasItem("Test location 8")); // child element
assertThat(locationNames, hasItem("Test location 11")); // grandchild element
assertThat(locationNames, hasItem("Test location 12")); // great grandchild element
}

@Test
public void searchForLocations_shouldReturnCorrectLocationsByAncestorCity() {

ReferenceAndListParam ancestorLocation = new ReferenceAndListParam().addAnd(new ReferenceOrListParam()
.add(new ReferenceParam().setValue(LOCATION_ANCESTOR_TEST_CITY).setChain("address-city")));

SearchParameterMap theParams = new SearchParameterMap().addParameter(FhirConstants.LOCATION_ANCESTOR_SEARCH_HANDLER,
ancestorLocation);

IBundleProvider locations = search(theParams);

assertThat(locations, notNullValue());
assertThat(locations.size(), equalTo(5));

List<Location> resultList = get(locations);

assertThat(resultList, hasSize(equalTo(5)));
List<String> locationNames = resultList.stream().map(Location::getName).collect(Collectors.toList());
assertThat(locationNames, hasItem("Test location 4")); // element search for ("below" search should be inclusive)
assertThat(locationNames, hasItem("Test location 6")); // child element
assertThat(locationNames, hasItem("Test location 8")); // child element
assertThat(locationNames, hasItem("Test location 11")); // grandchild element
assertThat(locationNames, hasItem("Test location 12")); // great grandchild element
}

public void searchForLocations_shouldReturnCorrectLocationsByAncestorState() {

ReferenceAndListParam ancestorLocation = new ReferenceAndListParam().addAnd(new ReferenceOrListParam()
.add(new ReferenceParam().setValue(LOCATION_ANCESTOR_TEST_STATE).setChain("address-state")));

SearchParameterMap theParams = new SearchParameterMap().addParameter(FhirConstants.LOCATION_ANCESTOR_SEARCH_HANDLER,
ancestorLocation);

IBundleProvider locations = search(theParams);

assertThat(locations, notNullValue());
assertThat(locations.size(), equalTo(5));

List<Location> resultList = get(locations);

assertThat(resultList, hasSize(equalTo(5)));
List<String> locationNames = resultList.stream().map(Location::getName).collect(Collectors.toList());
assertThat(locationNames, hasItem("Test location 4")); // element search for ("below" search should be inclusive)
assertThat(locationNames, hasItem("Test location 6")); // child element
assertThat(locationNames, hasItem("Test location 8")); // child element
assertThat(locationNames, hasItem("Test location 11")); // grandchild element
assertThat(locationNames, hasItem("Test location 12")); // great grandchild element
}

public void searchForLocations_shouldReturnCorrectLocationsByAncestorCountry() {

ReferenceAndListParam ancestorLocation = new ReferenceAndListParam().addAnd(new ReferenceOrListParam()
.add(new ReferenceParam().setValue(LOCATION_ANCESTOR_TEST_COUNTRY).setChain("address-country")));

SearchParameterMap theParams = new SearchParameterMap().addParameter(FhirConstants.LOCATION_ANCESTOR_SEARCH_HANDLER,
ancestorLocation);

IBundleProvider locations = search(theParams);

assertThat(locations, notNullValue());
assertThat(locations.size(), equalTo(5));

List<Location> resultList = get(locations);

assertThat(resultList, hasSize(equalTo(5)));
List<String> locationNames = resultList.stream().map(Location::getName).collect(Collectors.toList());
assertThat(locationNames, hasItem("Test location 4")); // element search for ("below" search should be inclusive)
assertThat(locationNames, hasItem("Test location 6")); // child element
assertThat(locationNames, hasItem("Test location 8")); // child element
assertThat(locationNames, hasItem("Test location 11")); // grandchild element
assertThat(locationNames, hasItem("Test location 12")); // great grandchild element
}

public void searchForLocations_shouldReturnCorrectLocationsByAncestorPostalCode() {

ReferenceAndListParam ancestorLocation = new ReferenceAndListParam().addAnd(new ReferenceOrListParam()
.add(new ReferenceParam().setValue(LOCATION_ANCESTOR_TEST_POSTAL_CODE).setChain("address-postal-code")));

SearchParameterMap theParams = new SearchParameterMap().addParameter(FhirConstants.LOCATION_ANCESTOR_SEARCH_HANDLER,
ancestorLocation);

IBundleProvider locations = search(theParams);

assertThat(locations, notNullValue());
assertThat(locations.size(), equalTo(5));

List<Location> resultList = get(locations);

assertThat(resultList, hasSize(equalTo(5)));
List<String> locationNames = resultList.stream().map(Location::getName).collect(Collectors.toList());
assertThat(locationNames, hasItem("Test location 4")); // element search for ("below" search should be inclusive)
assertThat(locationNames, hasItem("Test location 6")); // child element
assertThat(locationNames, hasItem("Test location 8")); // child element
assertThat(locationNames, hasItem("Test location 11")); // grandchild element
assertThat(locationNames, hasItem("Test location 12")); // great grandchild element
}

private List<Location> getLocationListWithoutNulls(SortSpec sort) {
SearchParameterMap theParams = new SearchParameterMap().setSortSpec(sort);
IBundleProvider locations = search(theParams);
Expand Down
Loading
Loading