issue #418 - add order by clause to searchByIds to ensure sorted results

1. Updated the SearchPerformanceTest so that the observations have
components with values...just to easily create many observations which
helps verify the search behavior.  This change could be removed if we
want.

2. Deleted unused variants of searchForIds and searchByIds. I think
these came from the original "basic" schema that was removed for
#93 but they were no longer in use and
should be safe to delete.

3. Introduced ORDER BY CASE clause to ResourceDAOImpl.searchByIds.
I found that DERBY is always returning the results in the same order as
the resource ids provided in the "IN" clause, but in general we
shouldn't rely on that.  I tested the CASE statements on derby with up
to 1000 results, but we need to run a similar test on Db2 to ensure it
works there as well.

Signed-off-by: Lee Surprenant <lmsurpre@us.ibm.com>

Author: lmsurpre

fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/JDBCConstants.java

@@ -41,6 +41,9 @@ public class JDBCConstants {
     public static final String ESCAPE_UNDERSCORE = ESCAPE_CHAR + "_";
     public static final String ESCAPE_PERCENT = ESCAPE_CHAR + PERCENT_WILDCARD;
     public static final String ESCAPE_EXPR = " ESCAPE '" + ESCAPE_CHAR + "'";
+    public static final String WHEN = " WHEN ";
+    public static final String THEN = " THEN ";
+    public static final String END = " END";
 
     /**
      * Maps Parameter modifiers to SQL operators.

fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/api/ResourceDAO.java

@@ -118,19 +118,18 @@ List<Resource> search(String sqlSelect)
      * @throws FHIRPersistenceDataAccessException
      * @throws FHIRPersistenceDBConnectException
      */
-    List<Long> searchForIds(String sqlSelect)
-            throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException;
+    List<Long> searchForIds(SqlQueryData  queryData) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException;
 
     /**
      * Searches for Resources that contain one of the passed ids.
+     * @param resourceType - The type of the FHIR Resource
      * @param resourceIds - A List of resource ids.
      * @return List<Resource> - A List of resources matching the the passed list of ids.
      * @throws FHIRPersistenceDataAccessException
      * @throws FHIRPersistenceDBConnectException
      */
-    List<Resource> searchByIds(List<Long> resourceIds)
-            throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException;
-    
+    List<Resource> searchByIds(String resourceType, List<Long> resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException;
+
     /**
      * Executes a count query based on the data contained in the passed SqlQueryData, using it's encapsulated search string and bind variables.
      * @param queryData - Contains a search string and (optionally) bind variables.
@@ -173,27 +172,6 @@ List<Resource> searchByIds(List<Long> resourceIds)
      * @throws FHIRPersistenceDataAccessException
      */
     Integer readResourceTypeId(String parameterName) throws FHIRPersistenceDBConnectException, FHIRPersistenceDataAccessException;
-    
-    /**
-     * This method supports the execution of a specialized query designed to return Resource ids, based on the contents
-     * of the passed select statement.
-     * Note that the first column to be selected MUST be the Resource.id column.
-     * @param sqlSelect - A select for Resource ids.
-     * @return - A List of resource ids that satisfy the passed SQL query.
-     * @throws FHIRPersistenceDataAccessException
-     * @throws FHIRPersistenceDBConnectException
-     */
-    List<Long> searchForIds(SqlQueryData  queryData) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException;
-    
-    /**
-     * Searches for Resources that contain one of the passed ids.
-     * @param resourceType - The type of the FHIR Resource.
-     * @param resourceIds - A List of resource ids.
-     * @return List<Resource> - A List of resources matching the the passed list of ids.
-     * @throws FHIRPersistenceDataAccessException
-     * @throws FHIRPersistenceDBConnectException
-     */
-    List<Resource> searchByIds(String resourceType, List<Long> resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException;
 
     /**
      * Adds a resource type / resource id pair to a candidate collection for population into the ResourceTypesCache. 

fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/dao/impl/ResourceDAOImpl.java

@@ -6,6 +6,8 @@
 
 package com.ibm.fhir.persistence.jdbc.dao.impl;
 
+import static com.ibm.fhir.persistence.jdbc.JDBCConstants.*;
+
 import java.sql.CallableStatement;
 import java.sql.Connection;
 import java.sql.PreparedStatement;
@@ -90,6 +92,8 @@ public class ResourceDAOImpl extends FHIRDbDAOImpl implements ResourceDAO {
                                                     "FROM %s_RESOURCES R, %s_LOGICAL_RESOURCES LR WHERE R.LOGICAL_RESOURCE_ID = LR.LOGICAL_RESOURCE_ID AND " +
                                                     "R.RESOURCE_ID IN ";
 
+    private static final String SQL_ORDER_BY_IDS = "ORDER BY CASE R.RESOURCE_ID ";
+    
     private static final String DERBY_PAGINATION_PARMS = "OFFSET ? ROWS FETCH NEXT ? ROWS ONLY";
 
     private static final String DB2_PAGINATION_PARMS = "LIMIT ? OFFSET ?";
@@ -418,64 +422,7 @@ public List<Long> searchForIds(SqlQueryData queryData) throws FHIRPersistenceDat
         return resourceIds;
     }
 
-    @Override
-    public List<Resource> searchByIds(String resourceType, List<Long> resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException {
-        final String METHODNAME = "searchByIds";
-        log.entering(CLASSNAME, METHODNAME);
-        
-        if (resourceIds.isEmpty()) {
-            return Collections.emptyList();
-        }
-
-        Connection connection = null;
-        PreparedStatement stmt = null;
-        ResultSet resultSet = null;
-        String errMsg;
-        StringBuilder idQuery = new StringBuilder();
-        List<Resource> resources = new ArrayList<>();
-        String stmtString = null;
-        long dbCallStartTime;
-        double dbCallDuration;
-
-        try {
-            stmtString = String.format(this.getSearchByIdsSql(resourceType));
-            idQuery.append(stmtString);
-            idQuery.append("(");
-            for (int i = 0; i < resourceIds.size(); i++) {
-                if (i > 0) {
-                    idQuery.append(",");
-                }
-                idQuery.append(resourceIds.get(i));
-            }
-            idQuery.append(")");
-
-            connection = this.getConnection();
-            stmt = connection.prepareStatement(idQuery.toString());
-            dbCallStartTime = System.nanoTime();
-            resultSet = stmt.executeQuery();
-            dbCallDuration = (System.nanoTime()-dbCallStartTime)/1e6;
-            if (log.isLoggable(Level.FINE)) {
-                log.fine("DB search by ids complete. SQL=[" + idQuery + "]  executionTime=" + dbCallDuration + "ms");
-            }
-            resources = this.createDTOs(resultSet);
-        } catch(FHIRPersistenceException e) {
-            throw e;
-        } catch (Throwable e) {
-            FHIRPersistenceDataAccessException fx = new FHIRPersistenceDataAccessException("Failure retrieving FHIR Resources");
-            errMsg = "Failure retrieving FHIR Resources. SQL=[" + idQuery + "]";
-            throw severe(log, fx, errMsg, e);
-        } finally {
-            this.cleanup(resultSet, stmt, connection);
-            log.exiting(CLASSNAME, METHODNAME);
-        }
-        return resources;
-    }
-
-    protected String getSearchByIdsSql(String resourceType) {
-        String stmtString;
-        stmtString = String.format(SQL_SEARCH_BY_IDS, resourceType, resourceType);
-        return stmtString;
-    }
+    
 
      /**
      * Adds a resource type/ resource id pair to a candidate collection for population into the ResourceTypesCache.
@@ -734,78 +681,55 @@ public List<Resource> search(String sqlSelect) throws FHIRPersistenceDataAccessE
     }
 
     @Override
-    public List<Long> searchForIds(String sqlSelect) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException {
-        final String METHODNAME = "searchForIds";
-        log.entering(CLASSNAME, METHODNAME);
-        
-        List<Long> resourceIds = new ArrayList<>();
-        Connection connection = null;
-        PreparedStatement stmt = null;
-        ResultSet resultSet = null;
-        String errMsg;
-        
-        try {
-            connection = this.getConnection();
-            stmt = connection.prepareStatement(sqlSelect);
-            resultSet = stmt.executeQuery();
-            while (resultSet.next()) {
-                resourceIds.add(resultSet.getLong(1));
-            }
-        } catch(FHIRPersistenceException e) {
-            throw e;
-        } catch (Throwable e) {
-            // Log the SQL but don't expose it in the exception
-            FHIRPersistenceDataAccessException fx = new FHIRPersistenceDataAccessException("Failure retrieving FHIR Resource Ids.");
-            errMsg = "Failure retrieving FHIR Resource Ids. SQL=" + sqlSelect;
-            throw severe(log, fx, errMsg, e);
-        } finally {
-            this.cleanup(resultSet, stmt, connection);
-            log.exiting(CLASSNAME, METHODNAME);
-        }
-        return resourceIds;
-    }
-
-    @Override
-    public List<Resource> searchByIds(List<Long> resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException {
+    public List<Resource> searchByIds(String resourceType, List<Long> resourceIds) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException {
         final String METHODNAME = "searchByIds";
         log.entering(CLASSNAME, METHODNAME);
 
         if (resourceIds.isEmpty()) {
             return Collections.emptyList();
         }
-        
+
         Connection connection = null;
         PreparedStatement stmt = null;
         ResultSet resultSet = null;
         String errMsg;
         StringBuilder idQuery = new StringBuilder();
         List<Resource> resources = new ArrayList<>();
-        
+        String stmtString = null;
+        long dbCallStartTime;
+        double dbCallDuration;
+
         try {
-            idQuery.append(SQL_SEARCH_BY_IDS);
+            stmtString = getSearchByIdsSql(resourceType);
+            idQuery.append(stmtString);
             idQuery.append("(");
+            // resourceIds should have a max length of 1000 (the max page size)
+            StringBuilder caseStmts = new StringBuilder();
             for (int i = 0; i < resourceIds.size(); i++) {
                 if (i > 0) {
                     idQuery.append(",");
                 }
-                idQuery.append("?");
+                idQuery.append(resourceIds.get(i));
+                
+                // build up the caseStmts here so we only need to iterate the list once
+                caseStmts.append(WHEN + resourceIds.get(i) + THEN + i);
             }
-            idQuery.append(")");
-                        
+            idQuery.append(") " + SQL_ORDER_BY_IDS + caseStmts + END);
+
             connection = this.getConnection();
             stmt = connection.prepareStatement(idQuery.toString());
-            // Inject IDs into the prepared stmt.
-            for (int i = 0; i < resourceIds.size(); i++) {
-                stmt.setObject(i+1, resourceIds.get(i));
-            }
+            dbCallStartTime = System.nanoTime();
             resultSet = stmt.executeQuery();
+            dbCallDuration = (System.nanoTime()-dbCallStartTime)/1e6;
+            if (log.isLoggable(Level.FINE)) {
+                log.fine("DB search by ids complete. SQL=[" + idQuery + "]  executionTime=" + dbCallDuration + "ms");
+            }
             resources = this.createDTOs(resultSet);
         } catch(FHIRPersistenceException e) {
             throw e;
         } catch (Throwable e) {
-            // Log the SQL but don't expose it in the exception
-            FHIRPersistenceDataAccessException fx = new FHIRPersistenceDataAccessException("Failure retrieving FHIR Resources.");
-            errMsg = "Failure retrieving FHIR Resources. SQL=" + idQuery;
+            FHIRPersistenceDataAccessException fx = new FHIRPersistenceDataAccessException("Failure retrieving FHIR Resources");
+            errMsg = "Failure retrieving FHIR Resources. SQL=[" + idQuery + "]";
             throw severe(log, fx, errMsg, e);
         } finally {
             this.cleanup(resultSet, stmt, connection);
@@ -814,6 +738,10 @@ public List<Resource> searchByIds(List<Long> resourceIds) throws FHIRPersistence
         return resources;
     }
 
+    protected String getSearchByIdsSql(String resourceType) {
+        return String.format(SQL_SEARCH_BY_IDS, resourceType, resourceType);
+    }
+
     @Override
     public int searchCount(String sqlSelectCount) throws FHIRPersistenceDataAccessException, FHIRPersistenceDBConnectException {
         final String METHODNAME = "searchCount";

fhir-persistence-jdbc/src/main/java/com/ibm/fhir/persistence/jdbc/util/SortedQuerySegmentAggregator.java

@@ -57,15 +57,14 @@ protected SortedQuerySegmentAggregator(Class<?> resourceType, int offset, int pa
      * <p>
      * A simple example query produced by this method:
      * <pre>
-     * SELECT R.RESOURCE_ID,MIN(S1.STR_VALUE) FROM 
-     * Patient_RESOURCES R JOIN 
-     * Patient_LOGICAL_RESOURCES LR ON R.LOGICAL_RESOURCE_ID=LR.LOGICAL_RESOURCE_ID  JOIN 
-     * Patient_TOKEN_VALUES P1 ON P1.RESOURCE_ID=R.RESOURCE_ID  
-     * LEFT OUTER JOIN Patient_STR_VALUES S1 ON (S1.PARAMETER_NAME_ID=50 AND S1.RESOURCE_ID = R.RESOURCE_ID) WHERE 
-     * R.RESOURCE_ID = LR.CURRENT_RESOURCE_ID AND 
-     * R.IS_DELETED <> 'Y' AND 
-     * P1.RESOURCE_ID = R.RESOURCE_ID AND 
-     * (P1.PARAMETER_NAME_ID=196 AND ((P1.TOKEN_VALUE = false))) 
+     * SELECT R.RESOURCE_ID,MIN(S1.STR_VALUE) FROM Patient_RESOURCES R 
+     *   JOIN Patient_LOGICAL_RESOURCES LR ON R.LOGICAL_RESOURCE_ID=LR.LOGICAL_RESOURCE_ID
+     *   JOIN Patient_TOKEN_VALUES P1 ON P1.RESOURCE_ID=R.RESOURCE_ID  
+     *   LEFT OUTER JOIN Patient_STR_VALUES S1 ON (S1.PARAMETER_NAME_ID=50 AND S1.RESOURCE_ID = R.RESOURCE_ID)
+     *   WHERE R.RESOURCE_ID = LR.CURRENT_RESOURCE_ID AND 
+     *         R.IS_DELETED <> 'Y' AND 
+     *         P1.RESOURCE_ID = R.RESOURCE_ID AND 
+     *         (P1.PARAMETER_NAME_ID=196 AND ((P1.TOKEN_VALUE = false))) 
      * GROUP BY R.RESOURCE_ID  
      * ORDER BY MIN(S1.STR_VALUE) asc NULLS LAST 
      * OFFSET 0 ROWS FETCH NEXT 100 ROWS ONLY;

fhir-server-test/src/test/java/com/ibm/fhir/server/test/SearchPerformaceTest.java

@@ -15,6 +15,7 @@
 import static org.testng.Assert.assertTrue;
 
 import java.lang.reflect.Method;
+import java.math.BigDecimal;
 import java.util.List;
 
 import javax.ws.rs.client.Entity;
@@ -27,9 +28,14 @@
 import com.ibm.fhir.model.resource.Bundle;
 import com.ibm.fhir.model.resource.Observation;
 import com.ibm.fhir.model.resource.Patient;
+import com.ibm.fhir.model.resource.Observation.Component;
 import com.ibm.fhir.model.test.TestUtil;
 import com.ibm.fhir.model.type.Code;
+import com.ibm.fhir.model.type.CodeableConcept;
 import com.ibm.fhir.model.type.Coding;
+import com.ibm.fhir.model.type.Decimal;
+import com.ibm.fhir.model.type.Quantity;
+import com.ibm.fhir.model.type.Uri;
 import com.ibm.fhir.model.type.code.AdministrativeGender;
 import com.ibm.fhir.model.util.FHIRUtil;
 
@@ -44,7 +50,7 @@ public class SearchPerformaceTest extends FHIRServerTestBase {
     // Controls how many observations and patients to create for the test.
     // Using invocationCount of testng can cause the testng report grows too big, so
     // this config is used to make sure all test users are created in one testng step.
-    private final int numOfPatientObservationsToCreate = 100;
+    private final int numOfPatientObservationsToCreate = 1000;
 
     /**
      * Retrieve the server's conformance statement to determine the status of certain runtime options.
@@ -87,6 +93,21 @@ public void testCreatePatientAndObservation() throws Exception {
             // create observation for the patient
             Observation observation =
                     TestUtil.buildPatientObservation(patientId, "Observation1.json");
+            observation = observation.toBuilder()
+                    .code(CodeableConcept.builder()
+                            .coding(Coding.builder()
+                                    .system(Uri.of("http://loinc.org"))
+                                    .code(Code.of("55284-4"))
+                                    .build())
+                            .build())
+                    .component(Component.builder()
+                            .code(CodeableConcept.builder().text(string("component1")).build())
+                            .value(Quantity.builder()
+                                    .value(Decimal.of(BigDecimal.valueOf(Math.random())))
+                                    .unit(string("mmHg"))
+                                    .build())
+                            .build())
+                    .build();
             Entity<Observation> entity2 =
                     Entity.entity(observation, FHIRMediaType.APPLICATION_FHIR_JSON);
             response =

fhir-server-test/src/test/java/com/ibm/fhir/server/test/SortingTest.java

@@ -674,6 +674,7 @@ private void assertTrueNaturalOrdering(List<BigDecimal> sortedList) {
                 }
 
                 assertTrue(prior.compareTo(current) <= 0);
+                prior = current;
             }
             done = true;
         }