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

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;
         }