Skip to content

Commit

Permalink
Fix #198 - Sorting should only sort on the individual parameter searc…
Browse files Browse the repository at this point in the history 
…hed on, not all params of the same type
  • Loading branch information
@jamesagnew
jamesagnew committed Jul 18, 2015
1 parent 57ee1fe commit 626f467
Show file tree
Hide file tree
Showing 6 changed files with 99 additions and 14 deletions.
2 changes: 1 addition & 1 deletion ...ase/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java
View file
Expand Up @@ -283,7 +283,7 @@ public boolean handleException(RequestDetails theRequestDetails, Throwable theEx

BaseServerResponseException bsre = (BaseServerResponseException)theException;
if (bsre.getOperationOutcome() == null) {

return super.handleException(theRequestDetails, theException, theServletRequest, theServletResponse);
}

streamResponse(theRequestDetails, theServletRequest, theServletResponse, bsre.getOperationOutcome());
Expand Down
10 changes: 6 additions & 4 deletions hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java
View file
Expand Up @@ -1050,7 +1050,7 @@ private Predicate createResourceLinkPathPredicate(String theParamName, CriteriaB
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;
}
Expand All @@ -1065,7 +1065,7 @@ private void createSort(CriteriaBuilder theBuilder, Root<ResourceTable> theFrom,
theOrders.add(theBuilder.desc(theFrom.get("myId")));
}

createSort(theBuilder, theFrom, theSort.getChain(), theOrders);
createSort(theBuilder, theFrom, theSort.getChain(), theOrders, thePredicates);
return;
}

Expand Down Expand Up @@ -1096,6 +1096,8 @@ private void createSort(CriteriaBuilder theBuilder, Root<ResourceTable> theFrom,
}

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));
Expand All @@ -1106,7 +1108,7 @@ private void createSort(CriteriaBuilder theBuilder, Root<ResourceTable> theFrom,
theOrders.add(theBuilder.desc(stringJoin.get(sortAttrName)));
}

createSort(theBuilder, theFrom, theSort.getChain(), theOrders);
createSort(theBuilder, theFrom, theSort.getChain(), theOrders, thePredicates);
}

@Override
Expand Down Expand Up @@ -1715,7 +1717,7 @@ public IBundleProvider search(final SearchParameterMap theParams) {
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>();
Expand Down
90 changes: 82 additions & 8 deletions hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/FhirResourceDaoDstu2Test.java
View file
Expand Up @@ -2374,47 +2374,121 @@ public void testSortByReference() {
assertThat(actual.subList(2, 4), containsInAnyOrder(id1, id3));
}

/**
* See #198
*/
@Test
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 testSortByString() {
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());
Expand Down
2 changes: 1 addition & 1 deletion hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml
View file
Expand Up @@ -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>

Expand Down
5 changes: 5 additions & 0 deletions hapi-fhir-jpaserver-uhnfhirtest/pom.xml
View file
Expand Up @@ -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>
Expand Down
4 changes: 4 additions & 0 deletions src/changes/changes.xml
View file
Expand Up @@ -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">
Expand Down

0 comments on commit 626f467

Please sign in to comment.