Skip to content

Commit

Permalink
issue #1777 - do not store temporary/internal param values
Browse files Browse the repository at this point in the history 
In the previous commit, we started extracting all compartment inclusion
criteria search parameters, even in cases where they are filtered out
via config.
Although the filtered out parameters would not be searchable, we were
still storing these non-searchable parameter values in the db (once with
the inclusion criteria name and once with the corresponding
ibm-internal-x-compartment search parameter name).

This commit updates the search extraction implementation to avoid
storing these intermediate inclusion criteria parameters to the db.
To accomplish this, we now:
1. Add a DO_NOT_STORE extension on the intermediate search parameters
that are used to extract compartment membership information prior to
collecting those values for the ibm-internal-x-compartment search params
2. Propagate that information to the ExtractedParameterValues
3. Filter out parameter values that are not for storage before computing
the extract search parameters hash and returning
ExtractedSearchParameters

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>
  • Loading branch information
@lmsurpre
lmsurpre committed Feb 22, 2022
1 parent 2c9ea8c commit 21eeb11
Show file tree
Hide file tree
Showing 7 changed files with 88 additions and 265 deletions.
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;
}
}
100 changes: 53 additions & 47 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 @@ -152,7 +152,6 @@
import com.ibm.fhir.persistence.jdbc.util.TimestampPrefixedUUID;
import com.ibm.fhir.persistence.payload.FHIRPayloadPersistence;
import com.ibm.fhir.persistence.payload.PayloadPersistenceResponse;
import com.ibm.fhir.persistence.util.FHIRPersistenceUtil;
import com.ibm.fhir.persistence.util.InputOutputByteStream;
import com.ibm.fhir.persistence.util.LogicalIdentityProvider;
import com.ibm.fhir.schema.control.FhirSchemaConstants;
Expand Down Expand Up @@ -489,20 +488,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 @@ -1514,6 +1499,7 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
String version;
String type;
String expression;
boolean isForStoring;

List<ExtractedParameterValue> allParameters = new ArrayList<>();
String parameterHashB64 = null;
Expand All @@ -1530,6 +1516,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 @@ -1578,9 +1567,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 @@ -1670,6 +1658,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 @@ -1692,6 +1681,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 @@ -1727,6 +1717,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 @@ -1738,14 +1729,19 @@ 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("Removed unexpectedly found extracted search parameter values that weren't for storing");
}

// 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 @@ -1799,57 +1795,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, ExtractedParameterValue> collectedEPV = allParameters.stream()
.collect(Collectors.toMap(epv -> epv.getName(), epv -> epv));
Map<String, List<ExtractedParameterValue>> collectedEPVByCode = allParameters.stream()
.collect(Collectors.groupingBy(epv -> epv.getName()));

for (Entry<String, Set<String>> entry : compartmentRefParams.entrySet()) {
String param = entry.getKey();
Set<String> compartments = entry.getValue();

ExtractedParameterValue epv = collectedEPV.get(param);
// If the parameter has no corresponding extracted value, log a warning and continue
if (epv == null) {
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.fine("Compartment inclusion param " + param + " has no value");
log.warning("Compartment inclusion param " + param + " has no value");
}
continue;
}

