Skip to content

Commit

Permalink
issues #1625 and #1596 - apply search parameter filters on init (#2942)
Browse files Browse the repository at this point in the history 
* issues #1625 and #1596 - apply search parameter filters on init

This is a big, end-user-visible change to the way we load search
parameters and apply filtering from fhir-server-config.

Previously, we loaded the built-in search parameters from the registry,
but then applied tenant-specific extension-search-parameters on top of
that.

Now, we have a new ExtensionSearchParametersResourceProvider that reads
the same tenant-specific extension-search-parameters (for backwards
compatibility) and provides those resources to the registry in a
tenant-aware manner.  This is #1596.

Additionally, we used to apply search filtering on each and every
request. Now, the ParametersUtil will loop through all configured
tenants and load their search parameters from the registry during
initialization. This tenant-aware search parameter map will then be
consulted anytime we need a search parameter or set of search parameters
in the context of a particular request (for a particular tenant).

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* JDBC test cleanup

LocationParmBehaviorUtilTest wasn't restoring the FHIRRequestContext
after running.
Additionally, I removed the 'static' modifier from the before and after
methods...its not needed for TestNG and, in fact, TestNG warns against
using static methods for this.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Handle missing config more gracefully

The CQL SearchParameterResolverTest was calling
SearchUtil.getSearchParameters without any fhir-server-config files in
the home directory.
This was causing a NullPointerException while initializing
ParametersUtil.
The updated logic will now load all parameters in such cases...just as
if fhir-server-config was there but without a `resources` PropertyGroup.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* updated documentation to reflect new behavior

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* General cleanup

Extracted private helpers from ParametersUtil.computeTenantSPs and added
some comments.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Update SearchUtil.getSearchParameters method sig

This one now returns a map from code to SearchParameter instead of just
a the list of SearchParameter. This way, we can support cases where the
explicitly-configured code differs from the code in the SearchParameter.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* Update ParametersMap to store disambiguated results

Previously, ParametersMap had to store all the built-in parameters and
make those available to SearchUtil for filtering/disambiguation.

Now that we apply filtering before populating ParametersMap, we can
select a "winner" in cases where there are multiple parameter with the
same code (or multiple parameters with the same url / canonical).

For now, I chose a "last insert wins" approach...which means that the
order we load the map in will matter.

Note: this change uncovered an issue in our fhir-persistence search
tests where we defined a parameter with code "code" that conflicts with
a base spec parameter. The fix was to disambiguate via
fhir-server-config.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

* minor updates per review feedback

mostly copyright dates...

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
  • Loading branch information
@lmsurpre
lmsurpre authored Nov 9, 2021
1 parent db6813d commit 95d1278
Show file tree
Hide file tree
Showing 51 changed files with 996 additions and 1,115 deletions.
68 changes: 36 additions & 32 deletions ...l/src/main/java/com/ibm/fhir/cql/engine/retrieve/SearchParameterFHIRRetrieveProvider.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@
import com.ibm.fhir.cql.engine.searchparam.TokenParameter;
import com.ibm.fhir.model.resource.SearchParameter;
import com.ibm.fhir.model.type.code.SearchParamType;
import com.ibm.fhir.model.util.ModelSupport;
import com.ibm.fhir.search.SearchConstants;
import com.ibm.fhir.search.SearchConstants.Prefix;

Expand Down Expand Up @@ -72,18 +73,24 @@ public int getMaxCodesPerQuery() {
return this.maxCodesPerQuery;
}

protected abstract Iterable<Object> executeQueries(String dataType, List<SearchParameterMap> queries) throws Exception;
protected abstract Iterable<Object> executeQueries(String resourceType, List<SearchParameterMap> queries) throws Exception;

