Fix #198 - Sorting should only sort on the individual parameter searched on, not all params of the same type

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java

@@ -283,7 +283,7 @@ public class ResponseHighlighterInterceptor extends InterceptorAdapter {
 		
 		BaseServerResponseException bsre = (BaseServerResponseException)theException;
 		if (bsre.getOperationOutcome() == null) {
-			
+			return super.handleException(theRequestDetails, theException, theServletRequest, theServletResponse);
 		}
 		
 		streamResponse(theRequestDetails, theServletRequest, theServletResponse, bsre.getOperationOutcome());

hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java

@@ -1050,7 +1050,7 @@ public abstract class BaseHapiFhirResourceDao<T extends IResource> extends BaseH
 		return type;
 	}
 
-	private void createSort(CriteriaBuilder theBuilder, Root<ResourceTable> theFrom, SortSpec theSort, List<Order> theOrders) {
+	private void createSort(CriteriaBuilder theBuilder, Root<ResourceTable> theFrom, SortSpec theSort, List<Order> theOrders, List<Predicate> thePredicates) {
 		if (theSort == null || isBlank(theSort.getParamName())) {
 			return;
 		}
@@ -1065,7 +1065,7 @@ public abstract class BaseHapiFhirResourceDao<T extends IResource> extends BaseH
 				theOrders.add(theBuilder.desc(theFrom.get("myId")));
 			}
 
-			createSort(theBuilder, theFrom, theSort.getChain(), theOrders);
+			createSort(theBuilder, theFrom, theSort.getChain(), theOrders, thePredicates);
 			return;
 		}
 
@@ -1096,6 +1096,8 @@ public abstract class BaseHapiFhirResourceDao<T extends IResource> extends BaseH
 		}
 
 		From<?, ?> stringJoin = theFrom.join(joinAttrName, JoinType.INNER);
+		thePredicates.add(theBuilder.equal(stringJoin.get("myParamName"), theSort.getParamName()));
+		
 		// Predicate p = theBuilder.equal(stringJoin.get("myParamName"), theSort.getParamName());
 		// Predicate pn = theBuilder.isNull(stringJoin.get("myParamName"));
 		// thePredicates.add(theBuilder.or(p, pn));
@@ -1106,7 +1108,7 @@ public abstract class BaseHapiFhirResourceDao<T extends IResource> extends BaseH
 			theOrders.add(theBuilder.desc(stringJoin.get(sortAttrName)));
 		}
 
-		createSort(theBuilder, theFrom, theSort.getChain(), theOrders);
+		createSort(theBuilder, theFrom, theSort.getChain(), theOrders, thePredicates);
 	}
 
 	@Override
@@ -1715,7 +1717,7 @@ public abstract class BaseHapiFhirResourceDao<T extends IResource> extends BaseH
 			CriteriaQuery<Tuple> cq = builder.createTupleQuery();
 			Root<ResourceTable> from = cq.from(ResourceTable.class);
 			predicates.add(from.get("myId").in(loadPids));