if (!(epv instanceof ReferenceParmVal)) {
log.warning("Skipping compartment inclusion param " + param + "; expected ReferenceParmVal but found " +
epv.getClass().getSimpleName());
continue;
}
for (ExtractedParameterValue epv : collectedEPV) {
if (!(epv instanceof ReferenceParmVal)) {
log.warning("Skipping compartment inclusion param " + param + "; expected ReferenceParmVal but found " +
epv.getClass().getSimpleName());
continue;
}

ReferenceValue rv = ((ReferenceParmVal) epv).getRefValue();
if (rv != null && rv.getType() == ReferenceType.LITERAL_RELATIVE
&& compartments.contains(rv.getTargetResourceType())) {
String internalCompartmentParamName = CompartmentUtil.makeCompartmentParamName(rv.getTargetResourceType());
ReferenceValue rv = ((ReferenceParmVal) epv).getRefValue();
if (rv != null && rv.getType() == ReferenceType.LITERAL_RELATIVE
&& compartments.contains(rv.getTargetResourceType())) {
String internalCompartmentParamName = CompartmentUtil.makeCompartmentParamName(rv.getTargetResourceType());

// 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 (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());
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
1 change: 1 addition & 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 @@ -102,6 +102,7 @@ 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";

// Extracted search parameter suffix for :identifier modifier
public static final String IDENTIFIER_MODIFIER_SUFFIX = ":identifier";
Expand Down
16 changes: 13 additions & 3 deletions fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersMap.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2019, 2021
* (C) Copyright IBM Corp. 2019, 2022
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand All @@ -17,6 +17,8 @@
import java.util.stream.Collectors;

import com.ibm.fhir.model.resource.SearchParameter;
import com.ibm.fhir.model.type.Extension;
import com.ibm.fhir.search.SearchConstants;

/**
* A multi-key map that indexes a set of search parameters by SearchParameter.code and
Expand All @@ -27,6 +29,11 @@ public class ParametersMap {

public static final String MISSING_EXPRESSION_WARNING = "Skipping parameter '%s' with missing expression";

public static final Extension DO_NOT_STORE = Extension.builder()
.url(SearchConstants.DO_NOT_STORE_EXT_URL)
.value(true)
.build();

private final Map<String, SearchParameter> codeMap;
private final Map<String, SearchParameter> canonicalMap;

Expand Down Expand Up @@ -91,17 +98,20 @@ public void insert(String code, SearchParameter parameter) {
/**
* @param code
* @param parameter
* @param compartments
* @implSpec Any existing parameters will be replaced and a warning will be logged; last insert wins
*/
public void insertInclusionParam(String code, SearchParameter parameter, Set<String> compartments) {
public void insertInclusionParam(String code, SearchParameter parameter) {
Objects.requireNonNull(code, "cannot insert a null code");
Objects.requireNonNull(parameter, "cannot insert a null parameter");

if (inclusionParamMap.containsKey(code)) {
SearchParameter previous = inclusionParamMap.get(code);
logParamConflict("inclusion criteria '" + code + "'", parameter, ParametersUtil.getCanonicalUrl(parameter), previous);
}

parameter = parameter.toBuilder()
.extension(DO_NOT_STORE)
.build();
inclusionParamMap.put(code, parameter);
}

Expand Down
8 changes: 3 additions & 5 deletions fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java
View file
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* (C) Copyright IBM Corp. 2019, 2021
* (C) Copyright IBM Corp. 2019, 2022
*
* SPDX-License-Identifier: Apache-2.0
*/
Expand Down Expand Up @@ -178,8 +178,6 @@ private static Map<String, ParametersMap> computeTenantSPs(PropertyGroup rsrcsGr

addWildcardAndCompartmentParams(paramMapsByType, resourceTypesWithWildcardParams, configuredCodes);

// addCompartmentParams(paramMapsByType, resourceTypesWithWildcardParams, configuredCodes);

return Collections.unmodifiableMap(paramMapsByType);
}

Expand Down Expand Up @@ -210,7 +208,7 @@ private static ParametersMap getExplicitParams(Set<String> resourceTypesWithWild

// If this param is an inclusion criteria for one or more compartments
if (compartmentParamToCompartment.containsKey(code)) {
paramMap.insertInclusionParam(code, sp, compartmentParamToCompartment.get(code));
paramMap.insertInclusionParam(code, sp);
}
} else {
log.warning("Search parameter '" + code + "' with the configured url '" + spEntry.getValue() +
Expand Down Expand Up @@ -274,7 +272,7 @@ private static void addWildcardAndCompartmentParams(Map<String, ParametersMap> p
sp.getCode().getValue() + "' is already configured.");
}
} else {
paramMap.insertInclusionParam(code, sp, compartmentParamToCompartment.get(code));
paramMap.insertInclusionParam(code, sp);
}
}
}
Expand Down
Loading

0 comments on commit 21eeb11

Please sign in to comment.