@Override
public Iterable<Object> retrieve(String context, String contextPath, Object contextValue, String dataType,
String templateId, String codePath, Iterable<Code> codes, String valueSet, String datePath,
String dateLowPath, String dateHighPath, Interval dateRange) {

if (!ModelSupport.isResourceType(dataType)) {
throw new IllegalArgumentException("'" + dataType + "' is not a valid resource type;"
+ " SearchParameterFHIRRetrieveProvider can only retrieve FHIR resource data");
}
String resourceType = dataType;

try {
List<SearchParameterMap> queries =
this.setupQueries(context, contextPath, contextValue, dataType, templateId, codePath, codes, valueSet, datePath, dateLowPath, dateHighPath, dateRange);
this.setupQueries(context, contextPath, contextValue, resourceType, templateId, codePath, codes, valueSet, datePath, dateLowPath, dateHighPath, dateRange);

return this.executeQueries(dataType, queries);
return this.executeQueries(resourceType, queries);
} catch (RuntimeException rex) {
throw rex;
} catch (Exception ex) {
Expand All @@ -92,28 +99,27 @@ public Iterable<Object> retrieve(String context, String contextPath, Object cont
}

protected List<SearchParameterMap> setupQueries(String context, String contextPath, Object contextValue,
String dataType, String templateId, String codePath, Iterable<Code> codes, String valueSet, String datePath,
String resourceType, String templateId, String codePath, Iterable<Code> codes, String valueSet, String datePath,
String dateLowPath, String dateHighPath, Interval dateRange) throws Exception {

List<SearchParameterMap> queries = null;

Pair<String, IQueryParameter> templateParam = this.getTemplateParam(dataType, templateId);
Pair<String, IQueryParameter> contextParam = this.getContextParam(dataType, context, contextPath, contextValue);
Pair<String, DateRangeParameter> dateRangeParam = this.getDateRangeParam(dataType, datePath, dateLowPath, dateHighPath, dateRange);
Pair<String, List<IQueryParameterOr<?>>> codeParams = this.getCodeParams(dataType, codePath, codes, valueSet);
Pair<String, IQueryParameter> templateParam = this.getTemplateParam(resourceType, templateId);
Pair<String, IQueryParameter> contextParam = this.getContextParam(resourceType, context, contextPath, contextValue);
Pair<String, DateRangeParameter> dateRangeParam = this.getDateRangeParam(resourceType, datePath, dateLowPath, dateHighPath, dateRange);
Pair<String, List<IQueryParameterOr<?>>> codeParams = this.getCodeParams(resourceType, codePath, codes, valueSet);

// In the case we filtered to a valueSet without codes, there are no possible
// results.
// In the case we filtered to a valueSet without codes, there are no possible results.
if (valueSet != null && (codeParams == null || codeParams.getValue().isEmpty())) {
queries = Collections.emptyList();
} else {
queries = this.innerSetupQueries(dataType, templateParam, contextParam, dateRangeParam, codeParams);
queries = this.innerSetupQueries(resourceType, templateParam, contextParam, dateRangeParam, codeParams);
}

return queries;
}

protected List<SearchParameterMap> innerSetupQueries(String dataType, Pair<String, IQueryParameter> templateParam,
protected List<SearchParameterMap> innerSetupQueries(String resourceType, Pair<String, IQueryParameter> templateParam,
Pair<String, IQueryParameter> contextParam, Pair<String, DateRangeParameter> dateRangeParam,
Pair<String, List<IQueryParameterOr<?>>> codeParams) throws Exception {

Expand All @@ -134,30 +140,30 @@ protected List<SearchParameterMap> innerSetupQueries(String dataType, Pair<Strin
return result;
}

protected Pair<String, IQueryParameter> getTemplateParam(String dataType, String templateId) {
protected Pair<String, IQueryParameter> getTemplateParam(String resourceType, String templateId) {
// purposefully empty
return null;
}

protected Pair<String, IQueryParameter> getContextParam(String dataType, String context, String contextPath,
Object contextValue) throws Exception {
protected Pair<String, IQueryParameter> getContextParam(String resourceType, String context, String contextPath,
Object contextValue) throws Exception {
Pair<String, IQueryParameter> result = null;

if (context != null && "Patient".equals(context) && contextValue != null && contextPath != null) {
result = searchParameterResolver.createSearchParameter(context, dataType, contextPath, (String) contextValue);
result = searchParameterResolver.createSearchParameter(context, resourceType, contextPath, (String) contextValue);
if (result == null) {
throw new IllegalArgumentException(String.format("Could not resolve search parameter for dataType '%s' and contextPath '%s'", dataType, contextPath));
throw new IllegalArgumentException(String.format("Could not resolve search parameter for resourceType '%s' and contextPath '%s'", resourceType, contextPath));
}
}

return result;
}

protected Pair<String, DateRangeParameter> getDateRangeParam(String dataType, String datePath, String dateLowPath,
String dateHighPath, Interval dateRange) throws Exception {
protected Pair<String, DateRangeParameter> getDateRangeParam(String resourceType, String datePath, String dateLowPath,
String dateHighPath, Interval dateRange) throws Exception {
Pair<String, DateRangeParameter> result = null;
if (datePath != null) {
SearchParameter dateParam = this.searchParameterResolver.getSearchParameterDefinition(dataType, datePath, SearchParamType.DATE);
SearchParameter dateParam = this.searchParameterResolver.getSearchParameterDefinition(resourceType, datePath, SearchParamType.DATE);
if (dateParam != null) {
String name = dateParam.getName().getValue();

Expand All @@ -182,35 +188,33 @@ protected Pair<String, DateRangeParameter> getDateRangeParam(String dataType, St

result = Pair.of(name, rangeParam);
} else {
throw new UnsupportedOperationException(String.format("Could not resolve a search parameter with date type for %s.%s ", dataType, datePath));
throw new UnsupportedOperationException(String.format("Could not resolve a search parameter with date type for %s.%s ", resourceType, datePath));
}
}
return result;
}

protected Pair<String, List<IQueryParameterOr<?>>> getCodeParams(String dataType, String codePath, Iterable<Code> codes,
String valueSet) throws Exception {
protected Pair<String, List<IQueryParameterOr<?>>> getCodeParams(String resourceType, String codePath, Iterable<Code> codes,
String valueSet) throws Exception {

Pair<String, List<IQueryParameterOr<?>>> result = null;

if (codePath != null) {
SearchParameter searchParam = searchParameterResolver.getSearchParameterDefinition(dataType, codePath, SearchParamType.TOKEN);
SearchParameter searchParam = searchParameterResolver.getSearchParameterDefinition(resourceType, codePath, SearchParamType.TOKEN);
if (searchParam != null) {
String name = searchParam.getName().getValue();
result = Pair.of(name, getCodeParams(name, codes, valueSet));
} else {
throw new IllegalArgumentException(String.format("Could not resolve search parameter for dataType '%s' and codePath '%s'", dataType, codePath));
throw new IllegalArgumentException(String.format("Could not resolve search parameter for resourceType '%s' and codePath '%s'", resourceType, codePath));
}
}

return result;
}

// The code params will be either the literal set of codes in the event the data
// server doesn't have the referenced ValueSet
// (or doesn't support pulling and caching a ValueSet).
// If the target server DOES support that then it's "dataType.codePath in
// ValueSet"
// server doesn't have the referenced ValueSet (or doesn't support pulling and caching a ValueSet).
// If the target server DOES support that then it's "resourceType.codePath in ValueSet"
protected List<IQueryParameterOr<?>> getCodeParams(String name, Iterable<Code> codes, String valueSet) {
List<IQueryParameterOr<?>> params = null;

Expand Down Expand Up @@ -260,7 +264,7 @@ protected List<IQueryParameterOr<?>> getCodeParams(String name, Iterable<Code> c
}

protected SearchParameterMap getBaseMap(Pair<String, IQueryParameter> templateParam,
Pair<String, IQueryParameter> contextParam, Pair<String, DateRangeParameter> dateRangeParam) throws Exception {
Pair<String, IQueryParameter> contextParam, Pair<String, DateRangeParameter> dateRangeParam) throws Exception {

SearchParameterMap searchParameters = new SearchParameterMap();
// baseMap.setLastUpdated(new DateRangeParam());
Expand All @@ -283,11 +287,11 @@ protected SearchParameterMap getBaseMap(Pair<String, IQueryParameter> templatePa

return searchParameters;
}

/**
* Given a query parameter name and contents, transmute the name
* into something that includes all the appropriate modifiers.
*
*
* @param name query parameter name
* @param param query parameter contents
* @return query parameter name with appropriate modifiers appended
Expand Down
32 changes: 17 additions & 15 deletions cql/fhir-cql/src/main/java/com/ibm/fhir/cql/engine/searchparam/SearchParameterResolver.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@
import java.util.ArrayList;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Set;

import org.apache.commons.lang3.tuple.Pair;
Expand All @@ -20,13 +21,13 @@

public class SearchParameterResolver {

public SearchParameter getSearchParameterDefinition(String dataType, String path) throws Exception {
return getSearchParameterDefinition(dataType, path, null);
public SearchParameter getSearchParameterDefinition(String resourceType, String path) throws Exception {
return getSearchParameterDefinition(resourceType, path, null);
}

public SearchParameter getSearchParameterDefinition(String dataType, String path, SearchParamType paramType)
throws Exception {
if (dataType == null || path == null) {
public SearchParameter getSearchParameterDefinition(String resourceType, String path, SearchParamType paramType)
throws Exception {
if (resourceType == null || path == null) {
return null;
}

Expand All @@ -36,15 +37,16 @@ public SearchParameter getSearchParameterDefinition(String dataType, String path
path = "";
}

List<SearchParameter> params = SearchUtil.getApplicableSearchParameters(dataType);
// XXX should this use the registry (all parameters) or the filtered set of search parameters for a given tenant?
Map<String, SearchParameter> params = SearchUtil.getSearchParameters(resourceType);

for (SearchParameter param : params) {
for (SearchParameter param : params.values()) {
if (name != null && param.getName().getValue().equals(name)) {
return param;
}

if (paramType == null || param.getType().equals(paramType)) {
Set<String> normalizedPath = normalizePath(dataType, param.getExpression().getValue());
Set<String> normalizedPath = normalizePath(resourceType, param.getExpression().getValue());
if (normalizedPath.contains(path)) {
return param;
}
Expand All @@ -54,12 +56,12 @@ public SearchParameter getSearchParameterDefinition(String dataType, String path
return null;
}

public Pair<String, IQueryParameter> createSearchParameter(String context, String dataType, String path, String value)
throws Exception {
public Pair<String, IQueryParameter> createSearchParameter(String context, String resourceType, String path, String value)
throws Exception {

Pair<String, IQueryParameter> result = null;

SearchParameter searchParam = this.getSearchParameterDefinition(dataType, path);
SearchParameter searchParam = this.getSearchParameterDefinition(resourceType, path);
if (searchParam != null) {

String name = searchParam.getName().getValue();
Expand All @@ -86,7 +88,7 @@ public Pair<String, IQueryParameter> createSearchParameter(String context, Strin

// This is actually a lot of processing. We should cache search parameter
// resolutions.
private Set<String> normalizePath(String dataType, String path) {
private Set<String> normalizePath(String resourceType, String path) {
// TODO: What we really need is FhirPath parsing to just get the path
// MedicationAdministration.medication.as(CodeableConcept)
// MedicationAdministration.medication.as(Reference)
Expand All @@ -105,7 +107,7 @@ private Set<String> normalizePath(String dataType, String path) {
part = removeParens(part);

// Trim off DataType
if (part.startsWith(dataType)) {
if (part.startsWith(resourceType)) {
part = part.substring(part.indexOf(".") + 1, part.length());

// Split into components
Expand Down Expand Up @@ -148,8 +150,8 @@ private Set<String> normalizePath(String dataType, String path) {
// }
return normalizedParts;
}
private String removeParens(String path) {

private String removeParens(String path) {
if (path.startsWith("(")) {
path = path.substring(1, path.length() - 1);
}
Expand Down
Loading

0 comments on commit 95d1278

Please sign in to comment.