-			createSort(builder, from, theParams.getSort(), orders);
+			createSort(builder, from, theParams.getSort(), orders, predicates);
 			if (orders.size() > 0) {
 				Set<Long> originalPids = loadPids;
 				loadPids = new LinkedHashSet<Long>();

hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/FhirResourceDaoDstu2Test.java

@@ -2374,47 +2374,121 @@ public class FhirResourceDaoDstu2Test extends BaseJpaTest {
 		assertThat(actual.subList(2, 4), containsInAnyOrder(id1, id3));
 	}
 
+	/**
+	 * See #198
+	 */
 	@Test
-	public void testSortByString() {
+	public void testSortByString02() {
+		Patient p;
+		String string = "testSortByString02";
+
+		p = new Patient();
+		p.addIdentifier().setSystem("urn:system").setValue(string);
+		p.addName().addFamily("Fam1").addGiven("Giv1");
+		ourPatientDao.create(p).getId().toUnqualifiedVersionless();
+
+		p = new Patient();
+		p.addIdentifier().setSystem("urn:system").setValue(string);
+		p.addName().addFamily("Fam2").addGiven("Giv1");
+		ourPatientDao.create(p).getId().toUnqualifiedVersionless();
+
+		p = new Patient();
+		p.addIdentifier().setSystem("urn:system").setValue(string);
+		p.addName().addFamily("Fam2").addGiven("Giv2");
+		ourPatientDao.create(p).getId().toUnqualifiedVersionless();
+		
+		p = new Patient();
+		p.addIdentifier().setSystem("urn:system").setValue(string);
+		p.addName().addFamily("Fam1").addGiven("Giv2");
+		ourPatientDao.create(p).getId().toUnqualifiedVersionless();
+
+		SearchParameterMap pm;
+		List<String> names;
+		
+		pm = new SearchParameterMap();
+		pm.add(Patient.SP_IDENTIFIER, new TokenParam("urn:system", string));
+		pm.setSort(new SortSpec(Patient.SP_FAMILY));
+		names = extractNames(ourPatientDao.search(pm));
+		ourLog.info("Names: {}", names);
+		assertThat(names.subList(0, 2), containsInAnyOrder("Giv1 Fam1", "Giv2 Fam1"));
+		assertThat(names.subList(2, 4), containsInAnyOrder("Giv1 Fam2", "Giv2 Fam2"));
+
+		pm = new SearchParameterMap();
+		pm.add(Patient.SP_IDENTIFIER, new TokenParam("urn:system", string));
+		pm.setSort(new SortSpec(Patient.SP_FAMILY).setChain(new SortSpec(Patient.SP_GIVEN)));
+		names = extractNames(ourPatientDao.search(pm));
+		ourLog.info("Names: {}", names);
+		assertThat(names.subList(0, 2), contains("Giv1 Fam1", "Giv2 Fam1"));
+		assertThat(names.subList(2, 4), contains("Giv1 Fam2", "Giv2 Fam2"));
+
+		pm = new SearchParameterMap();
+		pm.add(Patient.SP_IDENTIFIER, new TokenParam("urn:system", string));
+		pm.setSort(new SortSpec(Patient.SP_FAMILY).setChain(new SortSpec(Patient.SP_GIVEN, SortOrderEnum.DESC)));
+		names = extractNames(ourPatientDao.search(pm));
+		ourLog.info("Names: {}", names);
+		assertThat(names.subList(0, 2), contains("Giv2 Fam1", "Giv1 Fam1"));
+		assertThat(names.subList(2, 4), contains("Giv2 Fam2", "Giv1 Fam2"));
+
+		pm = new SearchParameterMap();
+		pm.add(Patient.SP_IDENTIFIER, new TokenParam("urn:system", string));
+		pm.setSort(new SortSpec(Patient.SP_FAMILY, SortOrderEnum.DESC).setChain(new SortSpec(Patient.SP_GIVEN, SortOrderEnum.DESC)));
+		names = extractNames(ourPatientDao.search(pm));
+		ourLog.info("Names: {}", names);
+		assertThat(names.subList(0, 2), contains("Giv2 Fam2", "Giv1 Fam2"));
+		assertThat(names.subList(2, 4), contains("Giv2 Fam1", "Giv1 Fam1"));
+}
+	
+	private List<String> extractNames(IBundleProvider theSearch) {
+		ArrayList<String> retVal = new ArrayList<String>();
+		for (IBaseResource next : theSearch.getResources(0, theSearch.size())) {
+			Patient nextPt = (Patient)next;
+			retVal.add(nextPt.getNameFirstRep().getNameAsSingleString());
+		}
+		return retVal;
+	}
+
+	@Test
+	public void testSortByString01() {
 		Patient p = new Patient();
-		p.addIdentifier().setSystem("urn:system").setValue("testSortByString");
+		String string = "testSortByString01";
+		p.addIdentifier().setSystem("urn:system").setValue(string);
 		p.addName().addFamily("testSortF1").addGiven("testSortG1");
 		IIdType id1 = ourPatientDao.create(p).getId().toUnqualifiedVersionless();
 
 		// Create out of order
 		p = new Patient();
-		p.addIdentifier().setSystem("urn:system").setValue("testSortByString");
+		p.addIdentifier().setSystem("urn:system").setValue(string);
 		p.addName().addFamily("testSortF3").addGiven("testSortG3");
 		IIdType id3 = ourPatientDao.create(p).getId().toUnqualifiedVersionless();
 
 		p = new Patient();
-		p.addIdentifier().setSystem("urn:system").setValue("testSortByString");
+		p.addIdentifier().setSystem("urn:system").setValue(string);
 		p.addName().addFamily("testSortF2").addGiven("testSortG2");
 		IIdType id2 = ourPatientDao.create(p).getId().toUnqualifiedVersionless();
 
 		p = new Patient();
-		p.addIdentifier().setSystem("urn:system").setValue("testSortByString");
+		p.addIdentifier().setSystem("urn:system").setValue(string);
 		IIdType id4 = ourPatientDao.create(p).getId().toUnqualifiedVersionless();
 
 		SearchParameterMap pm;
 		List<IIdType> actual;
 
 		pm = new SearchParameterMap();
-		pm.add(Patient.SP_IDENTIFIER, new TokenParam("urn:system", "testSortByString"));
+		pm.add(Patient.SP_IDENTIFIER, new TokenParam("urn:system", string));
 		pm.setSort(new SortSpec(Patient.SP_FAMILY));
 		actual = toUnqualifiedVersionlessIds(ourPatientDao.search(pm));
 		assertEquals(4, actual.size());
 		assertThat(actual, contains(id1, id2, id3, id4));
 
 		pm = new SearchParameterMap();
-		pm.add(Patient.SP_IDENTIFIER, new TokenParam("urn:system", "testSortByString"));
+		pm.add(Patient.SP_IDENTIFIER, new TokenParam("urn:system", string));
 		pm.setSort(new SortSpec(Patient.SP_FAMILY).setOrder(SortOrderEnum.ASC));
 		actual = toUnqualifiedVersionlessIds(ourPatientDao.search(pm));
 		assertEquals(4, actual.size());
 		assertThat(actual, contains(id1, id2, id3, id4));
 
 		pm = new SearchParameterMap();
-		pm.add(Patient.SP_IDENTIFIER, new TokenParam("urn:system", "testSortByString"));
+		pm.add(Patient.SP_IDENTIFIER, new TokenParam("urn:system", string));
 		pm.setSort(new SortSpec(Patient.SP_FAMILY).setOrder(SortOrderEnum.DESC));
 		actual = toUnqualifiedVersionlessIds(ourPatientDao.search(pm));
 		assertEquals(4, actual.size());

hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml

@@ -18,7 +18,7 @@
 		<appender-ref ref="STDOUT" />
 	</logger>
 	
-	<logger name="org.hibernate.SQL" additivity="false" level="info">
+	<logger name="org.hibernate.SQL" additivity="false" level="trace">
 		<appender-ref ref="STDOUT" />
 	</logger>
 

hapi-fhir-jpaserver-uhnfhirtest/pom.xml

@@ -30,6 +30,11 @@
 			<artifactId>hapi-fhir-structures-dstu2</artifactId>
 			<version>1.2-SNAPSHOT</version>
 		</dependency>
+		<dependency>
+			<groupId>ca.uhn.hapi.fhir</groupId>
+			<artifactId>hapi-fhir-structures-hl7org-dstu2</artifactId>
+			<version>1.2-SNAPSHOT</version>
+		</dependency>
 		<dependency>
 			<groupId>ca.uhn.hapi.fhir</groupId>
 			<artifactId>hapi-fhir-testpage-overlay</artifactId>

src/changes/changes.xml

@@ -28,6 +28,10 @@
 				count extensions exported in the Conformance statement by the JPA
 				server.
 			</action>
+			<action type="fix" issue="198">
+				JPA server sorting often returned unexpected orders when multiple
+				indexes of the same type were found on the same resource (e.g. multiple string indexed fields). Thanks to Travis Cummings for reporting!
+			</action>
 		</release>
 		<release version="1.1" date="2015-07-13">
 			<action type="add">