Merge pull request #372 from IBM/Albert-Master-New

issue #346 paging fix for include/revinclude search

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

@@ -31,6 +31,7 @@ public class InclusionQuerySegmentAggregator extends QuerySegmentAggregator {
     private static final String SELECT_ROOT = "SELECT RESOURCE_ID, LOGICAL_RESOURCE_ID, VERSION_ID, LAST_UPDATED, IS_DELETED, DATA, LOGICAL_ID FROM ";
     private static final String UNION_ALL = " UNION ALL ";
     private static final String REVINCLUDE_JOIN = "JOIN  {0}_STR_VALUES P1 ON P1.LOGICAL_RESOURCE_ID = R.LOGICAL_RESOURCE_ID ";
+    private static final String ORDERING = " ORDER BY R.LOGICAL_RESOURCE_ID ASC ";
 
     private List<InclusionParameter> includeParameters;
     private List<InclusionParameter> revIncludeParameters;
@@ -132,9 +133,6 @@ protected SqlQueryData buildCountQuery() throws Exception {
         queryString.append(super.buildFromClause());
         queryString.append(super.buildWhereClause());
 
-        this.processIncludeParameters(queryString, allBindVariables);
-        this.processRevIncludeParameters(queryString, allBindVariables);
-
         queryString.append(")");    
         queryString.append(COMBINED_RESULTS);    
 
@@ -164,20 +162,22 @@ protected SqlQueryData buildQuery() throws Exception {
 
         this.addBindVariables(allBindVariables);
 
+        queryString.append(InclusionQuerySegmentAggregator.SELECT_ROOT);
+        queryString.append("(");
         queryString.append(InclusionQuerySegmentAggregator.SELECT_ROOT);
         queryString.append("(");
         queryString.append(QuerySegmentAggregator.SELECT_ROOT);
         queryString.append(super.buildFromClause());
         queryString.append(super.buildWhereClause());
-
+        // Add ordering
+        queryString.append(ORDERING);
+        this.addPaginationClauses(queryString);
+        queryString.append(") RESULT ");
         this.processIncludeParameters(queryString, allBindVariables);
         this.processRevIncludeParameters(queryString, allBindVariables);
 
         queryString.append(")");        
-        queryString.append(COMBINED_RESULTS);    
-
-        this.addPaginationClauses(queryString);
-
+        queryString.append(COMBINED_RESULTS);
         queryData = new SqlQueryData(queryString.toString(), allBindVariables);
         log.exiting(CLASSNAME, METHODNAME);
         return queryData;
@@ -201,6 +201,10 @@ private void executeIncludeSubQuery(StringBuilder queryString, InclusionParamete
         subQueryString.append(super.buildFromClause());
         // Add WHERE clause for "root" resource type
         subQueryString.append(super.buildWhereClause());
+        // ORDER BY R.LOGICAL_RESOURCE_ID ASC
+        subQueryString.append(ORDERING);
+        // Only include resources related to the required page of the main resources.
+        this.addPaginationClauses(subQueryString);
         subQueryString.append(")");
 
         queryString.append("(");
@@ -276,6 +280,10 @@ private void processRevIncludeParameters(StringBuilder queryString, List<Object>
             queryString.append(super.buildFromClause());
             // Add WHERE clause for "root" resource type
             queryString.append(super.buildWhereClause());
+            // ORDER BY R.LOGICAL_RESOURCE_ID ASC
+            queryString.append(ORDERING);
+            // Only include resources related to the required page of the main resources.
+            this.addPaginationClauses(queryString);
 
             queryString.append(")");
 

fhir-persistence/src/test/java/com/ibm/fhir/persistence/test/common/AbstractIncludeRevincludeTest.java

@@ -15,6 +15,7 @@
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.HashMap;
+import java.util.HashSet;
 import java.util.List;
 import java.util.Map;
 
@@ -482,7 +483,65 @@ else if (resource instanceof Patient) {
         }
 
     }
-
+
+    private void checkIncludeAndRevIncludeResources(List<Resource> resources, int numOfPatients) {
+        HashSet<String> foundPatientOrgIds = new HashSet<String>();
+        HashSet<String> foundPatientIds = new HashSet<String>();
+        int numOfMatchedOrgs = 0;
+        for (Resource resource : resources) {
+            if (resource instanceof Patient) {
+                if (((Patient)resource).getManagingOrganization() != null) {
+                    foundPatientOrgIds.add(((Patient)resource).getManagingOrganization()
+                            .getReference().getValue().substring(13));
+                }
+                foundPatientIds.add(resource.getId().getValue());
+            } 
+        }
+        // verify the number of patients in the result page.
+        assertTrue(numOfPatients == foundPatientIds.size());
+        for (Resource resource : resources) {
+            if (resource instanceof Observation) {
+                // verify the returned observations are related to the patients of the same page
+                assertTrue(foundPatientIds.contains(((Observation)resource).getSubject()
+                        .getReference().getValue().substring(8)));
+            } else if (resource instanceof Organization) {
+                // verify the returned managing organizations are related to the patients of the same page
+                assertTrue(foundPatientOrgIds.contains(resource.getId().getValue()));
+                numOfMatchedOrgs++;
+            } else if (resource instanceof Patient) {
+                assertTrue(foundPatientIds.contains(resource.getId().getValue()));
+            } else {
+                fail("Unexpected resource type returned.");
+            }
+        }
+        // verify all related managing organizations are in the same page.
+        assertTrue(numOfMatchedOrgs == foundPatientOrgIds.size());
+    }
+    /**
+     * This test queries page of Patient with inclusion of the referenced managing organizations. 
+     * and also requests the reverse inclusion of Observations.
+     * It verifies:
+     * (1) the number of patients in the result page; 
+     * (2) the returned managing organizations and observations are related to the patients of the same page;
+     * (3) all related managing organizations are in the same page.
+     * @throws Exception
+     */
+    @Test
+    public void testIncludeAndRevIncludeMultiplePatients() throws Exception {
+        Map<String, List<String>> queryParms = new HashMap<String, List<String>>();
+        queryParms.put("_include", Collections.singletonList("Patient:organization"));
+        queryParms.put("_revinclude", Collections.singletonList("Observation:patient"));
+        queryParms.put("_count", Collections.singletonList("2"));
+        queryParms.put("_page", Collections.singletonList("1"));
+        List<Resource> resources = runQueryTest(Patient.class, queryParms);
+        assertNotNull(resources);
+        // check the first page
+        checkIncludeAndRevIncludeResources(resources, 2);
+        queryParms.put("_page", Collections.singletonList("2"));
+        // check the second page
+        checkIncludeAndRevIncludeResources(resources, 2);
+    }
+
     private Reference reference(String reference) {
         return Reference.builder().reference(string(reference)).build();
     }