issue #1777 - extract compartment inclusion params even when filtered

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
  */
@@ -177,6 +177,8 @@ public static void set(FHIRRequestContext context) {
 
     /**
      * Returns the FHIRRequestContext on the current thread.
+     * If it doesn't exist yet, this method will create a default instance
+     * and associate that with the current thread before returning it.
      */
     public static FHIRRequestContext get() {
         FHIRRequestContext result = contexts.get();

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,29 @@ public static JsonObject getRequestJsonObject(String method, String url) {
      * the specified patient via a subject attribute.
      */
     public static Observation buildPatientObservation(String patientId, String fileName) throws Exception {
-        // TODO review Reference id
         Observation observation = readLocalResource(fileName);
 
         observation = observation
             .toBuilder()
-            .subject(Reference.builder().reference(string("Patient/" + patientId)).build())
+            .subject(Reference.builder().reference("Patient/" + patientId).build())
+            .build();
+        return observation;
+    }
+
+    /**
+     * Loads an Observation resource from the specified file, then associates it with
+     * <ul>
+     * <li>the specified patient via Observation.subject
+     * <li>the specified practitioner via Observation.performer
+     * </ul>
+     */
+    public static Observation buildPatientObservation(String patientId, String practitionerId, String fileName) throws Exception {
+        Observation observation = readLocalResource(fileName);
+
+        observation = observation
+            .toBuilder()
+            .subject(Reference.builder().reference("Patient/" + patientId).build())
+            .performer(Reference.builder().reference("Practitioner/" + practitionerId).build())
             .build();
         return observation;
     }

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<ExtractedPar
     private String url;
     private String version;
 
+    // Whether to store the extracted value or not; defaulting to true
+    private boolean isForStoring = true;
+
     /**
      * Protected constructor
      */
@@ -175,4 +178,17 @@ public int compareTo(ExtractedParameterValue o) {
      */
     protected abstract int compareToInner(ExtractedParameterValue o);
 
+    /**
+     * @return whether this extracted parameter value is for storing
+     */
+    public boolean isForStoring() {
+        return isForStoring;
+    }
+
+    /**
+     * @param isForStoring whether this extracted parameter value is for storing
+     */
+    public void setForStoring(boolean isForStoring) {
+        this.isForStoring = isForStoring;
+    }
 }

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;
@@ -169,7 +170,6 @@
 import com.ibm.fhir.search.exception.FHIRSearchException;
 import com.ibm.fhir.search.parameters.InclusionParameter;
 import com.ibm.fhir.search.parameters.QueryParameter;
-import com.ibm.fhir.search.reference.value.CompartmentReference;
 import com.ibm.fhir.search.util.ReferenceValue;
 import com.ibm.fhir.search.util.ReferenceValue.ReferenceType;
 import com.ibm.fhir.search.util.SearchUtil;
@@ -493,20 +493,6 @@ private com.ibm.fhir.persistence.jdbc.dto.Resource createResourceDTO(Class<? ext
         return resourceDTO;
     }
 
-    /**
-     * Creates and returns a copy of the passed resource with the {@code Resource.id}
-     * {@code Resource.meta.versionId}, and {@code Resource.meta.lastUpdated} elements replaced.
-     *
-     * @param resource
-     * @param logicalId
-     * @param newVersionNumber
-     * @param lastUpdated
-     * @return the updated resource
-     */
-    private <T extends Resource> T copyAndSetResourceMetaFields(T resource, String logicalId, int newVersionNumber, Instant lastUpdated) {
-        return FHIRPersistenceUtil.copyAndSetResourceMetaFields(resource, logicalId, newVersionNumber, lastUpdated);
-    }
-
     /**
      * Convenience method to construct a new instance of the {@link ResourceDAO}
      * @param connection the connection to the database for the DAO to use
@@ -1601,8 +1587,10 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
         String version;
         String type;
         String expression;
+        boolean isForStoring;
 
-        List<ExtractedParameterValue> allParameters = new ArrayList<>();
+        // LinkedList because we don't need random access, we might remove from it later, and we want this fast
+        List<ExtractedParameterValue> allParameters = new LinkedList<>();
         String parameterHashB64 = null;
 
         try {
@@ -1617,6 +1605,9 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
                     version = sp.getVersion() != null ? sp.getVersion().getValue(): null;
                     final boolean wholeSystemParam = isWholeSystem(sp);
 
+                    String doNotStoreExtVal = FHIRUtil.getExtensionStringValue(sp, SearchConstants.DO_NOT_STORE_EXT_URL);
+                    isForStoring = (doNotStoreExtVal == null) || "false".equals(doNotStoreExtVal);
+
                     // As not to inject any other special handling logic, this is a simple inline check to see if
                     // _id or _lastUpdated are used, and ignore those extracted values.
                     if (SPECIAL_HANDLING.contains(code)) {
@@ -1665,9 +1656,8 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
                                     continue;
                                 }
 
-                                // Alternative: consider pulling the search parameter from the FHIRRegistry instead so we can use versioned references.
-                                // Of course, that would require adding extension-search-params to the Registry which would require the Registry to be tenant-aware.
-    //                            SearchParameter compSP = FHIRRegistry.getInstance().getResource(component.getDefinition().getValue(), SearchParameter.class);
+                                // Alternatively, we could pull the search parameter from the FHIRRegistry so we can use versioned references.
+                                // However, that would bypass search parameter filtering and so we favor the SeachUtil method here instead.
                                 SearchParameter compSP = SearchUtil.getSearchParameter(p.getResourceType(), component.getDefinition());
                                 JDBCParameterBuildingVisitor parameterBuilder = new JDBCParameterBuildingVisitor(p.getResourceType(), compSP);
                                 FHIRPathNode node = nodes.iterator().next();
@@ -1757,6 +1747,7 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
                             if (components.size() == p.getComponent().size()) {
 
                                 // only add the parameter if all of the components are present and accounted for
+                                p.setForStoring(isForStoring);
                                 allParameters.add(p);
                             }
                         }
@@ -1779,6 +1770,7 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
                                     if (wholeSystemParam) {
                                         p.setWholeSystem(true);
                                     }
+                                    p.setForStoring(isForStoring);
                                     allParameters.add(p);
                                     if (log.isLoggable(Level.FINE)) {
                                         log.fine("Extracted Parameter '" + p.getName() + "' from Resource.");
@@ -1814,6 +1806,7 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
                             if (wholeSystemParam) {
                                 p.setWholeSystem(true);
                             }
+                            p.setForStoring(isForStoring);
                             allParameters.add(p);
                             if (log.isLoggable(Level.FINE)) {
                                 log.fine("Extracted Parameter '" + p.getName() + "' from Resource.");
@@ -1825,14 +1818,20 @@ private ExtractedSearchParameters extractSearchParameters(Resource fhirResource,
                 // Augment the extracted parameter list with special values we use to represent compartment relationships.
                 // These references are stored as tokens and are used by the search query builder
                 // for compartment-based searches
-                addCompartmentParams(allParameters, fhirResource);
+                addCompartmentParams(allParameters, fhirResource.getClass().getSimpleName());
 
                 // If this is a definitional resource, augment the extracted parameter list with a composite
                 // parameter that will be used for canonical searches. It will contain the url and version
                 // values.
                 addCanonicalCompositeParam(allParameters);
             }
 
+            // Remove parameters that aren't to be stored
+            boolean anyRemoved = allParameters.removeIf(value -> !value.isForStoring());
+            if (anyRemoved) {
+                log.warning("The set of extracted parameters values unexpectedly contained values "
+                        + "that weren't for storing; these have been removed");
+            }
 
             // Generate the hash which is used to quickly determine whether the extracted parameters
             // are different than the extracted parameters that currently exist in the database.
@@ -1886,39 +1885,67 @@ private boolean isWholeSystem(SearchParameter sp) {
     }
 
     /**
-     * Augment the given allParameters list with ibm-internal parameters that represent relationships
-     * between the fhirResource to its compartments. These parameter values are subsequently used
+     * Augment the given allParameters list with ibm-internal parameters that represent the relationship
+     * between the fhirResource and its compartments. These parameter values are subsequently used
      * to improve the performance of compartment-based FHIR search queries. See
      * {@link CompartmentUtil#makeCompartmentParamName(String)} for details on how the
      * parameter name is composed for each relationship.
      * @param allParameters
+     * @param resourceType
      */
-    protected void addCompartmentParams(List<ExtractedParameterValue> allParameters, Resource fhirResource) throws FHIRSearchException {
-        final String resourceType = fhirResource.getClass().getSimpleName();
+    protected void addCompartmentParams(List<ExtractedParameterValue> allParameters, String resourceType) {
         if (log.isLoggable(Level.FINE)) {
             log.fine("Processing compartment parameters for resourceType: " + resourceType);
         }
+
         Map<String,Set<String>> compartmentRefParams = CompartmentUtil.getCompartmentParamsForResourceType(resourceType);
-        Map<String, Set<CompartmentReference>> compartmentMap = SearchUtil.extractCompartmentParameterValues(fhirResource, compartmentRefParams);
+        Map<String, List<ExtractedParameterValue>> collectedEPVByCode = allParameters.stream()
+                .collect(Collectors.groupingBy(epv -> epv.getName()));
 
-        for (Map.Entry<String, Set<CompartmentReference>> entry: compartmentMap.entrySet()) {
-            final String compartmentName = entry.getKey();
-            final String parameterName = CompartmentUtil.makeCompartmentParamName(compartmentName);
+        for (Entry<String, Set<String>> entry : compartmentRefParams.entrySet()) {
+            String param = entry.getKey();
+            Set<String> compartments = entry.getValue();
 
-            // Create a reference parameter value for each CompartmentReference extracted from the resource
-            for (CompartmentReference compartmentRef: entry.getValue()) {
-                ReferenceParmVal pv = new ReferenceParmVal();
-                pv.setName(parameterName);
-                pv.setResourceType(resourceType);
+            List<ExtractedParameterValue> collectedEPV = collectedEPVByCode.get(param);
+            // If the parameter has no corresponding extracted values, log and continue
+            if (collectedEPV == null) {
+                if (log.isLoggable(Level.FINE)) {
+                    log.warning("Compartment inclusion param " + param + " has no value");
+                }
+                continue;
+            }
 
-                // ReferenceType doesn't really matter here, but LITERAL_RELATIVE is appropriate
-                ReferenceValue rv = new ReferenceValue(compartmentName, compartmentRef.getReferenceResourceValue(), ReferenceType.LITERAL_RELATIVE, null);
-                pv.setRefValue(rv);
+            for (ExtractedParameterValue epv : collectedEPV) {
+                if (!(epv instanceof ReferenceParmVal)) {
+                    log.warning("Skipping compartment inclusion param " + param + "; expected ReferenceParmVal but found " +
+                            epv.getClass().getSimpleName());
+                    continue;
+                }
 
-                if (log.isLoggable(Level.FINE)) {
-                    log.fine("Adding compartment reference parameter: [" + resourceType + "] "+ parameterName + " = " + rv.getTargetResourceType() + "/" + rv.getValue());
+                ReferenceValue rv = ((ReferenceParmVal) epv).getRefValue();
+                if (rv != null && rv.getType() == ReferenceType.LITERAL_RELATIVE
+                        && compartments.contains(rv.getTargetResourceType())) {
+                    String internalCompartmentParamName = CompartmentUtil.makeCompartmentParamName(rv.getTargetResourceType());
+
+                    if (epv.isForStoring()) {
+                        // create a copy of the extracted parameter value but with the new internal compartment parameter name
+                        ReferenceParmVal pv = new ReferenceParmVal();
+                        pv.setName(internalCompartmentParamName);
+                        pv.setResourceType(resourceType);
+                        pv.setRefValue(rv);
+
+                        if (log.isLoggable(Level.FINE)) {
+                            log.fine("Adding compartment reference parameter: [" + resourceType + "] " +
+                                    internalCompartmentParamName + " = " + rv.getTargetResourceType() + "/" + rv.getValue());
+                        }
+                        allParameters.add(pv);
+                    } else {
+                        // since the extracted parameter value isn't going to be stored,
+                        // just rename it with our internal compartment param name and mark that for storing
+                        epv.setName(internalCompartmentParamName);
+                        epv.setForStoring(true);
+                    }
                 }
-                allParameters.add(pv);
             }
         }
     }

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;
 
@@ -102,6 +103,11 @@ private SearchConstants() {
     public static final String VERSION = "version";
 
     public static final String IMPLICIT_SYSTEM_EXT_URL = FHIRConstants.EXT_BASE + "implicit-system";
+    public static final String DO_NOT_STORE_EXT_URL = FHIRConstants.EXT_BASE + "do-not-store";
+    public static final Extension DO_NOT_STORE_EXT = Extension.builder()
+            .url(SearchConstants.DO_NOT_STORE_EXT_URL)
+            .value(true)
+            .build();
 
     // Extracted search parameter suffix for :identifier modifier
     public static final String IDENTIFIER_MODIFIER_SUFFIX = ":identifier";