From c10c55d53d946ac8d6a67fe2e760b1072d82380d Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Mon, 21 Feb 2022 16:13:28 -0500 Subject: [PATCH] issue #1777 - do not store temporary/internal param values 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 --- .../jdbc/dto/ExtractedParameterValue.java | 18 ++- .../jdbc/impl/FHIRPersistenceJDBCImpl.java | 100 ++++++------- .../com/ibm/fhir/search/SearchConstants.java | 1 + .../fhir/search/parameters/ParametersMap.java | 16 ++- .../search/parameters/ParametersUtil.java | 8 +- .../com/ibm/fhir/search/util/SearchUtil.java | 132 +----------------- 6 files changed, 88 insertions(+), 187 deletions(-) diff --git a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dto/ExtractedParameterValue.java b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dto/ExtractedParameterValue.java index ae836291ee0..d8969a40cdc 100644 --- a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dto/ExtractedParameterValue.java +++ b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dto/ExtractedParameterValue.java @@ -1,5 +1,5 @@ /* - * (C) Copyright IBM Corp. 2017, 2021 + * (C) Copyright IBM Corp. 2017, 2022 * * SPDX-License-Identifier: Apache-2.0 */ @@ -26,6 +26,9 @@ public abstract class ExtractedParameterValue implements Comparable 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 @@ -1514,6 +1499,7 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource, String version; String type; String expression; + boolean isForStoring; List allParameters = new ArrayList<>(); String parameterHashB64 = null; @@ -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)) { @@ -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(); @@ -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); } } @@ -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."); @@ -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."); @@ -1738,7 +1729,7 @@ 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 @@ -1746,6 +1737,11 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource, 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. @@ -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 allParameters, Resource fhirResource) throws FHIRSearchException { - final String resourceType = fhirResource.getClass().getSimpleName(); + protected void addCompartmentParams(List allParameters, String resourceType) { if (log.isLoggable(Level.FINE)) { log.fine("Processing compartment parameters for resourceType: " + resourceType); } + Map> compartmentRefParams = CompartmentUtil.getCompartmentParamsForResourceType(resourceType); - Map collectedEPV = allParameters.stream() - .collect(Collectors.toMap(epv -> epv.getName(), epv -> epv)); + Map> collectedEPVByCode = allParameters.stream() + .collect(Collectors.groupingBy(epv -> epv.getName())); for (Entry> entry : compartmentRefParams.entrySet()) { String param = entry.getKey(); Set 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 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); } } } diff --git a/fhir-search/src/main/java/com/ibm/fhir/search/SearchConstants.java b/fhir-search/src/main/java/com/ibm/fhir/search/SearchConstants.java index 81c6be24193..cc70d0dac72 100644 --- a/fhir-search/src/main/java/com/ibm/fhir/search/SearchConstants.java +++ b/fhir-search/src/main/java/com/ibm/fhir/search/SearchConstants.java @@ -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"; diff --git a/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersMap.java b/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersMap.java index 20e18dd5f9f..c0f739bbda5 100644 --- a/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersMap.java +++ b/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersMap.java @@ -1,5 +1,5 @@ /* - * (C) Copyright IBM Corp. 2019, 2021 + * (C) Copyright IBM Corp. 2019, 2022 * * SPDX-License-Identifier: Apache-2.0 */ @@ -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 @@ -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 codeMap; private final Map canonicalMap; @@ -91,10 +98,9 @@ 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 compartments) { + public void insertInclusionParam(String code, SearchParameter parameter) { Objects.requireNonNull(code, "cannot insert a null code"); Objects.requireNonNull(parameter, "cannot insert a null parameter"); @@ -102,6 +108,10 @@ public void insertInclusionParam(String code, SearchParameter parameter, Set computeTenantSPs(PropertyGroup rsrcsGr addWildcardAndCompartmentParams(paramMapsByType, resourceTypesWithWildcardParams, configuredCodes); -// addCompartmentParams(paramMapsByType, resourceTypesWithWildcardParams, configuredCodes); - return Collections.unmodifiableMap(paramMapsByType); } @@ -210,7 +208,7 @@ private static ParametersMap getExplicitParams(Set 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() + @@ -274,7 +272,7 @@ private static void addWildcardAndCompartmentParams(Map p sp.getCode().getValue() + "' is already configured."); } } else { - paramMap.insertInclusionParam(code, sp, compartmentParamToCompartment.get(code)); + paramMap.insertInclusionParam(code, sp); } } } diff --git a/fhir-search/src/main/java/com/ibm/fhir/search/util/SearchUtil.java b/fhir-search/src/main/java/com/ibm/fhir/search/util/SearchUtil.java index 3b4a4334bc3..8700ef49f34 100644 --- a/fhir-search/src/main/java/com/ibm/fhir/search/util/SearchUtil.java +++ b/fhir-search/src/main/java/com/ibm/fhir/search/util/SearchUtil.java @@ -6,8 +6,6 @@ package com.ibm.fhir.search.util; -import static com.ibm.fhir.model.util.ModelSupport.FHIR_STRING; - import java.math.BigDecimal; import java.net.URISyntaxException; import java.net.URLEncoder; @@ -43,9 +41,7 @@ import com.ibm.fhir.model.resource.ValueSet; import com.ibm.fhir.model.type.Canonical; import com.ibm.fhir.model.type.Code; -import com.ibm.fhir.model.type.Element; import com.ibm.fhir.model.type.Extension; -import com.ibm.fhir.model.type.Reference; import com.ibm.fhir.model.type.Uri; import com.ibm.fhir.model.type.code.IssueSeverity; import com.ibm.fhir.model.type.code.IssueType; @@ -77,7 +73,6 @@ import com.ibm.fhir.search.parameters.ParametersUtil; import com.ibm.fhir.search.parameters.QueryParameter; import com.ibm.fhir.search.parameters.QueryParameterValue; -import com.ibm.fhir.search.reference.value.CompartmentReference; import com.ibm.fhir.search.sort.Sort; import com.ibm.fhir.search.uri.UriBuilder; import com.ibm.fhir.search.util.ReferenceValue.ReferenceType; @@ -159,33 +154,6 @@ public static void init() { ParametersUtil.init(); } -// /** -// * @param resourceType -// * @param code -// * @return the SearchParameter for type {@code resourceType} with code {@code code} or null if it doesn't exist -// * @throws Exception -// */ -// public static SearchParameter getInclusionParameter(String resourceType, String code) { -// if (code == null) { -// return null; -// } -// String tenantId = FHIRRequestContext.get().getTenantId(); -// Map paramsByResourceType = ParametersUtil.getTenantSPs(tenantId); -// -// // First try the passed resourceType, then fall back to the Resource resourceType (for whole system params) -// for (String type : new String[]{resourceType, FHIRConfigHelper.RESOURCE_RESOURCE}) { -// ParametersMap parametersMap = paramsByResourceType.get(type); -// if (parametersMap != null) { -// SearchParameter searchParam = parametersMap.lookupByCode(code); -// if (searchParam != null) { -// return searchParam; -// } -// } -// } -// -// return null; -// } - /** * @param resourceType * @param code @@ -347,7 +315,7 @@ public static Map> extractParameterValues(Re Map parameters = getSearchParameters(resourceType.getSimpleName()); for (String inclusionParamName : CompartmentUtil.getCompartmentParamsForResourceType(resourceType.getSimpleName()).keySet()) { - if (!parameters.containsKey(inclusionParamName)) { + if (!COMPARTMENT_PARM_DEF.equals(inclusionParamName) && !parameters.containsKey(inclusionParamName)) { String tenantId = FHIRRequestContext.get().getTenantId(); ParametersMap parametersMap = ParametersUtil.getTenantSPs(tenantId).get(resourceType.getSimpleName()); parameters.put(inclusionParamName, parametersMap.getInclusionParam(inclusionParamName)); @@ -2208,104 +2176,6 @@ private static void manageException(String message, IssueType issueType, FHIRSea } } - /** - * Extracts the parameter values defining compartment membership. - * @param fhirResource - * @param compartmentRefParams a map of parameter names to a set of compartment names (resource types) - * @return a map of compartment name to a set of unique compartment reference values - */ - public static Map> extractCompartmentParameterValues(Resource fhirResource, - Map> compartmentRefParams) throws FHIRSearchException { - final Map> result = new HashMap<>(); - final String resourceType = fhirResource.getClass().getSimpleName(); - - // TODO, probably should use a Bundle.Entry value here if we are processing a bundle - final String baseUrl = ReferenceUtil.getBaseUrl(null); - - try { - EvaluationContext resourceContext = new FHIRPathEvaluator.EvaluationContext(fhirResource); - - // Extract any references we find matching parameters representing compartment membership. - // For example CareTeam.participant can be used to refer to a Patient or RelatedPerson resource: - // "participant": { "reference": "Patient/abc123" } - // "participant": { "reference": "RelatedPerson/abc456" } - for (Map.Entry> paramEntry : compartmentRefParams.entrySet()) { - final String searchParm = paramEntry.getKey(); - - // Ignore {def} which is used in the compartment definition where - // no other search parm is given (e.g. Encounter->Encounter). - if (!COMPARTMENT_PARM_DEF.equals(searchParm)) { - SearchParameter sp = SearchUtil.getSearchParameter(resourceType, searchParm); - if (sp != null && sp.getExpression() != null) { - String expression = sp.getExpression().getValue(); - - if (log.isLoggable(Level.FINE)) { - log.fine("searchParam = [" + resourceType + "] '" + searchParm + "'; expression = '" + expression + "'"); - } - Collection nodes = FHIRPathEvaluator.evaluator().evaluate(resourceContext, expression); - - if (log.isLoggable(Level.FINEST)) { - log.finest("Expression [" + expression + "], parameter-code [" - + searchParm + "], size [" + nodes.size() + "]"); - } - - for (FHIRPathNode node : nodes) { - String compartmentName = null; - String compartmentId = null; - - Element element = node.asElementNode().element(); - if (element.is(Reference.class)) { - Reference reference = element.as(Reference.class); - ReferenceValue rv = ReferenceUtil.createReferenceValueFrom(reference, baseUrl); - if (rv.getType() == ReferenceType.DISPLAY_ONLY || rv.getType() == ReferenceType.INVALID) { - if (log.isLoggable(Level.FINE)) { - log.fine("Skipping reference of type " + rv.getType()); - } - continue; - } - compartmentName = rv.getTargetResourceType(); - compartmentId = rv.getValue(); - - // Check that the target resource type of the reference matches one of the - // target resource types in the compartment definition. - if (!paramEntry.getValue().contains(compartmentName)) { - if (log.isLoggable(Level.FINE)) { - String refVal = (reference.getReference() == null) ? null : reference.getReference().getValue(); - log.fine("Skipping reference with value " + refVal + ";" - + " target resource type does not match any of the allowed compartment types: " + paramEntry); - } - continue; - } - } else if (element.is(FHIR_STRING)) { - if (paramEntry.getValue().size() != 1) { - log.warning("CompartmentDefinition inclusion criteria must be of type Reference unless they have 1 and only 1 resource target"); - continue; - } - compartmentName = paramEntry.getValue().iterator().next(); - compartmentId = element.as(FHIR_STRING).getValue(); - } - - // Add this reference to the set of references we're collecting for each compartment - CompartmentReference cref = new CompartmentReference(searchParm, compartmentName, compartmentId); - Set references = result.computeIfAbsent(compartmentName, k -> new HashSet<>()); - references.add(cref); - } - } else if (!useStoredCompartmentParam()) { - log.warning("Compartment parameter not found: [" + resourceType + "] '" + searchParm + "'. " - + "This will stop compartment searches from working correctly."); - } - - } - } - } catch (Exception e) { - final String msg = "Unexpected exception extracting compartment references " - + " for resource type '" + resourceType + "'"; - log.log(Level.WARNING, msg, e); - throw SearchExceptionUtil.buildNewInvalidSearchException(msg); - } - return result; - } - /** * Throw an exception if the specified search parameter is null. *