From 2c9ea8c1796810f19395f8d7af141f4277eae6ee Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Mon, 21 Feb 2022 09:26:08 -0500 Subject: [PATCH 1/7] issue #1777 - extract compartment inclusion params even when filtered Signed-off-by: Lee Surprenant --- .../jdbc/impl/FHIRPersistenceJDBCImpl.java | 119 ++++++++++-------- .../fhir/search/parameters/ParametersMap.java | 27 ++++ .../search/parameters/ParametersUtil.java | 44 ++++++- .../com/ibm/fhir/search/util/SearchUtil.java | 35 ++++++ 4 files changed, 169 insertions(+), 56 deletions(-) diff --git a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java index 08c248bed7d..a3c2e4f9d33 100644 --- a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java +++ b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java @@ -166,7 +166,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; @@ -223,7 +222,7 @@ public class FHIRPersistenceJDBCImpl implements FHIRPersistence, SchemaNameSuppl // Enable use of legacy whole-system search parameters for the search request private final boolean legacyWholeSystemSearchParamsEnabled; - + // A list of payload persistence responses in case we have a rollback to clean up private final List payloadPersistenceResponses = new ArrayList<>(); @@ -377,7 +376,7 @@ public SingleResourceResult create(FHIRPersistenceContex // Create the new Resource DTO instance. com.ibm.fhir.persistence.jdbc.dto.Resource resourceDTO = - createResourceDTO(updatedResource.getClass(), logicalId, newVersionNumber, lastUpdated, updatedResource, + createResourceDTO(updatedResource.getClass(), logicalId, newVersionNumber, lastUpdated, updatedResource, getResourcePayloadKeyFromContext(context)); // The DAO objects are now created on-the-fly (not expensive to construct) and @@ -447,7 +446,7 @@ private void doCachePrefill() throws FHIRPersistenceException { /** * Creates and returns a data transfer object (DTO) with the contents of the passed arguments. - * + * * @param resourceType * @param logicalId * @param newVersionNumber @@ -582,9 +581,9 @@ public SingleResourceResult update(FHIRPersistenceContex // Persist the Resource DTO. resourceDao.setPersistenceContext(context); ExtractedSearchParameters searchParameters = this.extractSearchParameters(resource, resourceDTO); - resourceDao.insert(resourceDTO, searchParameters.getParameters(), searchParameters.getParameterHashB64(), + resourceDao.insert(resourceDTO, searchParameters.getParameters(), searchParameters.getParameterHashB64(), parameterDao, context.getIfNoneMatch()); - + if (log.isLoggable(Level.FINE)) { if (resourceDTO.getInteractionStatus() == InteractionStatus.IF_NONE_MATCH_EXISTED) { log.fine("If-None-Match: Existing FHIR Resource '" + resourceDTO.getResourceType() + "/" + resourceDTO.getLogicalId() + "' id=" + resourceDTO.getId() @@ -967,12 +966,12 @@ private List runIncludeQuery(Class lrIds = includeDTOs.stream() .map(r -> Long.toString(r.getLogicalResourceId())).collect(Collectors.toSet()); Map> resultMap = queryResultMap.computeIfAbsent(iterationLevel, k -> new HashMap<>()); - + final String targetResourceType = SearchConstants.INCLUDE.equals(includeType) ? inclusionParm.getSearchParameterTargetType() : inclusionParm.getJoinResourceType(); Set resultLogicalResourceIds = resultMap.computeIfAbsent(targetResourceType, k -> new HashSet<>()); resultLogicalResourceIds.addAll(lrIds); - + // Because the resultLogicalResourceIds may contain resources of different types, we need // to make sure the resourceTypeId is properly marked on each DTO. We could've selected // that from the database, but we have the info here, so it's easy to inject it and avoid @@ -1022,7 +1021,7 @@ private FHIRPersistenceNotSupportedException buildNotSupportedException(String m } @Override - public void delete(FHIRPersistenceContext context, Class resourceType, String logicalId, int versionId, + public void delete(FHIRPersistenceContext context, Class resourceType, String logicalId, int versionId, com.ibm.fhir.model.type.Instant lastUpdated) throws FHIRPersistenceException { final String METHODNAME = "delete"; log.entering(CLASSNAME, METHODNAME); @@ -1428,7 +1427,7 @@ protected List buildSortedResourceDT * @throws FHIRPersistenceDBConnectException */ private List getResourceDTOs(ResourceDAO resourceDao, - Class resourceType, List sortedIdList, boolean includeResourceData) + Class resourceType, List sortedIdList, boolean includeResourceData) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException { return resourceDao.searchByIds(resourceType.getSimpleName(), sortedIdList, includeResourceData); @@ -1456,18 +1455,18 @@ protected List> convertResourceDTOList(Resour // TODO Linear fetch of a large number of resources will extend response times. Need // to look into batch or parallel fetch requests ResourceResult resourceResult = convertResourceDTOToResourceResult(resourceDTO, resourceType, elements, includeResourceData); - + // Check to make sure we got a Resource if we asked for it and expect there to be one if (resourceResult.getResource() == null && includeResourceData && !resourceResult.isDeleted()) { String resourceTypeName = getResourceTypeInfo(resourceDTO); if (resourceTypeName == null) { resourceTypeName = resourceType.getSimpleName(); } - throw new FHIRPersistenceException("convertResourceDTO returned no resource for '" + throw new FHIRPersistenceException("convertResourceDTO returned no resource for '" + resourceTypeName + "/" + resourceDTO.getLogicalId() + "'"); } - // Note that if the resource has been erased or was not fetched on purpose, + // Note that if the resource has been erased or was not fetched on purpose, // ResourceResult.resource will be null and the caller will need to take this // into account resourceResults.add(resourceResult); @@ -1813,24 +1812,42 @@ protected void addCompartmentParams(List allParameters, log.fine("Processing compartment parameters for resourceType: " + resourceType); } Map> compartmentRefParams = CompartmentUtil.getCompartmentParamsForResourceType(resourceType); - Map> compartmentMap = SearchUtil.extractCompartmentParameterValues(fhirResource, compartmentRefParams); + Map collectedEPV = allParameters.stream() + .collect(Collectors.toMap(epv -> epv.getName(), epv -> epv)); + + 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) { + if (log.isLoggable(Level.FINE)) { + log.fine("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 (Map.Entry> entry: compartmentMap.entrySet()) { - final String compartmentName = entry.getKey(); - final String parameterName = CompartmentUtil.makeCompartmentParamName(compartmentName); + 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 reference parameter value for each CompartmentReference extracted from the resource - for (CompartmentReference compartmentRef: entry.getValue()) { + // create a copy of the extracted parameter value but with the new internal compartment parameter name ReferenceParmVal pv = new ReferenceParmVal(); - pv.setName(parameterName); + pv.setName(internalCompartmentParamName); pv.setResourceType(resourceType); - - // 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); if (log.isLoggable(Level.FINE)) { - log.fine("Adding compartment reference parameter: [" + resourceType + "] "+ parameterName + " = " + rv.getTargetResourceType() + "/" + rv.getValue()); + log.fine("Adding compartment reference parameter: [" + resourceType + "] " + + internalCompartmentParamName + " = " + rv.getTargetResourceType() + "/" + rv.getValue()); } allParameters.add(pv); } @@ -1972,7 +1989,7 @@ public OperationOutcome getHealth() throws FHIRPersistenceException { throw fx; } } - + /** * Checks to make sure the installed schema matches the version we expect * @param connection @@ -1991,7 +2008,7 @@ private boolean checkSchemaIsCurrent(Connection connection) throws SQLException, // doesn't exist versionId = -1; } - + // compare what's in the database with the latest FhirSchemaVersion. For now, // we allow the database schema to be equal to or ahead of the latest schema known // to this instance. This helps with rolling deploys. @@ -2004,12 +2021,12 @@ private boolean checkSchemaIsCurrent(Connection connection) throws SQLException, result = false; // not supported - database needs to be updated } else if (versionId > latest.vid()) { // the database has been updated, but this is the old code still running - log.warning("Deployment update required: database schema version [" + versionId + log.warning("Deployment update required: database schema version [" + versionId + "] is newer than code schema version [" + latest.vid() + "]"); result = true; // this is OK } else if (versionId < latest.vid()) { // the code is newer than the database schema - log.severe("Schema update required: database schema version [" + versionId + log.severe("Schema update required: database schema version [" + versionId + "] is older than code schema version [" + latest.vid() + "]"); result = false; // not supported - database needs to be updated } else { @@ -2062,7 +2079,7 @@ protected List convertResourceDTOList(List T convertResourceDTO(com.ibm.fhir.persistence.jdbc. rowResourceTypeName = resourceType.getSimpleName(); resourceTypeId = getResourceTypeId(resourceType); } - + // If a specific version of a resource has been deleted using $erase, it // is possible for the result here to be null. result = payloadPersistence.readResource(resourceType, rowResourceTypeName, resourceTypeId, resourceDTO.getLogicalId(), resourceDTO.getVersionId(), resourceDTO.getResourcePayloadKey(), elements); @@ -2120,7 +2137,7 @@ private T convertResourceDTO(com.ibm.fhir.persistence.jdbc. // resource doesn't exist result = null; } - + log.exiting(CLASSNAME, METHODNAME); return result; } @@ -2131,7 +2148,7 @@ private ResourceResult convertResourceDTOToResourceResul log.entering(CLASSNAME, METHODNAME); Objects.requireNonNull(resourceDTO, "resourceDTO must be not null"); T resource; - + if (includeData) { if (this.payloadPersistence != null) { // The payload needs to be read from the FHIRPayloadPersistence impl. If this is @@ -2145,7 +2162,7 @@ private ResourceResult convertResourceDTOToResourceResul rowResourceTypeName = resourceType.getSimpleName(); resourceTypeId = getResourceTypeId(resourceType); } - + // If a specific version of a resource has been deleted using $erase, it // is possible for the result here to be null. resource = payloadPersistence.readResource(resourceType, rowResourceTypeName, resourceTypeId, resourceDTO.getLogicalId(), resourceDTO.getVersionId(), resourceDTO.getResourcePayloadKey(), elements); @@ -2175,7 +2192,7 @@ private ResourceResult convertResourceDTOToResourceResul } else { resource = null; } - + // Note that resource may be null. We return a ResourceResult so we can // communicate back the type/id/version information even if we didn't get // an actual resource object @@ -2192,7 +2209,7 @@ private ResourceResult convertResourceDTOToResourceResul builder.resource(resource); // can be null builder.version(resourceDTO.getVersionId()); builder.lastUpdated(resourceDTO.getLastUpdated().toInstant()); - + log.exiting(CLASSNAME, METHODNAME); return builder.build(); } @@ -2206,7 +2223,7 @@ private ResourceResult convertResourceDTOToResourceResul * but the value cannot be found in the cache * @return */ - private String getResourceTypeInfo(com.ibm.fhir.persistence.jdbc.dto.Resource resourceDTO) + private String getResourceTypeInfo(com.ibm.fhir.persistence.jdbc.dto.Resource resourceDTO) throws FHIRPersistenceException { final String result; // resource type name needs to be derived from the resourceTypeId returned by the DB select query @@ -2279,7 +2296,7 @@ private OperationOutcome buildOKOperationOutcome() { private OperationOutcome buildErrorOperationOutcome() { return FHIRUtil.buildOperationOutcome("The database connection was not valid", IssueType.NO_STORE, IssueSeverity.ERROR); } - + private OperationOutcome buildSchemaVersionErrorOperationOutcome() { return FHIRUtil.buildOperationOutcome("The database schema version is old", IssueType.NO_STORE, IssueSeverity.ERROR); } @@ -2566,7 +2583,7 @@ private ParameterTransactionDataImpl getTransactionDataForDatasource(String data return result; } - + /** * Callback from TransactionData when a transaction has been rolled back * @param payloadPersistenceResponses an immutable list of {@link PayloadPersistenceResponse} @@ -2581,7 +2598,7 @@ private void handleRollback() { for (PayloadPersistenceResponse ppr: payloadPersistenceResponses) { try { log.fine(() -> "tx rollback - deleting payload: " + ppr.toString()); - payloadPersistence.deletePayload(ppr.getResourceTypeName(), ppr.getResourceTypeId(), + payloadPersistence.deletePayload(ppr.getResourceTypeName(), ppr.getResourceTypeId(), ppr.getLogicalId(), ppr.getVersionId(), ppr.getResourcePayloadKey()); } catch (Exception x) { // Nothing more we can do other than log the issue. Any rows we can't process @@ -2591,7 +2608,7 @@ private void handleRollback() { log.log(Level.SEVERE, "rollback failed to delete payload: " + ppr.toString(), x); } } - + payloadPersistenceResponses.clear(); } @@ -2657,7 +2674,7 @@ public ResourcePayload fetchResourcePayloads(Class resourceT @Override public List changes(int resourceCount, java.time.Instant sinceLastModified, java.time.Instant beforeLastModified, - Long changeIdMarker, List resourceTypeNames, boolean excludeTransactionTimeoutWindow, HistorySortOrder historySortOrder) + Long changeIdMarker, List resourceTypeNames, boolean excludeTransactionTimeoutWindow, HistorySortOrder historySortOrder) throws FHIRPersistenceException { try (Connection connection = openConnection()) { doCachePrefill(connection); @@ -2672,7 +2689,7 @@ public List changes(int resourceCount, java.time.Instan resourceTypeIds = null; // no filter on resource type } IDatabaseTranslator translator = FHIRResourceDAOFactory.getTranslatorForFlavor(connectionStrategy.getFlavor()); - FetchResourceChangesDAO dao = new FetchResourceChangesDAO(translator, schemaNameSupplier.getSchemaForRequestContext(connection), + FetchResourceChangesDAO dao = new FetchResourceChangesDAO(translator, schemaNameSupplier.getSchemaForRequestContext(connection), resourceCount, sinceLastModified, beforeLastModified, changeIdMarker, resourceTypeIds, excludeTransactionTimeoutWindow, historySortOrder); return dao.run(connection); @@ -2695,11 +2712,11 @@ public ResourceEraseRecord erase(EraseDTO eraseDto) throws FHIRPersistenceExcept doCachePrefill(connection); IDatabaseTranslator translator = FHIRResourceDAOFactory.getTranslatorForFlavor(connectionStrategy.getFlavor()); IResourceReferenceDAO rrd = makeResourceReferenceDAO(connection); - EraseResourceDAO eraseDao = new EraseResourceDAO(connection, FhirSchemaConstants.FHIR_ADMIN, translator, - schemaNameSupplier.getSchemaForRequestContext(connection), + EraseResourceDAO eraseDao = new EraseResourceDAO(connection, FhirSchemaConstants.FHIR_ADMIN, translator, + schemaNameSupplier.getSchemaForRequestContext(connection), connectionStrategy.getFlavor(), this.cache, rrd); long eraseResourceGroupId = eraseDao.erase(eraseRecord, eraseDto); - + // If offloading is enabled, we need to remove the corresponding offloaded resource payloads if (isOffloadingSupported()) { erasePayloads(eraseDao, eraseResourceGroupId); @@ -2707,7 +2724,7 @@ public ResourceEraseRecord erase(EraseDTO eraseDto) throws FHIRPersistenceExcept // clean up the erased_resources records because they're no longer needed eraseDao.clearErasedResourcesInGroup(eraseResourceGroupId); } - + } catch(FHIRPersistenceResourceNotFoundException e) { throw e; } catch(FHIRPersistenceException e) { @@ -2750,7 +2767,7 @@ private void erasePayloads(EraseResourceDAO dao, long erasedResourceGroupId) thr for (ErasedResourceRec rec: recs) { erasePayload(rec); } - + // If the above loop completed without throwing an exception, we can safely // remove all the records in the group. If an exception was thrown (because // the offload persistence layer was not accessible), don't delete right now @@ -2768,7 +2785,7 @@ private void erasePayload(ErasedResourceRec rec) throws FHIRPersistenceException if (resourceType == null) { throw new FHIRPersistenceException("Resource type not found in cache for resourceTypeId=" + rec.getResourceTypeId()); } - + // Note that if versionId is null, it means delete all known versions // The resourcePayloadKey is always null here, because the intention // for erase is to delete all instances of the record (in the rare case @@ -2851,15 +2868,15 @@ public List readResourcesForRecords(List reco for (ResourceChangeLogRecord r: records) { Class resourceType = ModelSupport.getResourceType(r.getResourceTypeName()); Resource resource = readResourceForRecord(readContext, r, resourceType); - - // We add the resource even if it's null because we want to keep the + + // We add the resource even if it's null because we want to keep the // list in alignment with the records list. A null might be returned // if the resource has been erased (hard delete). result.add(resource); } return result; } - + /** * Read the resource version for the given ResourceChangeLogRecord * @param 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 541f5d56af7..20e18dd5f9f 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 @@ -30,6 +30,8 @@ public class ParametersMap { private final Map codeMap; private final Map canonicalMap; + private final Map inclusionParamMap; + /** * Construct a ParametersMap from a Bundle of SearchParameter */ @@ -37,6 +39,10 @@ public ParametersMap() { // LinkedHashMaps to preserve insertion order codeMap = new LinkedHashMap<>(); canonicalMap = new LinkedHashMap<>(); + + // Inclusion parameters are stored separately because they may be internal-only + // i.e. not externally searchable except through compartment search + inclusionParamMap = new LinkedHashMap<>(); } /** @@ -82,6 +88,23 @@ 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) { + 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); + } + inclusionParamMap.put(code, parameter); + } + private void logParamConflict(String distinguisher, SearchParameter parameter, String canonical, SearchParameter previous) { if (previous.getExpression().equals(parameter.getExpression())) { if (log.isLoggable(Level.FINE)) { @@ -117,6 +140,10 @@ public SearchParameter lookupByCanonical(String searchParameterCanonical) { return canonicalMap.get(searchParameterCanonical); } + public SearchParameter getInclusionParam(String searchParameterCode) { + return inclusionParamMap.get(searchParameterCode); + } + public Collection values() { // use List to preserve order return Collections.unmodifiableList(canonicalMap.entrySet().stream() diff --git a/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java b/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java index 78aecc0fd7b..a4240d344aa 100644 --- a/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java +++ b/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java @@ -29,6 +29,7 @@ import com.ibm.fhir.model.util.ModelSupport; import com.ibm.fhir.registry.FHIRRegistry; import com.ibm.fhir.search.SearchConstants; +import com.ibm.fhir.search.compartment.CompartmentUtil; import com.ibm.fhir.search.exception.SearchExceptionUtil; /** @@ -175,7 +176,9 @@ private static Map computeTenantSPs(PropertyGroup rsrcsGr } } - addWildcardParams(paramMapsByType, resourceTypesWithWildcardParams, configuredCodes); + addWildcardAndCompartmentParams(paramMapsByType, resourceTypesWithWildcardParams, configuredCodes); + +// addCompartmentParams(paramMapsByType, resourceTypesWithWildcardParams, configuredCodes); return Collections.unmodifiableMap(paramMapsByType); } @@ -192,6 +195,9 @@ private static ParametersMap getExplicitParams(Set resourceTypesWithWild if (spGroup != null) { List spEntries = spGroup.getProperties(); if (spEntries != null && !spEntries.isEmpty()) { + + Map> compartmentParamToCompartment = CompartmentUtil.getCompartmentParamsForResourceType(resourceType); + for (PropertyEntry spEntry : spEntries) { String code = spEntry.getName(); if (SearchConstants.WILDCARD.equals(code)) { @@ -201,8 +207,14 @@ private static ParametersMap getExplicitParams(Set resourceTypesWithWild .getResource((String)spEntry.getValue(), SearchParameter.class); if (sp != null) { paramMap.insert(code, sp); + + // If this param is an inclusion criteria for one or more compartments + if (compartmentParamToCompartment.containsKey(code)) { + paramMap.insertInclusionParam(code, sp, compartmentParamToCompartment.get(code)); + } } else { - log.warning("Search parameter '" + code + "' with the configured url '" + spEntry.getValue() + "' for resourceType '" + resourceType + "' could not be found."); + log.warning("Search parameter '" + code + "' with the configured url '" + spEntry.getValue() + + "' for resourceType '" + resourceType + "' could not be found."); } } } @@ -215,10 +227,11 @@ private static ParametersMap getExplicitParams(Set resourceTypesWithWild return paramMap; } - private static void addWildcardParams(Map paramMapsByType, + private static void addWildcardAndCompartmentParams(Map paramMapsByType, Set resourceTypesWithWildcardParams, Map> configuredCodes) { for (SearchParameter sp : getAllSearchParameters()) { + String code = sp.getCode().getValue(); // For each resource type this search parameter applies to for (ResourceType resourceType : sp.getBase()) { @@ -232,17 +245,38 @@ private static void addWildcardParams(Map paramMapsByType // Only add it if the code wasn't explicitly configured in fhir-server-config Set configuredCodesForType = configuredCodes.get(resourceType.getValue()); - if (configuredCodesForType != null && configuredCodesForType.contains(sp.getCode().getValue())) { + if (configuredCodesForType != null && configuredCodesForType.contains(code)) { if (log.isLoggable(Level.FINE)) { String canonical = getCanonicalUrl(sp); log.fine("Skipping search parameter '" + canonical + "' because code '" + sp.getCode().getValue() + "' is already configured."); } } else { - paramMap.insert(sp.getCode().getValue(), sp); + paramMap.insert(code, sp); } } + // If this param is an inclusion criteria for one or more compartments + Map> compartmentParamToCompartment = CompartmentUtil.getCompartmentParamsForResourceType(resourceType.getValue()); + if (compartmentParamToCompartment.containsKey(code)) { + ParametersMap paramMap = paramMapsByType.get(resourceType.getValue()); + if (paramMap == null) { + paramMap = new ParametersMap(); + paramMapsByType.put(resourceType.getValue(), paramMap); + } + + // Only add it if the code wasn't explicitly configured in fhir-server-config + Set configuredCodesForType = configuredCodes.get(resourceType.getValue()); + if (configuredCodesForType != null && configuredCodesForType.contains(code)) { + if (log.isLoggable(Level.FINE)) { + String canonical = getCanonicalUrl(sp); + log.fine("Skipping compartment inclusion parameter '" + canonical + "' because code '" + + sp.getCode().getValue() + "' is already configured."); + } + } else { + paramMap.insertInclusionParam(code, sp, compartmentParamToCompartment.get(code)); + } + } } } } 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 130da8f15cd..3b4a4334bc3 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 @@ -159,6 +159,33 @@ 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 @@ -319,6 +346,14 @@ public static Map> extractParameterValues(Re Map parameters = getSearchParameters(resourceType.getSimpleName()); + for (String inclusionParamName : CompartmentUtil.getCompartmentParamsForResourceType(resourceType.getSimpleName()).keySet()) { + if (!parameters.containsKey(inclusionParamName)) { + String tenantId = FHIRRequestContext.get().getTenantId(); + ParametersMap parametersMap = ParametersUtil.getTenantSPs(tenantId).get(resourceType.getSimpleName()); + parameters.put(inclusionParamName, parametersMap.getInclusionParam(inclusionParamName)); + } + } + for (Entry parameterEntry : parameters.entrySet()) { String code = parameterEntry.getKey(); SearchParameter parameter = parameterEntry.getValue(); From 21eeb1155a37e9b365c34d3d9f7ace01a76df6cd Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Mon, 21 Feb 2022 16:13:28 -0500 Subject: [PATCH 2/7] 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 +- .../reference/value/CompartmentReference.java | 78 ----------- .../com/ibm/fhir/search/util/SearchUtil.java | 132 +----------------- 7 files changed, 88 insertions(+), 265 deletions(-) delete mode 100644 fhir-search/src/main/java/com/ibm/fhir/search/reference/value/CompartmentReference.java 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/reference/value/CompartmentReference.java b/fhir-search/src/main/java/com/ibm/fhir/search/reference/value/CompartmentReference.java deleted file mode 100644 index de214e59289..00000000000 --- a/fhir-search/src/main/java/com/ibm/fhir/search/reference/value/CompartmentReference.java +++ /dev/null @@ -1,78 +0,0 @@ -/* - * (C) Copyright IBM Corp. 2020 - * - * SPDX-License-Identifier: Apache-2.0 - */ - -package com.ibm.fhir.search.reference.value; - -import java.util.Objects; - -/** - * Represents a reference to a resource compartment extracted by SearchUtil - */ -public class CompartmentReference { - - private final String parameterName; - - private final String referenceResourceType; - - private final String referenceResourceValue; - - /** - * Public constructor - * @param parameterName - * @param referenceResourceType - * @param referenceResourceValue - */ - public CompartmentReference(String parameterName, String referenceResourceType, String referenceResourceValue) { - Objects.requireNonNull(parameterName, "parameterName"); - Objects.requireNonNull(referenceResourceType, "referenceResourceType"); - Objects.requireNonNull(referenceResourceValue, "referenceResourceValue"); - this.parameterName = parameterName; - this.referenceResourceType = referenceResourceType; - this.referenceResourceValue = referenceResourceValue; - } - - @Override - public int hashCode() { - int prime = 31; - return parameterName.hashCode() + prime * (referenceResourceType.hashCode() + prime * referenceResourceValue.hashCode()); - } - - @Override - public boolean equals(Object obj) { - if (obj instanceof CompartmentReference) { - CompartmentReference that = (CompartmentReference)obj; - return this.parameterName.equals(that.parameterName) - && this.referenceResourceType.equals(that.referenceResourceType) - && this.referenceResourceValue.equals(that.referenceResourceValue); - } else { - return false; - } - } - - - /** - * @return the parameterName - */ - public String getParameterName() { - return parameterName; - } - - - /** - * @return the referenceResourceType - */ - public String getReferenceResourceType() { - return referenceResourceType; - } - - - /** - * @return the referenceResourceValue - */ - public String getReferenceResourceValue() { - return referenceResourceValue; - } -} 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. * From 20fc22d28e738e1c5a04c8bdf08856a112cc436c Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Tue, 22 Feb 2022 16:06:51 -0500 Subject: [PATCH 3/7] issue #1777 - added tests and minor refactor 1. added simple unit tests for the new ParametersMap behavior 2. added single e2e test to SearchTest to cover the case where the compartment inclusion criteria parameter is filtered out in the config Also, we now add the DO_NOT_STORE extension from SearchUtil instead of as the parameters are added to the ParametersMap Signed-off-by: Lee Surprenant --- .../com/ibm/fhir/model/test/TestUtil.java | 21 +- .../com/ibm/fhir/search/SearchConstants.java | 5 + .../fhir/search/parameters/ParametersMap.java | 19 +- .../search/parameters/ParametersUtil.java | 14 +- .../com/ibm/fhir/search/util/SearchUtil.java | 5 +- .../search/parameters/ParametersMapTest.java | 261 ++++++++++-------- .../com/ibm/fhir/server/test/SearchTest.java | 53 +++- 7 files changed, 247 insertions(+), 131 deletions(-) diff --git a/fhir-model/src/test/java/com/ibm/fhir/model/test/TestUtil.java b/fhir-model/src/test/java/com/ibm/fhir/model/test/TestUtil.java index c727c0a98bf..09417818f96 100644 --- a/fhir-model/src/test/java/com/ibm/fhir/model/test/TestUtil.java +++ b/fhir-model/src/test/java/com/ibm/fhir/model/test/TestUtil.java @@ -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; @@ -117,12 +116,26 @@ 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 + * the specified patient via a subject attribute. + */ + 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; } 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 cc70d0dac72..35b105af567 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 @@ -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; @@ -103,6 +104,10 @@ private SearchConstants() { 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"; 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 c0f739bbda5..3040d6169af 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 @@ -17,8 +17,6 @@ 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 @@ -29,11 +27,6 @@ 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; @@ -108,10 +101,6 @@ public void insertInclusionParam(String code, SearchParameter parameter) { 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); } @@ -174,6 +163,14 @@ public Set> codeEntries() { return Collections.unmodifiableSet(codeMap.entrySet()); } + public Collection inclusionValues() { + // use List to preserve order + return Collections.unmodifiableList(inclusionParamMap.entrySet().stream() + .filter(e -> !e.getKey().contains("|")) + .map(e -> e.getValue()) + .collect(Collectors.toList())); + } + /** * @implSpec Note that versioned search parameters will be listed twice; * once with their version and once without diff --git a/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java b/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java index a7a47a7b3f4..aadad18521a 100644 --- a/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java +++ b/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java @@ -61,9 +61,10 @@ public final class ParametersUtil { // Logging: public static final String LOG_PARAMETERS = "Parameter is loaded -> %s"; - public static final String LOG_HEADER = "BASE:RESOURCE_NAME:SearchParameter"; - public static final String LOG_SIZE = "Size: %s"; - private static final String LOG_OUTPUT = "%s|%s|%s"; + public static final String LOG_HEADER = "ResourceType | SearchParameter.code | SearchParameter.expression\n" + + "'*' denotes compartment inclusion criteria parameters"; + public static final String LOG_SIZE = "Number of resource types: %s"; + private static final String LOG_OUTPUT = "%s | %s | %s"; private static final String LEFT = "["; private static final String RIGHT = "]"; @@ -370,6 +371,13 @@ private static void print(PrintStream out, Map searchPara } out.println(String.format(LOG_OUTPUT, base, param.getCode().getValue(), expression)); } + for(SearchParameter param : tmp.inclusionValues()) { + String expression = MISSING_EXPRESSION; + if (param.getExpression() != null) { + expression = param.getExpression().getValue(); + } + out.println(String.format(LOG_OUTPUT, "*" + base, param.getCode().getValue(), expression)); + } out.println(SearchConstants.LOG_BOUNDARY); } out.println(SearchConstants.LOG_BOUNDARY); 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 8700ef49f34..61a266376c4 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 @@ -318,7 +318,10 @@ public static Map> extractParameterValues(Re 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)); + SearchParameter inclusionParam = parametersMap.getInclusionParam(inclusionParamName).toBuilder() + .extension(SearchConstants.DO_NOT_STORE_EXT) + .build(); + parameters.put(inclusionParamName, inclusionParam); } } diff --git a/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersMapTest.java b/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersMapTest.java index 4d652bb54bf..3d19b628d99 100644 --- a/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersMapTest.java +++ b/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersMapTest.java @@ -1,5 +1,5 @@ /* - * (C) Copyright IBM Corp. 2021 + * (C) Copyright IBM Corp. 2021, 2022 * * SPDX-License-Identifier: Apache-2.0 */ @@ -19,114 +19,155 @@ import com.ibm.fhir.model.type.code.ResourceType; import com.ibm.fhir.model.type.code.SearchParamType; +/** + * ParametersMap tests + */ public class ParametersMapTest { - SearchParameter sp_a1 = SearchParameter.builder() - .url(Uri.of("http://ibm.com/fhir/test/sp_a1")) - .status(PublicationStatus.ACTIVE) - .name(string("a")) - .description(Markdown.of("First param with code 'a'")) - .base(ResourceType.RESOURCE) - .type(SearchParamType.STRING) - .code(Code.of("a")) - .expression(string("extension.value as String")) - .build(); - SearchParameter sp_a2 = SearchParameter.builder() - .url(Uri.of("http://ibm.com/fhir/test/sp_a2")) - .status(PublicationStatus.ACTIVE) - .name(string("a")) - .description(Markdown.of("Second param with code 'a'")) - .base(ResourceType.RESOURCE) - .type(SearchParamType.STRING) - .code(Code.of("a")) - .expression(string("extension.value as String")) - .build(); - SearchParameter sp_b = SearchParameter.builder() - .url(Uri.of("http://ibm.com/fhir/test/sp_b")) - .status(PublicationStatus.ACTIVE) - .name(string("b")) - .base(ResourceType.RESOURCE) - .type(SearchParamType.STRING) - .description(Markdown.of("Param with code 'b'")) - .code(Code.of("b")) - .expression(string("extension.value as String")) - .build(); - SearchParameter sp_b1 = SearchParameter.builder() - .url(Uri.of("http://ibm.com/fhir/test/sp_b")) - .version("1") - .status(PublicationStatus.ACTIVE) - .name(string("b")) - .base(ResourceType.RESOURCE) - .type(SearchParamType.STRING) - .description(Markdown.of("Param with code 'b'")) - .code(Code.of("b")) - .expression(string("extension.value as String")) - .build(); - SearchParameter sp_b2 = SearchParameter.builder() - .url(Uri.of("http://ibm.com/fhir/test/sp_b")) - .version("2") - .status(PublicationStatus.ACTIVE) - .name(string("b")) - .base(ResourceType.RESOURCE) - .type(SearchParamType.STRING) - .description(Markdown.of("Param with code 'b'")) - .code(Code.of("b")) - .expression(string("extension.value as String")) - .build(); - - @Test - public void testParametersMap() { - ParametersMap pm = new ParametersMap(); - pm.insert("a", sp_a1); - pm.insert("a", sp_a2); - pm.insert("b", sp_b); - pm.insert("b", sp_b1); - pm.insert("b", sp_b2); - - assertEquals(pm.lookupByCode("a"), sp_a2); - assertEquals(pm.lookupByCode("b"), sp_b2); - - assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_a1"), sp_a1); - assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_a2"), sp_a2); - assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b"), sp_b2); - assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b|1"), sp_b1); - assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b|2"), sp_b2); - - assertTrue(pm.codeEntries().size() == 2); - assertTrue(pm.canonicalEntries().size() == 5); // versionless ones for a1, a2, and b2; versioned ones for b1 and b2 - } - - /** - * Adding the same parameter under two different codes should be supported - */ - @Test - public void testParametersMap_duplicateUrl() { - ParametersMap pm = new ParametersMap(); - pm.insert("b", sp_b); - pm.insert("b2", sp_b); - - assertEquals(pm.lookupByCode("b"), sp_b); - assertEquals(pm.lookupByCode("b2"), sp_b); - - assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b"), sp_b); - - assertTrue(pm.codeEntries().size() == 2); - assertTrue(pm.canonicalEntries().size() == 1); - } - - /** - * Adding the same parameter twice should be the same as adding it once - */ - @Test - public void testParametersMap_duplicate() { - ParametersMap pm = new ParametersMap(); - pm.insert("b", sp_b); - pm.insert("b", sp_b); - - assertEquals(pm.lookupByCode("b"), sp_b); - - assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b"), sp_b); - - assertTrue(pm.codeEntries().size() == 1); - assertTrue(pm.canonicalEntries().size() == 1); - } + SearchParameter sp_a1 = SearchParameter.builder() + .url(Uri.of("http://ibm.com/fhir/test/sp_a1")) + .status(PublicationStatus.ACTIVE) + .name(string("a")) + .description(Markdown.of("First param with code 'a'")) + .base(ResourceType.RESOURCE) + .type(SearchParamType.STRING) + .code(Code.of("a")) + .expression(string("extension.value as String")) + .build(); + SearchParameter sp_a2 = SearchParameter.builder() + .url(Uri.of("http://ibm.com/fhir/test/sp_a2")) + .status(PublicationStatus.ACTIVE) + .name(string("a")) + .description(Markdown.of("Second param with code 'a'")) + .base(ResourceType.RESOURCE) + .type(SearchParamType.STRING) + .code(Code.of("a")) + .expression(string("extension.value as String")) + .build(); + SearchParameter sp_b = SearchParameter.builder() + .url(Uri.of("http://ibm.com/fhir/test/sp_b")) + .status(PublicationStatus.ACTIVE) + .name(string("b")) + .base(ResourceType.RESOURCE) + .type(SearchParamType.STRING) + .description(Markdown.of("Param with code 'b'")) + .code(Code.of("b")) + .expression(string("extension.value as String")) + .build(); + SearchParameter sp_b1 = SearchParameter.builder() + .url(Uri.of("http://ibm.com/fhir/test/sp_b")) + .version("1") + .status(PublicationStatus.ACTIVE) + .name(string("b")) + .base(ResourceType.RESOURCE) + .type(SearchParamType.STRING) + .description(Markdown.of("Param with code 'b'")) + .code(Code.of("b")) + .expression(string("extension.value as String")) + .build(); + SearchParameter sp_b2 = SearchParameter.builder() + .url(Uri.of("http://ibm.com/fhir/test/sp_b")) + .version("2") + .status(PublicationStatus.ACTIVE) + .name(string("b")) + .base(ResourceType.RESOURCE) + .type(SearchParamType.STRING) + .description(Markdown.of("Param with code 'b'")) + .code(Code.of("b")) + .expression(string("extension.value as String")) + .build(); + + /** + * Add various versions of search parameters 'a' and 'b' and check the results + */ + @Test + public void testParametersMap() { + ParametersMap pm = new ParametersMap(); + pm.insert("a", sp_a1); + pm.insert("a", sp_a2); + pm.insert("b", sp_b); + pm.insert("b", sp_b1); + pm.insert("b", sp_b2); + + assertEquals(pm.lookupByCode("a"), sp_a2); + assertEquals(pm.lookupByCode("b"), sp_b2); + + assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_a1"), sp_a1); + assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_a2"), sp_a2); + assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b"), sp_b2); + assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b|1"), sp_b1); + assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b|2"), sp_b2); + + assertTrue(pm.codeEntries().size() == 2); + assertTrue(pm.canonicalEntries().size() == 5); // versionless ones for a1, a2, and b2; versioned ones for b1 and b2 + } + + /** + * Adding the same parameter under two different codes should be supported + */ + @Test + public void testParametersMap_duplicateUrl() { + ParametersMap pm = new ParametersMap(); + pm.insert("b", sp_b); + pm.insert("b2", sp_b); + + assertEquals(pm.lookupByCode("b"), sp_b); + assertEquals(pm.lookupByCode("b2"), sp_b); + + assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b"), sp_b); + + assertTrue(pm.codeEntries().size() == 2); + assertTrue(pm.canonicalEntries().size() == 1); + } + + /** + * Adding the same parameter twice should be the same as adding it once + */ + @Test + public void testParametersMap_duplicate() { + ParametersMap pm = new ParametersMap(); + pm.insert("b", sp_b); + pm.insert("b", sp_b); + + assertEquals(pm.lookupByCode("b"), sp_b); + + assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b"), sp_b); + + assertTrue(pm.codeEntries().size() == 1); + assertTrue(pm.canonicalEntries().size() == 1); + } + + /** + * Add various versions of compartment inclusion parameters 'a' and 'b' and check the results + */ + @Test + public void testParametersMap_inclusionParams() { + ParametersMap pm = new ParametersMap(); + pm.insertInclusionParam("a", sp_a1); + pm.insertInclusionParam("a", sp_a2); + pm.insertInclusionParam("b", sp_b); + pm.insertInclusionParam("b", sp_b1); + pm.insertInclusionParam("b", sp_b2); + + assertEquals(pm.getInclusionParam("a"), sp_a2); + assertEquals(pm.getInclusionParam("b"), sp_b2); + + assertTrue(pm.inclusionValues().size() == 2); + } + + /** + * Adding the same inclusion parameter twice should be the same as adding it once + */ + @Test + public void testParametersMap_inclusionParams_duplicate() { + ParametersMap pm = new ParametersMap(); + pm.insert("b", sp_b); + pm.insert("b", sp_b); + + assertEquals(pm.lookupByCode("b"), sp_b); + + assertEquals(pm.lookupByCanonical("http://ibm.com/fhir/test/sp_b"), sp_b); + + assertTrue(pm.codeEntries().size() == 1); + assertTrue(pm.canonicalEntries().size() == 1); + } } diff --git a/fhir-server-test/src/test/java/com/ibm/fhir/server/test/SearchTest.java b/fhir-server-test/src/test/java/com/ibm/fhir/server/test/SearchTest.java index f9d4f4fe094..162cc3a34de 100644 --- a/fhir-server-test/src/test/java/com/ibm/fhir/server/test/SearchTest.java +++ b/fhir-server-test/src/test/java/com/ibm/fhir/server/test/SearchTest.java @@ -655,12 +655,12 @@ public void testCreateObservationWithRange() throws Exception { TestUtil.assertResourceEquals(observation, responseObservation); } - @Test(groups = { "server-search" }, dependsOnMethods = {"testCreatePatient" }) + @Test(groups = { "server-search" }, dependsOnMethods = {"testCreatePatient", "testCreatePractitioner"}) public void testCreateObservation() throws Exception { WebTarget target = getWebTarget(); Observation observation = - TestUtil.buildPatientObservation(patientId, "Observation1.json"); + TestUtil.buildPatientObservation(patientId, practitionerId, "Observation1.json"); Entity entity = Entity.entity(observation, FHIRMediaType.APPLICATION_FHIR_JSON); Response response = @@ -923,6 +923,11 @@ public void testSearchPatientWithObservationRevIncluded() { + patientId, observation.getSubject().getReference().getValue()); } + /** + * 'patient' is a configured Observation search parameter for tenant1, + * so this tests that compartment info is extracted and searchable + * when the related parameter is not filtered out + */ @Test(groups = { "server-search" }, dependsOnMethods = { "testCreateObservation", "retrieveConfig" }) public void testSearchObservationWithPatientCompartment() { @@ -968,6 +973,50 @@ public void testSearchObservationWithPatientCompartmentViaPost() { assertTrue(bundle.getEntry().size() >= 1); } + /** + * 'performer' is not a configured Observation search parameter for tenant1, + * so this tests that compartment info is still extracted and searchable + * when the related parameter is filtered + */ + @Test(groups = { "server-search" }, dependsOnMethods = { + "testCreateObservation", "retrieveConfig" }) + public void testSearchObservationWithPractitionerCompartment() { + assertNotNull(compartmentSearchSupported); + if (!compartmentSearchSupported.booleanValue()) { + return; + } + + WebTarget target = getWebTarget(); + String targetUri = "Practitioner/" + practitionerId + "/Observation"; + Response response = + target.path(targetUri).request(FHIRMediaType.APPLICATION_FHIR_JSON) + .header("X-FHIR-TENANT-ID", tenantName) + .header("X-FHIR-DSID", dataStoreId) + .get(); + assertResponse(response, Response.Status.OK.getStatusCode()); + Bundle bundle = response.readEntity(Bundle.class); + assertNotNull(bundle); + assertTrue(bundle.getEntry().size() >= 1); + // verify link does not include consecutive '&' characters + assertTrue(bundle.getLink().size() >= 1); + assertFalse(bundle.getLink().get(0).getUrl().getValue().contains("&&")); + + // Also verify a negative search + targetUri = "Practitioner/" + practitionerId2 + "/Observation"; + response = + target.path(targetUri).request(FHIRMediaType.APPLICATION_FHIR_JSON) + .header("X-FHIR-TENANT-ID", tenantName) + .header("X-FHIR-DSID", dataStoreId) + .get(); + assertResponse(response, Response.Status.OK.getStatusCode()); + bundle = response.readEntity(Bundle.class); + assertNotNull(bundle); + assertTrue(bundle.getEntry().size() == 0); + // verify link does not include consecutive '&' characters + assertTrue(bundle.getLink().size() >= 1); + assertFalse(bundle.getLink().get(0).getUrl().getValue().contains("&&")); + } + @Test(groups = { "server-search" }, dependsOnMethods = {"testCreateObservation" }) public void test_SearchObservationWithSubject() throws Exception { FHIRParameters parameters = new FHIRParameters(); From 79250349a0e4a3b064aa341442e72552dae61a26 Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Tue, 22 Feb 2022 17:36:23 -0500 Subject: [PATCH 4/7] issue #3283 - set ThreadLocal as part of FHIRRequestContext.get wrapper Signed-off-by: Lee Surprenant --- .../main/java/com/ibm/fhir/config/FHIRRequestContext.java | 5 +++++ .../search/location/NearLocationHandlerBoundingBoxTest.java | 2 ++ .../search/parameters/MultiTenantSearchParameterTest.java | 2 +- 3 files changed, 8 insertions(+), 1 deletion(-) diff --git a/fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java b/fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java index dbb2c7ab549..7c651199a04 100644 --- a/fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java +++ b/fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java @@ -177,12 +177,17 @@ 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(); if (log.isLoggable(Level.FINEST)) { log.finest("FHIRRequestContext.get: " + result.toString()); } + // Java ThreadLocal initial values are *not* automatically associated with the current thread + // so we need to explicitly set that here + contexts.set(result); return result; } diff --git a/fhir-search/src/test/java/com/ibm/fhir/search/location/NearLocationHandlerBoundingBoxTest.java b/fhir-search/src/test/java/com/ibm/fhir/search/location/NearLocationHandlerBoundingBoxTest.java index abbb260b0b0..07bd24bef76 100644 --- a/fhir-search/src/test/java/com/ibm/fhir/search/location/NearLocationHandlerBoundingBoxTest.java +++ b/fhir-search/src/test/java/com/ibm/fhir/search/location/NearLocationHandlerBoundingBoxTest.java @@ -7,6 +7,7 @@ package com.ibm.fhir.search.location; import static org.testng.Assert.assertEquals; +import static org.testng.Assert.assertFalse; import static org.testng.Assert.assertNotNull; import java.lang.reflect.Method; @@ -172,6 +173,7 @@ public void testLocationBoundaryPositionsFromParameters_tenant_near() throws Exc NearLocationHandler handler = new NearLocationHandler(); List bounding = handler.generateLocationPositionsFromParameters(ctx.getSearchParameters()); assertNotNull(bounding); + assertFalse(bounding.isEmpty(), "generateLocationPositionsFromParameters came back empty"); BoundingBox boundingBox = (BoundingBox) bounding.get(0); // At the low latitudes it's going to cover most of the area. diff --git a/fhir-search/src/test/java/com/ibm/fhir/search/parameters/MultiTenantSearchParameterTest.java b/fhir-search/src/test/java/com/ibm/fhir/search/parameters/MultiTenantSearchParameterTest.java index 0235d92c09c..249dad980b0 100644 --- a/fhir-search/src/test/java/com/ibm/fhir/search/parameters/MultiTenantSearchParameterTest.java +++ b/fhir-search/src/test/java/com/ibm/fhir/search/parameters/MultiTenantSearchParameterTest.java @@ -37,7 +37,7 @@ public class MultiTenantSearchParameterTest extends BaseSearchTest { @AfterMethod public void cleanup() throws FHIRException { // Restore the threadLocal FHIRRequestContext to the default tenant - FHIRRequestContext.get().setTenantId("default"); + FHIRRequestContext.remove(); } @Test From eec15d5cc40fcaa091d5e50c455484a3b37cd9d1 Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Tue, 22 Feb 2022 18:16:54 -0500 Subject: [PATCH 5/7] Attempt to compute search parameter config if missing in ParametersUtil Currently, if a tenant gets added / fixed it requires a server restart to take effect. With these changes, we'll log a warning and then attempt to compute the missing tenant search parameter config. Its still computed only once, but this should enable better support for dynamically adding new tenants. Additionally, ParametersSearchUtilTest and MultiTenantSearchParameterTest now use an 'extended' tenant to test search-parameter-extensions (instead of 'default') so-as to avoid conflicts with other tests. Signed-off-by: Lee Surprenant --- .../ibm/fhir/config/FHIRRequestContext.java | 2 +- .../com/ibm/fhir/model/test/TestUtil.java | 5 ++- .../search/parameters/ParametersUtil.java | 15 +++++++-- .../NearLocationHandlerBoundingBoxTest.java | 2 +- .../MultiTenantSearchParameterTest.java | 10 +++--- .../parameters/ParametersSearchUtilTest.java | 10 +++--- .../search/parameters/ParametersUtilTest.java | 4 +-- .../ibm/fhir/search/test/BaseSearchTest.java | 33 +++++++++---------- .../extension-search-parameters.json | 0 .../fhir-server-config.json | 0 10 files changed, 45 insertions(+), 36 deletions(-) rename fhir-search/src/test/resources/config/{default => extended}/extension-search-parameters.json (100%) rename fhir-search/src/test/resources/config/{default => extended}/fhir-server-config.json (100%) diff --git a/fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java b/fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java index 7c651199a04..286ddfd0f67 100644 --- a/fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java +++ b/fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java @@ -1,5 +1,5 @@ /* - * (C) Copyright IBM Corp. 2017, 2021 + * (C) Copyright IBM Corp. 2017, 2022 * * SPDX-License-Identifier: Apache-2.0 */ diff --git a/fhir-model/src/test/java/com/ibm/fhir/model/test/TestUtil.java b/fhir-model/src/test/java/com/ibm/fhir/model/test/TestUtil.java index 09417818f96..0c299b7540f 100644 --- a/fhir-model/src/test/java/com/ibm/fhir/model/test/TestUtil.java +++ b/fhir-model/src/test/java/com/ibm/fhir/model/test/TestUtil.java @@ -127,7 +127,10 @@ public static Observation buildPatientObservation(String patientId, String fileN /** * Loads an Observation resource from the specified file, then associates it with - * the specified patient via a subject attribute. + *
    + *
  • the specified patient via Observation.subject + *
  • the specified practitioner via Observation.performer + *
