From 626f4677e7d380301fe15b723457957e9b547fc6 Mon Sep 17 00:00:00 2001 From: jamesagnew Date: Sat, 18 Jul 2015 15:29:24 -0400 Subject: [PATCH] Fix #198 - Sorting should only sort on the individual parameter searched on, not all params of the same type --- .../ResponseHighlighterInterceptor.java | 2 +- .../fhir/jpa/dao/BaseHapiFhirResourceDao.java | 10 ++- .../jpa/dao/FhirResourceDaoDstu2Test.java | 90 +++++++++++++++++-- .../src/test/resources/logback-test.xml | 2 +- hapi-fhir-jpaserver-uhnfhirtest/pom.xml | 5 ++ src/changes/changes.xml | 4 + 6 files changed, 99 insertions(+), 14 deletions(-) diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java index 98b49cec3cd5..a2aa465e4c56 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java +++ b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/server/interceptor/ResponseHighlighterInterceptor.java @@ -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()); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java index 4bc74db63d43..a8cb8718db51 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirResourceDao.java @@ -1050,7 +1050,7 @@ private Predicate createResourceLinkPathPredicate(String theParamName, CriteriaB return type; } - private void createSort(CriteriaBuilder theBuilder, Root theFrom, SortSpec theSort, List theOrders) { + private void createSort(CriteriaBuilder theBuilder, Root theFrom, SortSpec theSort, List theOrders, List thePredicates) { if (theSort == null || isBlank(theSort.getParamName())) { return; } @@ -1065,7 +1065,7 @@ private void createSort(CriteriaBuilder theBuilder, Root theFrom, 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 @@ private void createSort(CriteriaBuilder theBuilder, Root 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)); @@ -1106,7 +1108,7 @@ private void createSort(CriteriaBuilder theBuilder, Root theFrom, 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 IBundleProvider search(final SearchParameterMap theParams) { CriteriaQuery cq = builder.createTupleQuery(); Root 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 originalPids = loadPids; loadPids = new LinkedHashSet(); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/FhirResourceDaoDstu2Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/FhirResourceDaoDstu2Test.java index b64e08d4a33a..5f30d358e306 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/FhirResourceDaoDstu2Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/FhirResourceDaoDstu2Test.java @@ -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 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 extractNames(IBundleProvider theSearch) { + ArrayList retVal = new ArrayList(); + 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 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()); diff --git a/hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml b/hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml index 658cd6bad1c2..c7b349bb74f7 100644 --- a/hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml +++ b/hapi-fhir-jpaserver-base/src/test/resources/logback-test.xml @@ -18,7 +18,7 @@ - + diff --git a/hapi-fhir-jpaserver-uhnfhirtest/pom.xml b/hapi-fhir-jpaserver-uhnfhirtest/pom.xml index 3d55b36529ba..7c3e56638829 100644 --- a/hapi-fhir-jpaserver-uhnfhirtest/pom.xml +++ b/hapi-fhir-jpaserver-uhnfhirtest/pom.xml @@ -30,6 +30,11 @@ hapi-fhir-structures-dstu2 1.2-SNAPSHOT + + ca.uhn.hapi.fhir + hapi-fhir-structures-hl7org-dstu2 + 1.2-SNAPSHOT + ca.uhn.hapi.fhir hapi-fhir-testpage-overlay diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 2733a86f1f52..acbbfccb81f9 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -28,6 +28,10 @@ count extensions exported in the Conformance statement by the JPA server. + + 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! +