Skip to content

Commit

Permalink
Merge pull request #3376 from IBM/issue-1777
Browse files Browse the repository at this point in the history 
issue #1777 - extract compartment inclusion params even when filtered
  • Loading branch information
@lmsurpre
lmsurpre authored Mar 2, 2022
2 parents 4ff0e06 + b320e22 commit 1e4e99f
Show file tree
Hide file tree
Showing 18 changed files with 487 additions and 398 deletions.
4 changes: 3 additions & 1 deletion fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2017, 2021
* (C) Copyright IBM Corp. 2017, 2022
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down Expand Up @@ -177,6 +177,8 @@ public static void set(FHIRRequestContext context) {

/**
* Returns the FHIRRequestContext on the current thread.
* If it doesn't exist yet, this method will create a default instance
* and associate that with the current thread before returning it.
*/
public static FHIRRequestContext get() {
FHIRRequestContext result = contexts.get();
Expand Down
24 changes: 20 additions & 4 deletions fhir-model/src/test/java/com/ibm/fhir/model/test/TestUtil.java
View file
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
/*
* (C) Copyright IBM Corp. 2019, 2021
* (C) Copyright IBM Corp. 2019, 2022
*
* SPDX-License-Identifier: Apache-2.0
*/

package com.ibm.fhir.model.test;

import static com.ibm.fhir.model.type.String.string;
import static org.testng.AssertJUnit.fail;

import java.io.File;
Expand Down Expand Up @@ -117,12 +116,29 @@ public static JsonObject getRequestJsonObject(String method, String url) {
* the specified patient via a subject attribute.
*/
public static Observation buildPatientObservation(String patientId, String fileName) throws Exception {
// TODO review Reference id
Observation observation = readLocalResource(fileName);

observation = observation
.toBuilder()
.subject(Reference.builder().reference(string("Patient/" + patientId)).build())
.subject(Reference.builder().reference("Patient/" + patientId).build())
.build();
return observation;
}

/**
* Loads an Observation resource from the specified file, then associates it with
* <ul>
* <li>the specified patient via Observation.subject
* <li>the specified practitioner via Observation.performer
* </ul>
*/
public static Observation buildPatientObservation(String patientId, String practitionerId, String fileName) throws Exception {
Observation observation = readLocalResource(fileName);

observation = observation
.toBuilder()
.subject(Reference.builder().reference("Patient/" + patientId).build())
.performer(Reference.builder().reference("Practitioner/" + practitionerId).build())
.build();
return observation;
}
Expand Down
18 changes: 17 additions & 1 deletion ...istence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dto/ExtractedParameterValue.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2017, 2021
* (C) Copyright IBM Corp. 2017, 2022
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand All @@ -26,6 +26,9 @@ public abstract class ExtractedParameterValue implements Comparable<ExtractedPar
private String url;
private String version;

// Whether to store the extracted value or not; defaulting to true
private boolean isForStoring = true;

/**
* Protected constructor
*/
Expand Down Expand Up @@ -175,4 +178,17 @@ public int compareTo(ExtractedParameterValue o) {
*/
protected abstract int compareToInner(ExtractedParameterValue o);

/**
* @return whether this extracted parameter value is for storing
*/
public boolean isForStoring() {
return isForStoring;
}

/**
* @param isForStoring whether this extracted parameter value is for storing
*/
public void setForStoring(boolean isForStoring) {
this.isForStoring = isForStoring;
}
}
105 changes: 66 additions & 39 deletions ...stence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.LinkedList;
import java.util.List;
import java.util.Map;
import java.util.Map.Entry;
Expand Down Expand Up @@ -169,7 +170,6 @@
import com.ibm.fhir.search.exception.FHIRSearchException;
import com.ibm.fhir.search.parameters.InclusionParameter;
import com.ibm.fhir.search.parameters.QueryParameter;
import com.ibm.fhir.search.reference.value.CompartmentReference;
import com.ibm.fhir.search.util.ReferenceValue;
import com.ibm.fhir.search.util.ReferenceValue.ReferenceType;
import com.ibm.fhir.search.util.SearchUtil;
Expand Down Expand Up @@ -493,20 +493,6 @@ private com.ibm.fhir.persistence.jdbc.dto.Resource createResourceDTO(Class<? ext
return resourceDTO;
}

/**
* Creates and returns a copy of the passed resource with the {@code Resource.id}
* {@code Resource.meta.versionId}, and {@code Resource.meta.lastUpdated} elements replaced.
*
* @param resource
* @param logicalId
* @param newVersionNumber
* @param lastUpdated
* @return the updated resource
*/
private <T extends Resource> T copyAndSetResourceMetaFields(T resource, String logicalId, int newVersionNumber, Instant lastUpdated) {
return FHIRPersistenceUtil.copyAndSetResourceMetaFields(resource, logicalId, newVersionNumber, lastUpdated);
}

/**
* Convenience method to construct a new instance of the {@link ResourceDAO}
* @param connection the connection to the database for the DAO to use
Expand Down Expand Up @@ -1601,8 +1587,10 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
String version;
String type;
String expression;
boolean isForStoring;

List<ExtractedParameterValue> allParameters = new ArrayList<>();
// LinkedList because we don't need random access, we might remove from it later, and we want this fast
List<ExtractedParameterValue> allParameters = new LinkedList<>();
String parameterHashB64 = null;

try {
Expand All @@ -1617,6 +1605,9 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
version = sp.getVersion() != null ? sp.getVersion().getValue(): null;
final boolean wholeSystemParam = isWholeSystem(sp);

String doNotStoreExtVal = FHIRUtil.getExtensionStringValue(sp, SearchConstants.DO_NOT_STORE_EXT_URL);
isForStoring = (doNotStoreExtVal == null) || "false".equals(doNotStoreExtVal);

// As not to inject any other special handling logic, this is a simple inline check to see if
// _id or _lastUpdated are used, and ignore those extracted values.
if (SPECIAL_HANDLING.contains(code)) {
Expand Down Expand Up @@ -1665,9 +1656,8 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
continue;
}

// Alternative: consider pulling the search parameter from the FHIRRegistry instead so we can use versioned references.
// Of course, that would require adding extension-search-params to the Registry which would require the Registry to be tenant-aware.
// SearchParameter compSP = FHIRRegistry.getInstance().getResource(component.getDefinition().getValue(), SearchParameter.class);
// Alternatively, we could pull the search parameter from the FHIRRegistry so we can use versioned references.
// However, that would bypass search parameter filtering and so we favor the SeachUtil method here instead.
SearchParameter compSP = SearchUtil.getSearchParameter(p.getResourceType(), component.getDefinition());
JDBCParameterBuildingVisitor parameterBuilder = new JDBCParameterBuildingVisitor(p.getResourceType(), compSP);
FHIRPathNode node = nodes.iterator().next();
Expand Down Expand Up @@ -1757,6 +1747,7 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
if (components.size() == p.getComponent().size()) {

// only add the parameter if all of the components are present and accounted for
p.setForStoring(isForStoring);
allParameters.add(p);
}
}
Expand All @@ -1779,6 +1770,7 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
if (wholeSystemParam) {
p.setWholeSystem(true);
}
p.setForStoring(isForStoring);
allParameters.add(p);
if (log.isLoggable(Level.FINE)) {
log.fine("Extracted Parameter '" + p.getName() + "' from Resource.");
Expand Down Expand Up @@ -1814,6 +1806,7 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
if (wholeSystemParam) {
p.setWholeSystem(true);
}
p.setForStoring(isForStoring);
allParameters.add(p);
if (log.isLoggable(Level.FINE)) {
log.fine("Extracted Parameter '" + p.getName() + "' from Resource.");
Expand All @@ -1825,14 +1818,20 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
// Augment the extracted parameter list with special values we use to represent compartment relationships.
// These references are stored as tokens and are used by the search query builder
// for compartment-based searches
addCompartmentParams(allParameters, fhirResource);
addCompartmentParams(allParameters, fhirResource.getClass().getSimpleName());

// If this is a definitional resource, augment the extracted parameter list with a composite
// parameter that will be used for canonical searches. It will contain the url and version
// values.
addCanonicalCompositeParam(allParameters);
}

// Remove parameters that aren't to be stored
boolean anyRemoved = allParameters.removeIf(value -> !value.isForStoring());
if (anyRemoved) {
log.warning("The set of extracted parameters values unexpectedly contained values "
+ "that weren't for storing; these have been removed");
}

// Generate the hash which is used to quickly determine whether the extracted parameters
// are different than the extracted parameters that currently exist in the database.
Expand Down Expand Up @@ -1886,39 +1885,67 @@ private boolean isWholeSystem(SearchParameter sp) {
}

/**
* Augment the given allParameters list with ibm-internal parameters that represent relationships
* between the fhirResource to its compartments. These parameter values are subsequently used
* Augment the given allParameters list with ibm-internal parameters that represent the relationship
* between the fhirResource and its compartments. These parameter values are subsequently used
* to improve the performance of compartment-based FHIR search queries. See
* {@link CompartmentUtil#makeCompartmentParamName(String)} for details on how the
* parameter name is composed for each relationship.
* @param allParameters
* @param resourceType
*/
protected void addCompartmentParams(List<ExtractedParameterValue> allParameters, Resource fhirResource) throws FHIRSearchException {
final String resourceType = fhirResource.getClass().getSimpleName();
protected void addCompartmentParams(List<ExtractedParameterValue> allParameters, String resourceType) {
if (log.isLoggable(Level.FINE)) {
log.fine("Processing compartment parameters for resourceType: " + resourceType);
}

Map<String,Set<String>> compartmentRefParams = CompartmentUtil.getCompartmentParamsForResourceType(resourceType);
Map<String, Set<CompartmentReference>> compartmentMap = SearchUtil.extractCompartmentParameterValues(fhirResource, compartmentRefParams);
Map<String, List<ExtractedParameterValue>> collectedEPVByCode = allParameters.stream()
.collect(Collectors.groupingBy(epv -> epv.getName()));

for (Map.Entry<String, Set<CompartmentReference>> entry: compartmentMap.entrySet()) {
final String compartmentName = entry.getKey();
final String parameterName = CompartmentUtil.makeCompartmentParamName(compartmentName);
for (Entry<String, Set<String>> entry : compartmentRefParams.entrySet()) {
String param = entry.getKey();
Set<String> compartments = entry.getValue();

// Create a reference parameter value for each CompartmentReference extracted from the resource
for (CompartmentReference compartmentRef: entry.getValue()) {
ReferenceParmVal pv = new ReferenceParmVal();
pv.setName(parameterName);
pv.setResourceType(resourceType);
List<ExtractedParameterValue> collectedEPV = collectedEPVByCode.get(param);
// If the parameter has no corresponding extracted values, log and continue
if (collectedEPV == null) {
if (log.isLoggable(Level.FINE)) {
log.warning("Compartment inclusion param " + param + " has no value");
}
continue;
}

// ReferenceType doesn't really matter here, but LITERAL_RELATIVE is appropriate
ReferenceValue rv = new ReferenceValue(compartmentName, compartmentRef.getReferenceResourceValue(), ReferenceType.LITERAL_RELATIVE, null);
pv.setRefValue(rv);
for (ExtractedParameterValue epv : collectedEPV) {
if (!(epv instanceof ReferenceParmVal)) {
log.warning("Skipping compartment inclusion param " + param + "; expected ReferenceParmVal but found " +
epv.getClass().getSimpleName());
continue;
}

if (log.isLoggable(Level.FINE)) {
log.fine("Adding compartment reference parameter: [" + resourceType + "] "+ parameterName + " = " + rv.getTargetResourceType() + "/" + rv.getValue());
ReferenceValue rv = ((ReferenceParmVal) epv).getRefValue();
if (rv != null && rv.getType() == ReferenceType.LITERAL_RELATIVE
&& compartments.contains(rv.getTargetResourceType())) {
String internalCompartmentParamName = CompartmentUtil.makeCompartmentParamName(rv.getTargetResourceType());

if (epv.isForStoring()) {
// create a copy of the extracted parameter value but with the new internal compartment parameter name
ReferenceParmVal pv = new ReferenceParmVal();
pv.setName(internalCompartmentParamName);
pv.setResourceType(resourceType);
pv.setRefValue(rv);

if (log.isLoggable(Level.FINE)) {
log.fine("Adding compartment reference parameter: [" + resourceType + "] " +
internalCompartmentParamName + " = " + rv.getTargetResourceType() + "/" + rv.getValue());
}
allParameters.add(pv);
} else {
// since the extracted parameter value isn't going to be stored,
// just rename it with our internal compartment param name and mark that for storing
epv.setName(internalCompartmentParamName);
epv.setForStoring(true);
}
}
allParameters.add(pv);
}
}
}
Expand Down
6 changes: 6 additions & 0 deletions fhir-search/src/main/java/com/ibm/fhir/search/SearchConstants.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,7 @@
import com.ibm.fhir.core.FHIRConstants;
import com.ibm.fhir.model.type.Code;
import com.ibm.fhir.model.type.Coding;
import com.ibm.fhir.model.type.Extension;
import com.ibm.fhir.model.type.Uri;
import com.ibm.fhir.search.exception.SearchExceptionUtil;

Expand Down Expand Up @@ -102,6 +103,11 @@ private SearchConstants() {
public static final String VERSION = "version";

public static final String IMPLICIT_SYSTEM_EXT_URL = FHIRConstants.EXT_BASE + "implicit-system";
public static final String DO_NOT_STORE_EXT_URL = FHIRConstants.EXT_BASE + "do-not-store";
public static final Extension DO_NOT_STORE_EXT = Extension.builder()
.url(SearchConstants.DO_NOT_STORE_EXT_URL)
.value(true)
.build();

// Extracted search parameter suffix for :identifier modifier
public static final String IDENTIFIER_MODIFIER_SUFFIX = ":identifier";
Expand Down
Loading

0 comments on commit 1e4e99f

Please sign in to comment.