diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5917-chain-sort-mssql.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5917-chain-sort-mssql.yaml new file mode 100644 index 00000000000..1e94d65c58d --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/7_2_0/5917-chain-sort-mssql.yaml @@ -0,0 +1,4 @@ +--- +type: fix +issue: 5917 +title: "Fix chained sorts on strings when using MS Sql" diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java index d7c90a955db..685f96c16f8 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/QueryStack.java @@ -353,26 +353,39 @@ public class QueryStack { throw new InvalidRequestException(Msg.code(2289) + msg); } - BaseSearchParamPredicateBuilder chainedPredicateBuilder; - DbColumn[] sortColumn; + // add a left-outer join to a predicate for the target type, then sort on value columns(s). switch (targetSearchParameter.getParamType()) { case STRING: StringPredicateBuilder stringPredicateBuilder = mySqlBuilder.createStringPredicateBuilder(); - sortColumn = new DbColumn[] {stringPredicateBuilder.getColumnValueNormalized()}; - chainedPredicateBuilder = stringPredicateBuilder; - break; + addSortCustomJoin( + resourceLinkPredicateBuilder.getColumnTargetResourceId(), + stringPredicateBuilder, + stringPredicateBuilder.createHashIdentityPredicate(targetType, theChain)); + + mySqlBuilder.addSortString( + stringPredicateBuilder.getColumnValueNormalized(), theAscending, myUseAggregate); + return; + case TOKEN: TokenPredicateBuilder tokenPredicateBuilder = mySqlBuilder.createTokenPredicateBuilder(); - sortColumn = - new DbColumn[] {tokenPredicateBuilder.getColumnSystem(), tokenPredicateBuilder.getColumnValue() - }; - chainedPredicateBuilder = tokenPredicateBuilder; - break; + addSortCustomJoin( + resourceLinkPredicateBuilder.getColumnTargetResourceId(), + tokenPredicateBuilder, + tokenPredicateBuilder.createHashIdentityPredicate(targetType, theChain)); + + mySqlBuilder.addSortString(tokenPredicateBuilder.getColumnSystem(), theAscending, myUseAggregate); + mySqlBuilder.addSortString(tokenPredicateBuilder.getColumnValue(), theAscending, myUseAggregate); + return; + case DATE: DatePredicateBuilder datePredicateBuilder = mySqlBuilder.createDatePredicateBuilder(); - sortColumn = new DbColumn[] {datePredicateBuilder.getColumnValueLow()}; - chainedPredicateBuilder = datePredicateBuilder; - break; + addSortCustomJoin( + resourceLinkPredicateBuilder.getColumnTargetResourceId(), + datePredicateBuilder, + datePredicateBuilder.createHashIdentityPredicate(targetType, theChain)); + + mySqlBuilder.addSortDate(datePredicateBuilder.getColumnValueLow(), theAscending, myUseAggregate); + return; /* * Note that many of the options below aren't implemented because they @@ -417,16 +430,6 @@ public class QueryStack { + theParamName + "." + theChain + " as this parameter. Can not sort on chains of target type: " + targetSearchParameter.getParamType().name()); } - - addSortCustomJoin(resourceLinkPredicateBuilder.getColumnTargetResourceId(), chainedPredicateBuilder, null); - // Support chained sort - when there is no data to sort, the outer join will produce null columns. We need to - // include those too. - Condition predicate = chainedPredicateBuilder.createHashIdentityOrNullPredicate(targetType, theChain); - mySqlBuilder.addPredicate(predicate); - - for (DbColumn next : sortColumn) { - mySqlBuilder.addSortNumeric(next, theAscending, myUseAggregate); - } } public void addSortOnString(String theResourceName, String theParamName, boolean theAscending) { diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/BaseSearchParamPredicateBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/BaseSearchParamPredicateBuilder.java index 16c674b420b..30636077c72 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/BaseSearchParamPredicateBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/predicate/BaseSearchParamPredicateBuilder.java @@ -27,7 +27,6 @@ import ca.uhn.fhir.jpa.util.QueryParameterUtils; import com.healthmarketscience.sqlbuilder.BinaryCondition; import com.healthmarketscience.sqlbuilder.ComboCondition; import com.healthmarketscience.sqlbuilder.Condition; -import com.healthmarketscience.sqlbuilder.Conditions; import com.healthmarketscience.sqlbuilder.NotCondition; import com.healthmarketscience.sqlbuilder.SelectQuery; import com.healthmarketscience.sqlbuilder.UnaryCondition; @@ -97,16 +96,6 @@ public abstract class BaseSearchParamPredicateBuilder extends BaseJoiningPredica return BinaryCondition.equalTo(myColumnHashIdentity, hashIdentityVal); } - @Nonnull - public Condition createHashIdentityOrNullPredicate(String theResourceType, String theParamName) { - final long hashIdentity = BaseResourceIndexedSearchParam.calculateHashIdentity( - getPartitionSettings(), getRequestPartitionId(), theResourceType, theParamName); - final String hashIdentityVal = generatePlaceholder(hashIdentity); - return Conditions.or( - BinaryCondition.equalTo(myColumnHashIdentity, hashIdentityVal), - Conditions.isNull(myColumnHashIdentity)); - } - public Condition createPredicateParamMissingForNonReference( String theResourceName, String theParamName, Boolean theMissing, RequestPartitionId theRequestPartitionId) { ComboCondition condition = ComboCondition.and( diff --git a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QuerySandboxTest.java b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QuerySandbox.java similarity index 97% rename from hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QuerySandboxTest.java rename to hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QuerySandbox.java index e5dab50f26f..95ab54ddb19 100644 --- a/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QuerySandboxTest.java +++ b/hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4QuerySandbox.java @@ -54,10 +54,10 @@ import static org.junit.jupiter.api.Assertions.assertTrue; @DirtiesContext(classMode = DirtiesContext.ClassMode.AFTER_CLASS) @TestExecutionListeners(listeners = { DependencyInjectionTestExecutionListener.class - , FhirResourceDaoR4QuerySandboxTest.TestDirtiesContextTestExecutionListener.class + , FhirResourceDaoR4QuerySandbox.TestDirtiesContextTestExecutionListener.class }) -public class FhirResourceDaoR4QuerySandboxTest extends BaseJpaTest { - private static final Logger ourLog = LoggerFactory.getLogger(FhirResourceDaoR4QuerySandboxTest.class); +public class FhirResourceDaoR4QuerySandbox extends BaseJpaTest { + private static final Logger ourLog = LoggerFactory.getLogger(FhirResourceDaoR4QuerySandbox.class); @Autowired PlatformTransactionManager myTxManager; diff --git a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/UpliftedRefchainsAndChainedSortingR5Test.java b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/UpliftedRefchainsAndChainedSortingR5Test.java index 67512c2c7cc..50e2c2787c0 100644 --- a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/UpliftedRefchainsAndChainedSortingR5Test.java +++ b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/UpliftedRefchainsAndChainedSortingR5Test.java @@ -2,6 +2,7 @@ package ca.uhn.fhir.jpa.dao.r5; import ca.uhn.fhir.context.RuntimeSearchParam; import ca.uhn.fhir.jpa.api.config.JpaStorageSettings; +import ca.uhn.fhir.jpa.dao.TestDaoSearch; import ca.uhn.fhir.jpa.model.entity.StorageSettings; import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.jpa.test.config.TestHSearchAddInConfig; @@ -12,6 +13,8 @@ import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; import ca.uhn.fhir.util.BundleBuilder; import ca.uhn.fhir.util.HapiExtensions; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.r5.model.Composition; import org.hl7.fhir.r5.model.IdType; import org.hl7.fhir.r5.model.Bundle; @@ -29,6 +32,7 @@ import org.hl7.fhir.r5.model.SearchParameter; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; +import org.springframework.beans.factory.annotation.Autowired; import org.springframework.test.context.ContextConfiguration; import jakarta.annotation.Nonnull; @@ -41,9 +45,10 @@ import static org.hamcrest.Matchers.containsInAnyOrder; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.empty; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertTrue; import static org.junit.jupiter.api.Assertions.fail; -@ContextConfiguration(classes = TestHSearchAddInConfig.NoFT.class) +@ContextConfiguration(classes = { TestHSearchAddInConfig.NoFT.class, TestDaoSearch.Config.class }) @SuppressWarnings({"Duplicates"}) public class UpliftedRefchainsAndChainedSortingR5Test extends BaseJpaR5Test { public static final String PRACTITIONER_PR1 = "Practitioner/PR1"; @@ -54,6 +59,8 @@ public class UpliftedRefchainsAndChainedSortingR5Test extends BaseJpaR5Test { public static final String ENCOUNTER_E2 = "Encounter/E2"; public static final String ENCOUNTER_E3 = "Encounter/E3"; public static final String ORGANIZATION_O1 = "Organization/O1"; + @Autowired + protected TestDaoSearch myTestDaoSearch; @Override @BeforeEach @@ -724,7 +731,7 @@ public class UpliftedRefchainsAndChainedSortingR5Test extends BaseJpaR5Test { String querySql = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false); assertEquals(1, countMatches(querySql, "HFJ_SPIDX_STRING"), querySql); assertEquals(0, countMatches(querySql, "HASH_NORM_PREFIX"), querySql); - assertEquals(2, countMatches(querySql, "HASH_IDENTITY"), querySql); + assertEquals(1, countMatches(querySql, "HASH_IDENTITY"), querySql); assertEquals(1, countMatches(querySql, "HFJ_RES_LINK"), querySql); } @@ -760,7 +767,7 @@ public class UpliftedRefchainsAndChainedSortingR5Test extends BaseJpaR5Test { String querySql = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false); assertEquals(2, countMatches(querySql, "HFJ_SPIDX_STRING"), querySql); assertEquals(1, countMatches(querySql, "HASH_NORM_PREFIX"), querySql); - assertEquals(2, countMatches(querySql, "HASH_IDENTITY"), querySql); + assertEquals(1, countMatches(querySql, "HASH_IDENTITY"), querySql); assertEquals(2, countMatches(querySql, "HFJ_RES_LINK"), querySql); } @@ -794,7 +801,7 @@ public class UpliftedRefchainsAndChainedSortingR5Test extends BaseJpaR5Test { assertEquals(2, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size()); String querySql = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false); assertEquals(1, countMatches(querySql, "HFJ_SPIDX_TOKEN"), querySql); - assertEquals(2, countMatches(querySql, "HASH_IDENTITY"), querySql); + assertEquals(1, countMatches(querySql, "HASH_IDENTITY"), querySql); assertEquals(1, countMatches(querySql, "HFJ_RES_LINK"), querySql); } @@ -828,7 +835,7 @@ public class UpliftedRefchainsAndChainedSortingR5Test extends BaseJpaR5Test { assertEquals(2, myCaptureQueriesListener.getSelectQueriesForCurrentThread().size()); String querySql = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false); assertEquals(1, countMatches(querySql, "HFJ_SPIDX_DATE"), querySql); - assertEquals(2, countMatches(querySql, "HASH_IDENTITY"), querySql); + assertEquals(1, countMatches(querySql, "HASH_IDENTITY"), querySql); assertEquals(1, countMatches(querySql, "HFJ_RES_LINK"), querySql); } @@ -863,10 +870,51 @@ public class UpliftedRefchainsAndChainedSortingR5Test extends BaseJpaR5Test { String querySql = myCaptureQueriesListener.getSelectQueriesForCurrentThread().get(0).getSql(true, false); assertEquals(1, countMatches(querySql, "HFJ_SPIDX_STRING"), querySql); assertEquals(0, countMatches(querySql, "HASH_NORM_PREFIX"), querySql); - assertEquals(2, countMatches(querySql, "HASH_IDENTITY"), querySql); + assertEquals(1, countMatches(querySql, "HASH_IDENTITY"), querySql); assertEquals(1, countMatches(querySql, "HFJ_RES_LINK"), querySql); } + + @Test + void testChainedSortWithNulls() { + final IIdType practitionerId1 = createPractitioner(withFamily("Chan")); + final IIdType practitionerId2 = createPractitioner(withFamily("Jones")); + + final String id1 = createPatient(withFamily("Smithy")).getIdPart(); + final String id2 = createPatient(withFamily("Smithwick"), + withReference("generalPractitioner", practitionerId2)).getIdPart(); + final String id3 = createPatient( + withFamily("Smith"), + withReference("generalPractitioner", practitionerId1)).getIdPart(); + + + final IBundleProvider iBundleProvider = myTestDaoSearch.searchForBundleProvider("Patient?_total=ACCURATE&_sort=Practitioner:general-practitioner.family"); + final List allResources = iBundleProvider.getAllResources(); + assertEquals(3, iBundleProvider.size()); + assertEquals(3, allResources.size()); + + final List actualIds = allResources.stream().map(IBaseResource::getIdElement).map(IIdType::getIdPart).toList(); + assertTrue(actualIds.containsAll(List.of(id1, id2, id3))); + } + + @Test + void testChainedReverseStringSort() { + final IIdType practitionerId = createPractitioner(withFamily("Jones")); + + final String id1 = createPatient(withFamily("Smithy")).getIdPart(); + final String id2 = createPatient(withFamily("Smithwick")).getIdPart(); + final String id3 = createPatient( + withFamily("Smith"), + withReference("generalPractitioner", practitionerId)).getIdPart(); + + final IBundleProvider iBundleProvider = myTestDaoSearch.searchForBundleProvider("Patient?_total=ACCURATE&_sort=-Practitioner:general-practitioner.family"); + assertEquals(3, iBundleProvider.size()); + + final List allResources = iBundleProvider.getAllResources(); + final List actualIds = allResources.stream().map(IBaseResource::getIdElement).map(IIdType::getIdPart).toList(); + assertTrue(actualIds.containsAll(List.of(id3, id2, id1))); + } + /** * Observation:focus is a Reference(Any) so it can't be used in a sort chain because * this would be horribly, horribly inefficient. diff --git a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/database/BaseDatabaseVerificationIT.java b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/database/BaseDatabaseVerificationIT.java index e010f25669a..beeef22ff91 100644 --- a/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/database/BaseDatabaseVerificationIT.java +++ b/hapi-fhir-jpaserver-test-r5/src/test/java/ca/uhn/fhir/jpa/dao/r5/database/BaseDatabaseVerificationIT.java @@ -1,38 +1,24 @@ package ca.uhn.fhir.jpa.dao.r5.database; -import ca.uhn.fhir.batch2.jobs.export.BulkDataExportProvider; -import ca.uhn.fhir.batch2.jobs.expunge.DeleteExpungeProvider; -import ca.uhn.fhir.batch2.jobs.reindex.ReindexProvider; import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.jpa.api.dao.DaoRegistry; -import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao; import ca.uhn.fhir.jpa.api.dao.IFhirResourceDaoPatient; -import ca.uhn.fhir.jpa.api.dao.PatientEverythingParameters; +import ca.uhn.fhir.jpa.dao.TestDaoSearch; import ca.uhn.fhir.jpa.embedded.JpaEmbeddedDatabase; -import ca.uhn.fhir.jpa.fql.provider.HfqlRestProvider; -import ca.uhn.fhir.jpa.graphql.GraphQLProvider; import ca.uhn.fhir.jpa.migrate.HapiMigrationStorageSvc; import ca.uhn.fhir.jpa.migrate.MigrationTaskList; import ca.uhn.fhir.jpa.migrate.SchemaMigrator; import ca.uhn.fhir.jpa.migrate.dao.HapiMigrationDao; import ca.uhn.fhir.jpa.migrate.tasks.HapiFhirJpaMigrationTasks; -import ca.uhn.fhir.jpa.provider.DiffProvider; -import ca.uhn.fhir.jpa.provider.JpaCapabilityStatementProvider; -import ca.uhn.fhir.jpa.provider.ProcessMessageProvider; -import ca.uhn.fhir.jpa.provider.SubscriptionTriggeringProvider; -import ca.uhn.fhir.jpa.provider.TerminologyUploaderProvider; -import ca.uhn.fhir.jpa.provider.ValueSetOperationProvider; import ca.uhn.fhir.jpa.search.DatabaseBackedPagingProvider; +import ca.uhn.fhir.jpa.searchparam.SearchParameterMap; import ca.uhn.fhir.jpa.test.BaseJpaTest; import ca.uhn.fhir.jpa.test.config.TestR5Config; -import ca.uhn.fhir.narrative.DefaultThymeleafNarrativeGenerator; import ca.uhn.fhir.rest.api.EncodingEnum; -import ca.uhn.fhir.rest.api.server.IBundleProvider; +import ca.uhn.fhir.rest.api.SortSpec; import ca.uhn.fhir.rest.api.server.SystemRequestDetails; import ca.uhn.fhir.rest.client.api.IGenericClient; -import ca.uhn.fhir.rest.client.interceptor.LoggingInterceptor; import ca.uhn.fhir.rest.server.exceptions.ResourceGoneException; -import ca.uhn.fhir.rest.server.interceptor.CorsInterceptor; import ca.uhn.fhir.rest.server.provider.ResourceProviderFactory; import ca.uhn.fhir.test.utilities.ITestDataBuilder; import ca.uhn.fhir.test.utilities.server.RestfulServerConfigurerExtension; @@ -44,7 +30,6 @@ import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IIdType; import org.hl7.fhir.r5.model.Bundle; import org.hl7.fhir.r5.model.IdType; -import org.hl7.fhir.r5.model.IntegerType; import org.hl7.fhir.r5.model.Parameters; import org.hl7.fhir.r5.model.Patient; import org.junit.jupiter.api.Test; @@ -55,7 +40,6 @@ import org.junit.jupiter.params.provider.ValueSource; import org.slf4j.Logger; import org.slf4j.LoggerFactory; import org.springframework.beans.factory.annotation.Autowired; -import org.springframework.beans.factory.annotation.Qualifier; import org.springframework.context.annotation.Bean; import org.springframework.context.annotation.Configuration; import org.springframework.data.envers.repository.support.EnversRevisionRepositoryFactoryBean; @@ -63,10 +47,8 @@ import org.springframework.data.jpa.repository.config.EnableJpaRepositories; import org.springframework.test.context.ContextConfiguration; import org.springframework.test.context.junit.jupiter.SpringExtension; import org.springframework.transaction.PlatformTransactionManager; -import org.springframework.web.cors.CorsConfiguration; import javax.sql.DataSource; -import java.util.Arrays; import java.util.HashSet; import java.util.List; import java.util.Properties; @@ -80,7 +62,7 @@ import static org.junit.jupiter.api.Assertions.assertThrows; @ExtendWith(SpringExtension.class) @EnableJpaRepositories(repositoryFactoryBeanClass = EnversRevisionRepositoryFactoryBean.class) -@ContextConfiguration(classes = {BaseDatabaseVerificationIT.TestConfig.class}) +@ContextConfiguration(classes = {BaseDatabaseVerificationIT.TestConfig.class, TestDaoSearch.Config.class}) public abstract class BaseDatabaseVerificationIT extends BaseJpaTest implements ITestDataBuilder { private static final Logger ourLog = LoggerFactory.getLogger(BaseDatabaseVerificationIT.class); private static final String MIGRATION_TABLENAME = "MIGRATIONS"; @@ -109,6 +91,9 @@ public abstract class BaseDatabaseVerificationIT extends BaseJpaTest implements @Autowired private DatabaseBackedPagingProvider myPagingProvider; + @Autowired + TestDaoSearch myTestDaoSearch; + @RegisterExtension protected RestfulServerExtension myServer = new RestfulServerExtension(FhirContext.forR5Cached()); @@ -175,6 +160,20 @@ public abstract class BaseDatabaseVerificationIT extends BaseJpaTest implements assertThat(values.toString(), values, containsInAnyOrder(expectedIds.toArray(new String[0]))); } + @Test + void testChainedSort() { + // given + + // when + SearchParameterMap map = SearchParameterMap + .newSynchronous() + .setSort(new SortSpec("Practitioner:general-practitioner.family")); + myCaptureQueriesListener.clear(); + myPatientDao.search(map, mySrd); + + } + + @Configuration @@ -222,8 +221,8 @@ public abstract class BaseDatabaseVerificationIT extends BaseJpaTest implements } public static class JpaDatabaseContextConfigParamObject { - private JpaEmbeddedDatabase myJpaEmbeddedDatabase; - private String myDialect; + final JpaEmbeddedDatabase myJpaEmbeddedDatabase; + final String myDialect; public JpaDatabaseContextConfigParamObject(JpaEmbeddedDatabase theJpaEmbeddedDatabase, String theDialect) { myJpaEmbeddedDatabase = theJpaEmbeddedDatabase;