From 207558dba3dc9ad6f500fd52ff899e05b479720a Mon Sep 17 00:00:00 2001 From: Jason Roberts Date: Tue, 7 Sep 2021 17:27:22 -0400 Subject: [PATCH] code review feedback --- .../jpa/search/builder/SearchBuilder.java | 4 +- ...rchTest.java => ChainingR4SearchTest.java} | 71 ++++++------------- .../r4/FhirResourceDaoR4ContainedTest.java | 4 +- 3 files changed, 27 insertions(+), 52 deletions(-) rename hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/{ChainedContainedR4SearchTest.java => ChainingR4SearchTest.java} (81%) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java index 5152973bfff..02414ae822d 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/builder/SearchBuilder.java @@ -373,7 +373,7 @@ public class SearchBuilder implements ISearchBuilder { SearchQueryBuilder sqlBuilder = new SearchQueryBuilder(myContext, myDaoConfig.getModelConfig(), myPartitionSettings, myRequestPartitionId, sqlBuilderResourceName, mySqlBuilderFactory, myDialectProvider, theCount); QueryStack queryStack3 = new QueryStack(theParams, myDaoConfig, myDaoConfig.getModelConfig(), myContext, sqlBuilder, mySearchParamRegistry, myPartitionSettings); - if (theParams.keySet().size() > 1 || theParams.getSort() != null || theParams.keySet().contains(Constants.PARAM_HAS) || isTraverseContainedReferenceAtRoot(theParams)) { + if (theParams.keySet().size() > 1 || theParams.getSort() != null || theParams.keySet().contains(Constants.PARAM_HAS) || isPotentiallyContainedReferenceParameterExistsAtRoot(theParams)) { List activeComboParams = mySearchParamRegistry.getActiveComboSearchParams(myResourceName, theParams.keySet()); if (activeComboParams.isEmpty()) { sqlBuilder.setNeedResourceTableRoot(true); @@ -483,7 +483,7 @@ public class SearchBuilder implements ISearchBuilder { return Optional.of(executor); } - private boolean isTraverseContainedReferenceAtRoot(SearchParameterMap theParams) { + private boolean isPotentiallyContainedReferenceParameterExistsAtRoot(SearchParameterMap theParams) { return myModelConfig.isIndexOnContainedResources() && theParams.values().stream() .flatMap(Collection::stream) .flatMap(Collection::stream) diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ChainedContainedR4SearchTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ChainingR4SearchTest.java similarity index 81% rename from hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ChainedContainedR4SearchTest.java rename to hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ChainingR4SearchTest.java index 003f68a9555..6c96220eb19 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ChainedContainedR4SearchTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/ChainingR4SearchTest.java @@ -28,9 +28,7 @@ import static org.hamcrest.Matchers.contains; import static org.junit.jupiter.api.Assertions.assertEquals; -public class ChainedContainedR4SearchTest extends BaseJpaR4Test { - - private static final org.slf4j.Logger ourLog = org.slf4j.LoggerFactory.getLogger(ChainedContainedR4SearchTest.class); +public class ChainingR4SearchTest extends BaseJpaR4Test { @Autowired MatchUrlService myMatchUrlService; @@ -77,16 +75,12 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { obs.getSubject().setReference(p.getId()); oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); - - ourLog.info("Input: {}", myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs)); } String url = "/Observation?subject.name=Smith"; // execute - myCaptureQueriesListener.clear(); List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); - ourLog.info(">>> " + myCaptureQueriesListener.getSelectQueriesForCurrentThread()); // validate assertEquals(1L, oids.size()); @@ -95,6 +89,7 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { @Test public void testShouldResolveATwoLinkChainWithAContainedResource() throws Exception { + // setup IIdType oid1; { @@ -109,16 +104,14 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { obs.getSubject().setReference("#pat"); oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); - - ourLog.info("Input: {}", myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs)); } String url = "/Observation?subject.name=Smith"; -// String url = "/Observation?subject.name=Smith&value-string=Test"; - myCaptureQueriesListener.clear(); - List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); - ourLog.info(">>> " + myCaptureQueriesListener.getSelectQueriesForCurrentThread()); + // execute + List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); + + // validate assertEquals(1L, oids.size()); assertThat(oids, contains(oid1.getIdPart())); } @@ -146,16 +139,12 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { obs.getSubject().setReference(p.getId()); oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); - - ourLog.info("Input: {}", myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs)); } String url = "/Observation?subject.organization.name=HealthCo"; // execute - myCaptureQueriesListener.clear(); List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); - ourLog.info(">>> " + myCaptureQueriesListener.getSelectQueriesForCurrentThread()); // validate assertEquals(1L, oids.size()); @@ -165,9 +154,10 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { @Test public void testShouldResolveAThreeLinkChainWithAContainedResourceAtTheEndOfTheChain() throws Exception { // This is the case that is most relevant to SMILE-2899 + + // setup IIdType oid1; - myCaptureQueriesListener.clear(); { Organization org = new Organization(); org.setId("org"); @@ -185,16 +175,14 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { obs.getSubject().setReference(p.getId()); oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); - - ourLog.info("Input: {}", myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs)); } - ourLog.info("Data setup SQL: " + myCaptureQueriesListener.getInsertQueriesForCurrentThread()); String url = "/Observation?subject.organization.name=HealthCo"; - myCaptureQueriesListener.clear(); - List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); - ourLog.info("Data retrieval SQL: " + myCaptureQueriesListener.getSelectQueriesForCurrentThread()); + // execute + List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); + + // validate assertEquals(1L, oids.size()); assertThat(oids, contains(oid1.getIdPart())); } @@ -204,6 +192,7 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { public void testShouldResolveAThreeLinkChainWithAContainedResourceAtTheBeginningOfTheChain() throws Exception { // We do not currently support this case - we may not be indexing the references of contained resources + // setup IIdType oid1; { @@ -223,15 +212,14 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { obs.getSubject().setReference("#pat"); oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); - - ourLog.info("Input: {}", myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs)); } String url = "/Observation?subject.organization.name=HealthCo"; - myCaptureQueriesListener.clear(); - List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); - ourLog.info(">>> " + myCaptureQueriesListener.getSelectQueriesForCurrentThread()); + // execute + List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); + + // validate assertEquals(1L, oids.size()); assertThat(oids, contains(oid1.getIdPart())); } @@ -259,16 +247,12 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { obs.getSubject().setReference(p.getId()); oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); - - ourLog.info("Input: {}", myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs)); } String url = "/Observation?subject:Patient.organization:Organization.name=HealthCo"; // execute - myCaptureQueriesListener.clear(); List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); - ourLog.info(">>> " + myCaptureQueriesListener.getSelectQueriesForCurrentThread()); // validate assertEquals(1L, oids.size()); @@ -278,9 +262,10 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { @Test public void testShouldResolveAThreeLinkChainWithQualifiersWithAContainedResourceAtTheEndOfTheChain() throws Exception { // This is the case that is most relevant to SMILE-2899 + + // setup IIdType oid1; - myCaptureQueriesListener.clear(); { Organization org = new Organization(); org.setId("org"); @@ -298,16 +283,14 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { obs.getSubject().setReference(p.getId()); oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); - - ourLog.info("Input: {}", myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs)); } - ourLog.info("Data setup SQL: " + myCaptureQueriesListener.getInsertQueriesForCurrentThread()); String url = "/Observation?subject:Patient.organization:Organization.name=HealthCo"; - myCaptureQueriesListener.clear(); - List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); - ourLog.info("Data retrieval SQL: " + myCaptureQueriesListener.getSelectQueriesForCurrentThread()); + // execute + List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); + + // validate assertEquals(1L, oids.size()); assertThat(oids, contains(oid1.getIdPart())); } @@ -340,16 +323,12 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { obs.getSubject().setReference(p.getId()); oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); - - ourLog.info("Input: {}", myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs)); } String url = "/Observation?subject.organization.partof.name=HealthCo"; // execute - myCaptureQueriesListener.clear(); List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); - ourLog.info(">>> " + myCaptureQueriesListener.getSelectQueriesForCurrentThread()); // validate assertEquals(1L, oids.size()); @@ -384,16 +363,12 @@ public class ChainedContainedR4SearchTest extends BaseJpaR4Test { obs.getSubject().setReference(p.getId()); oid1 = myObservationDao.create(obs, mySrd).getId().toUnqualifiedVersionless(); - - ourLog.info("Input: {}", myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(obs)); } String url = "/Observation?subject.organization.partof.name=HealthCo"; // execute - myCaptureQueriesListener.clear(); List oids = searchAndReturnUnqualifiedVersionlessIdValues(url); - ourLog.info(">>> " + myCaptureQueriesListener.getSelectQueriesForCurrentThread()); // validate assertEquals(1L, oids.size()); diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ContainedTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ContainedTest.java index 2074f5ab126..a6f26615bbc 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ContainedTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4ContainedTest.java @@ -114,9 +114,9 @@ public class FhirResourceDaoR4ContainedTest extends BaseJpaR4Test { map.add("subject", new ReferenceParam("name", "Smith")); map.setSearchContainedMode(SearchContainedModeEnum.TRUE); map.setLoadSynchronous(true); - myCaptureQueriesListener.clear(); + assertThat(toUnqualifiedVersionlessIdValues(myObservationDao.search(map)), containsInAnyOrder(toValues(id))); - ourLog.info(">>> " + myCaptureQueriesListener.getSelectQueriesForCurrentThread()); + }