*/ public static Observation buildPatientObservation(String patientId, String practitionerId, String fileName) throws Exception { Observation observation = readLocalResource(fileName); diff --git a/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java b/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java index aadad18521a..a7fce1cdc9e 100644 --- a/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java +++ b/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java @@ -131,7 +131,7 @@ private static Map> buildSearchParameterMaps( } } - return Collections.unmodifiableMap(result); + return result; } private static Map computeTenantSPs(PropertyGroup rsrcsGroup) throws Exception { @@ -301,8 +301,17 @@ public static Map getTenantSPs(String tenantId) { if (searchParameters.containsKey(tenantId)) { return searchParameters.get(tenantId); } else { - log.warning("No search parameter configuration was loaded for tenant " + tenantId); - return new HashMap<>(); + log.warning("No search parameter configuration was loaded for tenant " + tenantId + "; computing now"); + try { + PropertyGroup root = FHIRConfiguration.getInstance().loadConfigurationForTenant(tenantId); + PropertyGroup rsrcsGroup = root == null ? null : root.getPropertyGroup(FHIRConfiguration.PROPERTY_RESOURCES); + Map tenantSPs = computeTenantSPs(rsrcsGroup); + searchParameters.put(tenantId, tenantSPs); + return tenantSPs; + } catch (Exception e) { + log.log(Level.SEVERE, "Error while computing search parameters for tenant " + tenantId + "; returning empty", e); + return new HashMap<>(); + } } } diff --git a/fhir-search/src/test/java/com/ibm/fhir/search/location/NearLocationHandlerBoundingBoxTest.java b/fhir-search/src/test/java/com/ibm/fhir/search/location/NearLocationHandlerBoundingBoxTest.java index 07bd24bef76..ca8c8b066e4 100644 --- a/fhir-search/src/test/java/com/ibm/fhir/search/location/NearLocationHandlerBoundingBoxTest.java +++ b/fhir-search/src/test/java/com/ibm/fhir/search/location/NearLocationHandlerBoundingBoxTest.java @@ -173,7 +173,7 @@ public void testLocationBoundaryPositionsFromParameters_tenant_near() throws Exc NearLocationHandler handler = new NearLocationHandler(); List bounding = handler.generateLocationPositionsFromParameters(ctx.getSearchParameters()); assertNotNull(bounding); - assertFalse(bounding.isEmpty(), "generateLocationPositionsFromParameters came back empty"); + assertFalse(bounding.isEmpty(), "generateLocationPositionsFromParameters came back empty for tenant " + FHIRRequestContext.get().getTenantId()); BoundingBox boundingBox = (BoundingBox) bounding.get(0); // At the low latitudes it's going to cover most of the area. diff --git a/fhir-search/src/test/java/com/ibm/fhir/search/parameters/MultiTenantSearchParameterTest.java b/fhir-search/src/test/java/com/ibm/fhir/search/parameters/MultiTenantSearchParameterTest.java index 249dad980b0..91e365c4713 100644 --- a/fhir-search/src/test/java/com/ibm/fhir/search/parameters/MultiTenantSearchParameterTest.java +++ b/fhir-search/src/test/java/com/ibm/fhir/search/parameters/MultiTenantSearchParameterTest.java @@ -1,5 +1,5 @@ /* - * (C) Copyright IBM Corp. 2019, 2021 + * (C) Copyright IBM Corp. 2019, 2022 * * SPDX-License-Identifier: Apache-2.0 */ @@ -68,8 +68,8 @@ public void testGetApplicableSearchParameters1() throws Exception { public void testGetApplicableSearchParameters3() throws Exception { // Looking for built-in and tenant-specific search parameters for "Patient" and "Observation". - // Use the default tenant since it has some Patient search parameters defined. - FHIRRequestContext.get().setTenantId("default"); + // Use the extended tenant since it has some Patient search parameters defined. + FHIRRequestContext.get().setTenantId("extended"); Map result = SearchUtil.getSearchParameters("Patient"); assertNotNull(result); @@ -141,8 +141,8 @@ public void testGetApplicableSearchParameters6() throws Exception { @Test public void testGetApplicableSearchParameters7() throws Exception { - // Test filtering of search parameters for Patient (default tenant). - FHIRRequestContext.get().setTenantId("default"); + // Test filtering of search parameters for Patient (extended tenant). + FHIRRequestContext.get().setTenantId("extended"); Map result = SearchUtil.getSearchParameters("Patient"); assertNotNull(result); diff --git a/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersSearchUtilTest.java b/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersSearchUtilTest.java index a5f65c0e837..76915fb82ab 100644 --- a/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersSearchUtilTest.java +++ b/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersSearchUtilTest.java @@ -1,5 +1,5 @@ /* - * (C) Copyright IBM Corp. 2019, 2021 + * (C) Copyright IBM Corp. 2019, 2022 * * SPDX-License-Identifier: Apache-2.0 */ @@ -49,9 +49,9 @@ public void testGetSearchParameters1Default() throws Exception { @Test public void testGetSearchParameters2Default() throws Exception { // Looking for built-in and tenant-specific search parameters for "Patient" - // and "Observation". Use the default tenant since it has some Patient search + // and "Observation". Use the extended tenant since it has some Patient search // parameters defined. - FHIRRequestContext.get().setTenantId("default"); + FHIRRequestContext.get().setTenantId("extended"); Map result = SearchUtil.getSearchParameters("Patient"); assertNotNull(result); @@ -128,8 +128,8 @@ public void testGetSearchParameters5Tenant() throws Exception { @Test public void testGetSearchParameters6Default() throws Exception { - // Test filtering of search parameters for Patient (default tenant). - FHIRRequestContext.get().setTenantId("default"); + // Test filtering of search parameters for Patient (extended tenant). + FHIRRequestContext.get().setTenantId("extended"); Map result = SearchUtil.getSearchParameters("Patient"); assertNotNull(result); diff --git a/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersUtilTest.java b/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersUtilTest.java index d3c9600f2d2..b645208513b 100644 --- a/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersUtilTest.java +++ b/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersUtilTest.java @@ -1,5 +1,5 @@ /* - * (C) Copyright IBM Corp. 2019, 2021 + * (C) Copyright IBM Corp. 2019, 2022 * * SPDX-License-Identifier: Apache-2.0 */ @@ -49,7 +49,7 @@ public void testGetAllSearchParameters() throws IOException { ParametersUtil.print(out); Assert.assertNotNull(outBA); } - assertEquals(params.size(), 1385); + assertEquals(params.size(), 1379); } @Test diff --git a/fhir-search/src/test/java/com/ibm/fhir/search/test/BaseSearchTest.java b/fhir-search/src/test/java/com/ibm/fhir/search/test/BaseSearchTest.java index 311daebafe8..5cb7707183b 100644 --- a/fhir-search/src/test/java/com/ibm/fhir/search/test/BaseSearchTest.java +++ b/fhir-search/src/test/java/com/ibm/fhir/search/test/BaseSearchTest.java @@ -1,5 +1,5 @@ /* - * (C) Copyright IBM Corp. 2019, 2021 + * (C) Copyright IBM Corp. 2019, 2022 * * SPDX-License-Identifier: Apache-2.0 */ @@ -14,10 +14,11 @@ import java.nio.file.Files; import java.nio.file.StandardCopyOption; import java.nio.file.attribute.FileTime; -import java.util.List; import java.util.Map; import java.util.Map.Entry; import java.util.Set; +import java.util.SortedSet; +import java.util.TreeSet; import java.util.logging.LogManager; import org.testng.annotations.AfterMethod; @@ -27,7 +28,6 @@ import com.ibm.fhir.config.FHIRConfiguration; import com.ibm.fhir.config.FHIRRequestContext; import com.ibm.fhir.model.resource.SearchParameter; -import com.ibm.fhir.model.type.code.ResourceType; /** * A base search test with utilities for other search tests @@ -53,11 +53,7 @@ public void startMethod(Method method) { // Configure the request context for our search tests FHIRRequestContext context = FHIRRequestContext.get(); - if (context == null) { - context = new FHIRRequestContext(); - } context.setOriginalRequestUri(BASE); - FHIRRequestContext.set(context); } @AfterMethod @@ -77,15 +73,17 @@ public void clearThreadLocal() { FHIRRequestContext.remove(); } - /* - * The SearchParameters return an array of values, now the printSearchParameters returns all values. - * @param label - * @param spList + /** + * Conditionally print search parameters (code and url) under a header that includes the testName + * @param testName + * @param parameters */ - @SuppressWarnings("unused") - protected void printSearchParameters(String label, Map parameters) { + protected void printSearchParameters(String testName, Map parameters) { if (DEBUG && parameters != null) { - System.out.println("\nTest: " + label + "\nSearch Parameters:"); + System.out.println("\nTest: " + testName + "\nSearch Parameters:"); + + // sort the output for convenience + SortedSet set = new TreeSet(); for (Entry entry : parameters.entrySet()) { String code = entry.getKey(); SearchParameter sp = entry.getValue(); @@ -94,11 +92,10 @@ protected void printSearchParameters(String label, Map String version = (sp.getVersion() == null) ? null : sp.getVersion().getValue(); String canonical = (version == null) ? url : url + "|" + version; - List resources = sp.getBase(); - for (ResourceType resource : resources) { - System.out.println("\t" + resource.getValue() + ":" + code + ":" + canonical); - } + set.add("\t" + code + ":\t" + canonical); } + + set.forEach(System.out::println); } } diff --git a/fhir-search/src/test/resources/config/default/extension-search-parameters.json b/fhir-search/src/test/resources/config/extended/extension-search-parameters.json similarity index 100% rename from fhir-search/src/test/resources/config/default/extension-search-parameters.json rename to fhir-search/src/test/resources/config/extended/extension-search-parameters.json diff --git a/fhir-search/src/test/resources/config/default/fhir-server-config.json b/fhir-search/src/test/resources/config/extended/fhir-server-config.json similarity index 100% rename from fhir-search/src/test/resources/config/default/fhir-server-config.json rename to fhir-search/src/test/resources/config/extended/fhir-server-config.json From 9e8ff41d2dd3ffa0260496b450547ec8c435f22f Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Fri, 25 Feb 2022 08:33:15 -0500 Subject: [PATCH 6/7] issue #3283 - revert set ThreadLocal as part of FHIRRequestContext.get wrapper Thankfully I was mistaken in what I was seeing... I was thinking that the initialValue wasn't associated with the current thread (which made no sense) but in reality I was observing sloppy handling of the FHIRRequestContext Signed-off-by: Lee Surprenant --- .../src/main/java/com/ibm/fhir/config/FHIRRequestContext.java | 3 --- 1 file changed, 3 deletions(-) diff --git a/fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java b/fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java index 286ddfd0f67..f7f6d8888a7 100644 --- a/fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java +++ b/fhir-config/src/main/java/com/ibm/fhir/config/FHIRRequestContext.java @@ -185,9 +185,6 @@ public static FHIRRequestContext get() { if (log.isLoggable(Level.FINEST)) { log.finest("FHIRRequestContext.get: " + result.toString()); } - // Java ThreadLocal initial values are *not* automatically associated with the current thread - // so we need to explicitly set that here - contexts.set(result); return result; } From 1a69a035f03a53ae1e8832310d85b77e0d47937c Mon Sep 17 00:00:00 2001 From: Lee Surprenant Date: Fri, 25 Feb 2022 11:28:48 -0500 Subject: [PATCH 7/7] address review comments 1. added javadoc for the new ParametersMap methods and removed some of the other unused methods 2. use LinkedList instead of ArrayList in FHIRPersistenceJDBCImpl.extractSearchParameters 3. update warning message in FHIRPersistenceJDBCImpl.extractSearchParameters Signed-off-by: Lee Surprenant --- .../jdbc/impl/FHIRPersistenceJDBCImpl.java | 7 ++- .../fhir/search/parameters/ParametersMap.java | 58 +++++++++++++------ .../search/parameters/ParametersUtil.java | 4 +- .../search/parameters/ParametersUtilTest.java | 2 +- 4 files changed, 47 insertions(+), 24 deletions(-) diff --git a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java index 79d43afe6be..90b0c626be6 100644 --- a/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java +++ b/fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/impl/FHIRPersistenceJDBCImpl.java @@ -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; @@ -1501,7 +1502,8 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource, String expression; boolean isForStoring; - List allParameters = new ArrayList<>(); + // LinkedList because we don't need random access, we might remove from it later, and we want this fast + List allParameters = new LinkedList<>(); String parameterHashB64 = null; try { @@ -1740,7 +1742,8 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource, // 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"); + 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 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 3040d6169af..26741e38946 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 @@ -121,28 +121,50 @@ private void logParamConflict(String distinguisher, SearchParameter parameter, S } } - public void insertAll(ParametersMap map) { - for (Entry entry : map.codeEntries()) { - insert(entry.getKey(), entry.getValue()); - } - } - + /** + * Get the set of SearchParameter codes that have been added to this map. + * @return + * @implSpec This does not include any compartment inclusion criteria codes added + * via {@link #insertInclusionParam(String, SearchParameter)}; + * use {@link com.ibm.fhir.search.compartment.CompartmentUtil} for those. + */ public Set getCodes() { return codeMap.keySet(); } + /** + * Look up a search parameter that has been added to this map by its code. + * @param searchParameterCode + * @return null if it doesn't exist + */ public SearchParameter lookupByCode(String searchParameterCode) { return codeMap.get(searchParameterCode); } + /** + * Look up a search parameter that has been added to this map by its canonical URL. + * @param searchParameterCanonical + * @return null if it doesn't exist + */ public SearchParameter lookupByCanonical(String searchParameterCanonical) { return canonicalMap.get(searchParameterCanonical); } + /** + * Get a SearchParameter that has been added to this map as an inclusion parameter by its code. + * @param searchParameterCode + * @return null if it doesn't exist + */ public SearchParameter getInclusionParam(String searchParameterCode) { return inclusionParamMap.get(searchParameterCode); } + /** + * @return the set of search parameters added to this map + * @implSpec this set does not include SearchParameters added to the map via + * {@link ParametersMap#insertInclusionParam(String, SearchParameter)}; + * those can be obtained from {@link ParametersMap#inlcusionValues()} + */ public Collection values() { // use List to preserve order return Collections.unmodifiableList(canonicalMap.entrySet().stream() @@ -151,24 +173,22 @@ public Collection values() { .collect(Collectors.toList())); } - public boolean isEmpty() { - return codeMap.isEmpty(); - } - - public int size() { - return codeMap.size(); - } - + /** + * @return the set of search parameters in the map, indexed by code + * @implSpec this set does not include SearchParameters added to the map via + * {@link ParametersMap#insertInclusionParam(String, SearchParameter)} + */ public Set> codeEntries() { return Collections.unmodifiableSet(codeMap.entrySet()); } + /** + * @return the set of compartment inclusion criteria parameter values + * @implSpec this set may overlap with the set of "normal" SearchParameters + * that can be obtained from {@link #values()} + */ public Collection inclusionValues() { - // use List to preserve order - return Collections.unmodifiableList(inclusionParamMap.entrySet().stream() - .filter(e -> !e.getKey().contains("|")) - .map(e -> e.getValue()) - .collect(Collectors.toList())); + return inclusionParamMap.values(); } /** diff --git a/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java b/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java index a7fce1cdc9e..7a70e802dbd 100644 --- a/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java +++ b/fhir-search/src/main/java/com/ibm/fhir/search/parameters/ParametersUtil.java @@ -373,14 +373,14 @@ private static void print(PrintStream out, Map searchPara out.println(String.format(LOG_SIZE, keys.size())); for (String base : keys) { ParametersMap tmp = searchParamsMap.get(base); - for(SearchParameter param : tmp.values()) { + for (SearchParameter param : tmp.values()) { String expression = MISSING_EXPRESSION; if (param.getExpression() != null) { expression = param.getExpression().getValue(); } out.println(String.format(LOG_OUTPUT, base, param.getCode().getValue(), expression)); } - for(SearchParameter param : tmp.inclusionValues()) { + for (SearchParameter param : tmp.inclusionValues()) { String expression = MISSING_EXPRESSION; if (param.getExpression() != null) { expression = param.getExpression().getValue(); diff --git a/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersUtilTest.java b/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersUtilTest.java index b645208513b..2ea03489b1b 100644 --- a/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersUtilTest.java +++ b/fhir-search/src/test/java/com/ibm/fhir/search/parameters/ParametersUtilTest.java @@ -74,7 +74,7 @@ public void testGetTenantSPs() { Map result = ParametersUtil.getTenantSPs("default"); assertNotNull(result); assertNull(result.get("Junk")); - assertFalse(result.get("Observation").isEmpty()); + assertFalse(result.get("Observation").getCodes().isEmpty()); } @Test