From 836948010ff89ed4b8b0653412bd5d0aada6c90a Mon Sep 17 00:00:00 2001 From: Justin Dar Date: Thu, 9 Sep 2021 16:10:09 -0700 Subject: [PATCH 1/6] Fixes issue (#2958) where Procedure?patient caused inconsistent results and ensured that such requests gave http 400 --- ...ure-patient-consistent-error-handling.yaml | 6 ++++ .../jpa/search/SearchCoordinatorSvcImpl.java | 5 +++ .../r4/ResourceProviderR4CacheTest.java | 32 +++++++++++++++++++ .../jpa/searchparam/SearchParameterMap.java | 4 +++ 4 files changed, 47 insertions(+) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2958-procedure-patient-consistent-error-handling.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2958-procedure-patient-consistent-error-handling.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2958-procedure-patient-consistent-error-handling.yaml new file mode 100644 index 00000000000..f90ca63a917 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2958-procedure-patient-consistent-error-handling.yaml @@ -0,0 +1,6 @@ +--- +type: fix +issue: 2958 +jira: SMILE-643 +title: "Fixed issue where the processing of queries like Procedure?patient= before a cache search would cause the parameter key to be removed. +Additionally, ensured that requests like Procedure?patient cause HTTP 400 Bad Request instead of HTTP 500 Internal Error." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java index 4d266271bd9..8c1303f1571 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java @@ -1014,6 +1014,9 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { logged = true; ourLog.warn("Failed during search due to invalid request: {}", t.toString()); } + }else if (t instanceof java.lang.IllegalArgumentException && t.getMessage().contentEquals("The validated expression is false")) { + logged = true; + ourLog.warn("Failed during search due to invalid request: {}", t.toString()); } if (!logged) { @@ -1028,6 +1031,8 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { int failureCode = InternalErrorException.STATUS_CODE; if (t instanceof BaseServerResponseException) { failureCode = ((BaseServerResponseException) t).getStatusCode(); + }else if(t instanceof java.lang.IllegalArgumentException && t.getMessage().contentEquals("The validated expression is false")) { + failureCode = Constants.STATUS_HTTP_400_BAD_REQUEST; } if (System.getProperty(UNIT_TEST_CAPTURE_STACK) != null) { diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4CacheTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4CacheTest.java index 834d93ba816..fb33fcf46da 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4CacheTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4CacheTest.java @@ -7,7 +7,11 @@ import ca.uhn.fhir.parser.StrictErrorHandler; import ca.uhn.fhir.rest.api.CacheControlDirective; import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.client.interceptor.CapturingInterceptor; +import ca.uhn.fhir.rest.server.exceptions.BaseServerResponseException; +import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import com.ctc.wstx.shaded.msv_core.verifier.jarv.Const; +import org.hl7.fhir.r4.model.Base; import org.hl7.fhir.r4.model.Bundle; import org.hl7.fhir.r4.model.IdType; import org.hl7.fhir.r4.model.Patient; @@ -27,6 +31,7 @@ import static org.hamcrest.Matchers.lessThan; import static org.hamcrest.core.IsNot.not; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; public class ResourceProviderR4CacheTest extends BaseResourceProviderR4Test { @@ -227,5 +232,32 @@ public class ResourceProviderR4CacheTest extends BaseResourceProviderR4Test { assertEquals(1, resp2.getEntry().size()); } + @Test + public void testProcedurePatient(){ + Bundle resp2 = myClient + .search() + .byUrl("Procedure") + .returnBundle(Bundle.class) + .execute(); + + BaseServerResponseException exception = assertThrows(BaseServerResponseException.class, () -> {myClient + .search() + .byUrl("Procedure?patient=") + .returnBundle(Bundle.class) + .execute();}); + + assertEquals(Constants.STATUS_HTTP_400_BAD_REQUEST, exception.getStatusCode()); + } + + @Test + public void testPatient(){ + BaseServerResponseException exception = assertThrows(BaseServerResponseException.class, () -> {myClient + .search() + .byUrl("Procedure?patient=") + .returnBundle(Bundle.class) + .execute();}); + + assertEquals(Constants.STATUS_HTTP_400_BAD_REQUEST, exception.getStatusCode()); + } } diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java index 4ad522cf3be..061531f4cc1 100644 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java @@ -396,11 +396,15 @@ public class SearchParameterMap implements Serializable { for (List nextValuesAndIn : nextValuesAndsIn) { List nextValuesOrsOut = new ArrayList<>(); + /* for (IQueryParameterType nextValueOrIn : nextValuesAndIn) { if (nextValueOrIn.getMissing() != null || isNotBlank(nextValueOrIn.getValueAsQueryToken(theCtx))) { nextValuesOrsOut.add(nextValueOrIn); } } + */ + + nextValuesOrsOut.addAll(nextValuesAndIn); nextValuesOrsOut.sort(new QueryParameterTypeComparator(theCtx)); From 77845d66f76913a1c743450b62cd0149387aef03 Mon Sep 17 00:00:00 2001 From: Justin Dar Date: Fri, 10 Sep 2021 10:54:33 -0700 Subject: [PATCH 2/6] Improved code formatting --- ...procedure-patient-consistent-error-handling.yaml | 2 +- .../fhir/jpa/search/SearchCoordinatorSvcImpl.java | 8 +++----- .../provider/r4/ResourceProviderR4CacheTest.java | 13 +++++++++---- .../fhir/jpa/searchparam/SearchParameterMap.java | 7 ------- 4 files changed, 13 insertions(+), 17 deletions(-) diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2958-procedure-patient-consistent-error-handling.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2958-procedure-patient-consistent-error-handling.yaml index f90ca63a917..4cee4993a52 100644 --- a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2958-procedure-patient-consistent-error-handling.yaml +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/2958-procedure-patient-consistent-error-handling.yaml @@ -3,4 +3,4 @@ type: fix issue: 2958 jira: SMILE-643 title: "Fixed issue where the processing of queries like Procedure?patient= before a cache search would cause the parameter key to be removed. -Additionally, ensured that requests like Procedure?patient cause HTTP 400 Bad Request instead of HTTP 500 Internal Error." +Additionally, ensured that requests like Procedure?patient= cause HTTP 400 Bad Request instead of HTTP 500 Internal Error." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java index 8c1303f1571..c03fc350699 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java @@ -1014,9 +1014,6 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { logged = true; ourLog.warn("Failed during search due to invalid request: {}", t.toString()); } - }else if (t instanceof java.lang.IllegalArgumentException && t.getMessage().contentEquals("The validated expression is false")) { - logged = true; - ourLog.warn("Failed during search due to invalid request: {}", t.toString()); } if (!logged) { @@ -1031,8 +1028,6 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { int failureCode = InternalErrorException.STATUS_CODE; if (t instanceof BaseServerResponseException) { failureCode = ((BaseServerResponseException) t).getStatusCode(); - }else if(t instanceof java.lang.IllegalArgumentException && t.getMessage().contentEquals("The validated expression is false")) { - failureCode = Constants.STATUS_HTTP_400_BAD_REQUEST; } if (System.getProperty(UNIT_TEST_CAPTURE_STACK) != null) { @@ -1212,6 +1207,9 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } catch (IOException e) { ourLog.error("IO failure during database access", e); throw new InternalErrorException(e); + } catch (IllegalArgumentException e) { + ourLog.error("Illegal Argument during database access", e); + throw new InvalidRequestException("Parameter value missing in request", e); } } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4CacheTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4CacheTest.java index fb33fcf46da..eca83de0556 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4CacheTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4CacheTest.java @@ -17,6 +17,7 @@ import org.hl7.fhir.r4.model.IdType; import org.hl7.fhir.r4.model.Patient; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -31,6 +32,7 @@ import static org.hamcrest.Matchers.lessThan; import static org.hamcrest.core.IsNot.not; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.hamcrest.MatcherAssert.assertThat; +import static org.junit.jupiter.api.Assertions.assertNotEquals; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.fail; @@ -233,24 +235,27 @@ public class ResourceProviderR4CacheTest extends BaseResourceProviderR4Test { } @Test - public void testProcedurePatient(){ - Bundle resp2 = myClient + public void testParamWithNoValueIsConsideredForCacheResults(){ + // Given: We populate the cache by searching + myClient .search() .byUrl("Procedure") .returnBundle(Bundle.class) .execute(); + // When: We search Procedure?patient= BaseServerResponseException exception = assertThrows(BaseServerResponseException.class, () -> {myClient .search() .byUrl("Procedure?patient=") .returnBundle(Bundle.class) .execute();}); - assertEquals(Constants.STATUS_HTTP_400_BAD_REQUEST, exception.getStatusCode()); + // Then: We do not get a cache hit + assertNotEquals(Constants.STATUS_HTTP_200_OK, exception.getStatusCode()); } @Test - public void testPatient(){ + public void testReturn400ForParameterWithNoValue(){ BaseServerResponseException exception = assertThrows(BaseServerResponseException.class, () -> {myClient .search() .byUrl("Procedure?patient=") diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java index 061531f4cc1..1e88bb00673 100644 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/SearchParameterMap.java @@ -396,13 +396,6 @@ public class SearchParameterMap implements Serializable { for (List nextValuesAndIn : nextValuesAndsIn) { List nextValuesOrsOut = new ArrayList<>(); - /* - for (IQueryParameterType nextValueOrIn : nextValuesAndIn) { - if (nextValueOrIn.getMissing() != null || isNotBlank(nextValueOrIn.getValueAsQueryToken(theCtx))) { - nextValuesOrsOut.add(nextValueOrIn); - } - } - */ nextValuesOrsOut.addAll(nextValuesAndIn); From 2163e6e54870fbbbfcc01d67c6720e542d65e1b6 Mon Sep 17 00:00:00 2001 From: Justin Dar Date: Fri, 10 Sep 2021 12:36:11 -0700 Subject: [PATCH 3/6] Fixed error that previous fix caused --- .../java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java | 8 +++++++- .../ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java | 3 --- 2 files changed, 7 insertions(+), 4 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java index b667e3a1375..0674ba87ffe 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java @@ -34,6 +34,7 @@ import ca.uhn.fhir.jpa.util.MemoryCacheService; import ca.uhn.fhir.jpa.util.QueryChunker; import ca.uhn.fhir.model.primitive.IdDt; import ca.uhn.fhir.rest.api.server.storage.ResourcePersistentId; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import ca.uhn.fhir.rest.server.exceptions.PreconditionFailedException; import ca.uhn.fhir.rest.server.exceptions.ResourceNotFoundException; import com.google.common.annotations.VisibleForTesting; @@ -204,7 +205,12 @@ public class IdHelperService { */ @Nonnull public List resolveResourcePersistentIdsWithCache(RequestPartitionId theRequestPartitionId, List theIds) { - theIds.forEach(id -> Validate.isTrue(id.hasIdPart())); + try { + theIds.forEach(id -> Validate.isTrue(id.hasIdPart())); + } catch (IllegalArgumentException e) { + ourLog.error("Illegal Argument during database access", e); + throw new InvalidRequestException("Parameter value missing in request", e); + } if (theIds.isEmpty()) { return Collections.emptyList(); diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java index c03fc350699..4d266271bd9 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/search/SearchCoordinatorSvcImpl.java @@ -1207,9 +1207,6 @@ public class SearchCoordinatorSvcImpl implements ISearchCoordinatorSvc { } catch (IOException e) { ourLog.error("IO failure during database access", e); throw new InternalErrorException(e); - } catch (IllegalArgumentException e) { - ourLog.error("Illegal Argument during database access", e); - throw new InvalidRequestException("Parameter value missing in request", e); } } } From 46936e17046cea64f5848f79912c341015b3d129 Mon Sep 17 00:00:00 2001 From: Justin Dar Date: Mon, 13 Sep 2021 11:26:21 -0700 Subject: [PATCH 4/6] Fixed code formatting and better way of fixing issue --- .../ca/uhn/fhir/jpa/dao/index/IdHelperService.java | 10 +++++----- .../jpa/provider/r4/ResourceProviderR4CacheTest.java | 6 ++---- 2 files changed, 7 insertions(+), 9 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java index 0674ba87ffe..039eb3b6507 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java @@ -205,11 +205,11 @@ public class IdHelperService { */ @Nonnull public List resolveResourcePersistentIdsWithCache(RequestPartitionId theRequestPartitionId, List theIds) { - try { - theIds.forEach(id -> Validate.isTrue(id.hasIdPart())); - } catch (IllegalArgumentException e) { - ourLog.error("Illegal Argument during database access", e); - throw new InvalidRequestException("Parameter value missing in request", e); + for (IIdType id : theIds) { + if (!id.hasIdPart()) { + ourLog.error("Illegal Argument during database access"); + throw new InvalidRequestException("Parameter value missing in request"); + } } if (theIds.isEmpty()) { diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4CacheTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4CacheTest.java index eca83de0556..ba72739616c 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4CacheTest.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/r4/ResourceProviderR4CacheTest.java @@ -8,16 +8,12 @@ import ca.uhn.fhir.rest.api.CacheControlDirective; import ca.uhn.fhir.rest.api.Constants; import ca.uhn.fhir.rest.client.interceptor.CapturingInterceptor; import ca.uhn.fhir.rest.server.exceptions.BaseServerResponseException; -import ca.uhn.fhir.rest.server.exceptions.InternalErrorException; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; -import com.ctc.wstx.shaded.msv_core.verifier.jarv.Const; -import org.hl7.fhir.r4.model.Base; import org.hl7.fhir.r4.model.Bundle; import org.hl7.fhir.r4.model.IdType; import org.hl7.fhir.r4.model.Patient; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.slf4j.Logger; import org.slf4j.LoggerFactory; @@ -256,12 +252,14 @@ public class ResourceProviderR4CacheTest extends BaseResourceProviderR4Test { @Test public void testReturn400ForParameterWithNoValue(){ + // When: We search Procedure?patient= BaseServerResponseException exception = assertThrows(BaseServerResponseException.class, () -> {myClient .search() .byUrl("Procedure?patient=") .returnBundle(Bundle.class) .execute();}); + // Then: We get a HTTP 400 Bad Request assertEquals(Constants.STATUS_HTTP_400_BAD_REQUEST, exception.getStatusCode()); } From 876768718d95ef9ecbd9ea7f720d0a7dc84c9e9e Mon Sep 17 00:00:00 2001 From: Justin Dar Date: Mon, 13 Sep 2021 11:29:36 -0700 Subject: [PATCH 5/6] Remove log --- .../src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java | 1 - 1 file changed, 1 deletion(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java index 1582d1c0738..1f9908c458c 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/index/IdHelperService.java @@ -207,7 +207,6 @@ public class IdHelperService { public List resolveResourcePersistentIdsWithCache(RequestPartitionId theRequestPartitionId, List theIds) { for (IIdType id : theIds) { if (!id.hasIdPart()) { - ourLog.error("Illegal Argument during database access"); throw new InvalidRequestException("Parameter value missing in request"); } } From 67cf058318f38d9484a490f18ee5dea370186c1e Mon Sep 17 00:00:00 2001 From: jamesagnew Date: Mon, 13 Sep 2021 19:29:49 -0400 Subject: [PATCH 6/6] Add changelog --- .../5_4_0/2457-fix-single-threaded-search-execution.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_4_0/2457-fix-single-threaded-search-execution.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_4_0/2457-fix-single-threaded-search-execution.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_4_0/2457-fix-single-threaded-search-execution.yaml new file mode 100644 index 00000000000..ccedc14c04b --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_4_0/2457-fix-single-threaded-search-execution.yaml @@ -0,0 +1,5 @@ +--- +type: perf +issue: 2457 +title: "A regression in HAPI FHIR 5.3.0 resulted in concurrent searches being executed in a sequential + (and not parallel) fashion in some circumstances."