From dbbff1fd16bee7dd96d8fa9358ead26da55cf580 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Fri, 21 Aug 2020 21:00:55 -0400 Subject: [PATCH 1/4] Allow HTTP 204 response to transaction (#2051) * Allow HTTP 204 response to transaction * Add changelog * Test fix --- .../uhn/fhir/rest/client/impl/BaseClient.java | 4 +++ .../2051-allow-204-transaction-response.yaml | 5 ++++ .../rest/client/GenericClientDstu2Test.java | 27 ------------------- .../fhir/rest/client/GenericClientR4Test.java | 27 +++++++++++++++++++ 4 files changed, 36 insertions(+), 27 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2051-allow-204-transaction-response.yaml diff --git a/hapi-fhir-client/src/main/java/ca/uhn/fhir/rest/client/impl/BaseClient.java b/hapi-fhir-client/src/main/java/ca/uhn/fhir/rest/client/impl/BaseClient.java index 8aad6fc48d7..bb245ae08bd 100644 --- a/hapi-fhir-client/src/main/java/ca/uhn/fhir/rest/client/impl/BaseClient.java +++ b/hapi-fhir-client/src/main/java/ca/uhn/fhir/rest/client/impl/BaseClient.java @@ -567,6 +567,10 @@ public abstract class BaseClient implements IRestfulClient { @Override public T invokeClient(String theResponseMimeType, InputStream theResponseInputStream, int theResponseStatusCode, Map> theHeaders) throws BaseServerResponseException { + if (theResponseStatusCode == Constants.STATUS_HTTP_204_NO_CONTENT) { + return null; + } + EncodingEnum respType = EncodingEnum.forContentType(theResponseMimeType); if (respType == null) { if (myAllowHtmlResponse && theResponseMimeType.toLowerCase().contains(Constants.CT_HTML) && myReturnType != null) { diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2051-allow-204-transaction-response.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2051-allow-204-transaction-response.yaml new file mode 100644 index 00000000000..67093b148c1 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2051-allow-204-transaction-response.yaml @@ -0,0 +1,5 @@ +--- +type: add +issue: 2051 +title: The FHIR Client will now accept an HTTP 204 NO CONTENT as a response to a write operation such + as a create/update/transaction/etc. diff --git a/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/client/GenericClientDstu2Test.java b/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/client/GenericClientDstu2Test.java index 415e46da9c4..a6b755da160 100644 --- a/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/client/GenericClientDstu2Test.java +++ b/hapi-fhir-structures-dstu2/src/test/java/ca/uhn/fhir/rest/client/GenericClientDstu2Test.java @@ -2397,33 +2397,6 @@ public class GenericClientDstu2Test { assertEquals("Patient/2/_history/2", response.getEntry().get(1).getResponse().getLocation()); } - @Test - public void testTransactionHandle204NoBody() throws Exception { - - IGenericClient client = ourCtx.newRestfulGenericClient("http://example.com/fhir"); - - ca.uhn.fhir.model.dstu2.resource.Bundle bundle = new Bundle(); - bundle.setType(BundleTypeEnum.TRANSACTION); - - ca.uhn.fhir.model.dstu2.resource.Bundle.Entry entry = bundle.addEntry(); - entry.setResource(new Patient()); - entry.getRequest().setMethod(HTTPVerbEnum.PUT); - - - - ArgumentCaptor capt = ArgumentCaptor.forClass(HttpUriRequest.class); - when(myHttpClient.execute(capt.capture())).thenReturn(myHttpResponse); - when(myHttpResponse.getStatusLine()).thenReturn(new BasicStatusLine(new ProtocolVersion("HTTP", 1, 1), Constants.STATUS_HTTP_204_NO_CONTENT, "")); - when(myHttpResponse.getEntity() ).thenReturn(null); - - try { - client.transaction().withBundle(bundle).execute(); - fail("Should throw an exception"); - } catch (NonFhirResponseException e) { - assertEquals(Constants.STATUS_HTTP_204_NO_CONTENT, e.getStatusCode()); - } - } - @Test public void testUpdateConditional() throws Exception { ArgumentCaptor capt = ArgumentCaptor.forClass(HttpUriRequest.class); diff --git a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/client/GenericClientR4Test.java b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/client/GenericClientR4Test.java index ff5921f3202..2d123673c70 100644 --- a/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/client/GenericClientR4Test.java +++ b/hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/client/GenericClientR4Test.java @@ -25,6 +25,7 @@ import ca.uhn.fhir.rest.param.DateRangeParam; import ca.uhn.fhir.rest.param.ParamPrefixEnum; import ca.uhn.fhir.rest.server.exceptions.NotImplementedOperationException; import ca.uhn.fhir.rest.server.exceptions.UnclassifiedServerFailureException; +import ca.uhn.fhir.util.TransactionBuilder; import ca.uhn.fhir.util.UrlUtil; import com.google.common.base.Charsets; import com.helger.commons.io.stream.StringInputStream; @@ -36,6 +37,7 @@ import org.apache.http.client.methods.HttpPut; import org.apache.http.client.methods.HttpUriRequest; import org.apache.http.message.BasicHeader; import org.apache.http.message.BasicStatusLine; +import org.hl7.fhir.instance.model.api.IBaseBundle; import org.hl7.fhir.instance.model.api.IPrimitiveType; import org.hl7.fhir.r4.model.Binary; import org.hl7.fhir.r4.model.BooleanType; @@ -2106,6 +2108,31 @@ public class GenericClientR4Test extends BaseGenericClientR4Test { } + @Test + public void testTransactionWithResponseHttp204NoContent() throws IOException { + IGenericClient client = ourCtx.newRestfulGenericClient("http://example.com/fhir"); + + ArgumentCaptor capt = ArgumentCaptor.forClass(HttpUriRequest.class); + when(myHttpClient.execute(capt.capture())).thenReturn(myHttpResponse); + when(myHttpResponse.getStatusLine()).thenReturn(new BasicStatusLine(new ProtocolVersion("HTTP", 1, 1), 204, "No Content")); + when(myHttpResponse.getEntity()).thenReturn(null); + when(myHttpResponse.getAllHeaders()).thenAnswer(new Answer() { + @Override + public Header[] answer(InvocationOnMock theInvocation) { + return new Header[0]; + } + }); + + TransactionBuilder builder = new TransactionBuilder(ourCtx); + builder.addCreateEntry(new Patient().setActive(true)); + + IBaseBundle outcome = client.transaction().withBundle(builder.getBundle()).execute(); + assertNull(outcome); + + assertEquals("http://example.com/fhir", capt.getAllValues().get(0).getURI().toASCIIString()); + + } + @Test public void testUpdateById() throws Exception { IParser p = ourCtx.newXmlParser(); From 31d736915ff67e4305ee34c31cbf076c8a8ecfad Mon Sep 17 00:00:00 2001 From: vedion Date: Sat, 22 Aug 2020 22:59:36 +0200 Subject: [PATCH 2/4] - Adding missing null guard (#2049) * - Adding missing null guard. Guard needed when reference can't be resolved. * Adding test for INCLUDE_ALL null guard fix Co-authored-by: ahn --- .../ca/uhn/fhir/jpa/dao/SearchBuilder.java | 3 ++ .../dstu3/ResourceProviderDstu3Test.java | 45 ++++++++++++++++++- 2 files changed, 46 insertions(+), 2 deletions(-) diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java index 8459ce7864a..7a002d23e0a 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/SearchBuilder.java @@ -745,6 +745,9 @@ public class SearchBuilder implements ISearchBuilder { q.setParameter("target_pids", ResourcePersistentId.toLongList(nextPartition)); List results = q.getResultList(); for (Long resourceLink : results) { + if (resourceLink == null) { + continue; + } if (theReverseMode) { pidsToInclude.add(new ResourcePersistentId(resourceLink)); } else { diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java index 3be5b845c50..986931fffda 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/provider/dstu3/ResourceProviderDstu3Test.java @@ -33,7 +33,6 @@ import ca.uhn.fhir.rest.server.exceptions.UnprocessableEntityException; import ca.uhn.fhir.rest.server.interceptor.BaseValidatingInterceptor; import ca.uhn.fhir.rest.server.interceptor.RequestValidatingInterceptor; import ca.uhn.fhir.util.HapiExtensions; -import ca.uhn.fhir.util.TestUtil; import ca.uhn.fhir.util.UrlUtil; import ca.uhn.fhir.validation.IValidatorModule; import com.google.common.base.Charsets; @@ -76,6 +75,7 @@ import org.hl7.fhir.dstu3.model.DocumentReference; import org.hl7.fhir.dstu3.model.Encounter; import org.hl7.fhir.dstu3.model.Encounter.EncounterLocationComponent; import org.hl7.fhir.dstu3.model.Encounter.EncounterStatus; +import org.hl7.fhir.dstu3.model.Enumerations; import org.hl7.fhir.dstu3.model.Enumerations.AdministrativeGender; import org.hl7.fhir.dstu3.model.Extension; import org.hl7.fhir.dstu3.model.IdType; @@ -96,6 +96,7 @@ import org.hl7.fhir.dstu3.model.Organization; import org.hl7.fhir.dstu3.model.Parameters; import org.hl7.fhir.dstu3.model.Patient; import org.hl7.fhir.dstu3.model.Period; +import org.hl7.fhir.dstu3.model.PlanDefinition; import org.hl7.fhir.dstu3.model.Practitioner; import org.hl7.fhir.dstu3.model.ProcedureRequest; import org.hl7.fhir.dstu3.model.Quantity; @@ -103,6 +104,7 @@ import org.hl7.fhir.dstu3.model.Questionnaire; import org.hl7.fhir.dstu3.model.Questionnaire.QuestionnaireItemType; import org.hl7.fhir.dstu3.model.QuestionnaireResponse; import org.hl7.fhir.dstu3.model.Reference; +import org.hl7.fhir.dstu3.model.RelatedArtifact; import org.hl7.fhir.dstu3.model.StringType; import org.hl7.fhir.dstu3.model.StructureDefinition; import org.hl7.fhir.dstu3.model.Subscription; @@ -112,7 +114,6 @@ import org.hl7.fhir.dstu3.model.UnsignedIntType; import org.hl7.fhir.dstu3.model.ValueSet; import org.hl7.fhir.instance.model.api.IBaseResource; import org.hl7.fhir.instance.model.api.IIdType; -import org.junit.jupiter.api.AfterAll; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; @@ -138,6 +139,7 @@ import java.util.HashSet; import java.util.List; import java.util.Set; import java.util.TreeSet; +import java.util.UUID; import static org.apache.commons.lang3.StringUtils.isNotBlank; import static org.hamcrest.MatcherAssert.assertThat; @@ -362,6 +364,45 @@ public class ResourceProviderDstu3Test extends BaseResourceProviderDstu3Test { ourLog.info(myFhirCtx.newJsonParser().setPrettyPrint(true).encodeResourceToString(bundle)); } + @Test + public void testSearchWithIncludeAllWithNotResolvableReference() { + // Arrange + myDaoConfig.setAllowExternalReferences(true); + + Patient patient = new Patient(); + patient.addName().setFamily(UUID.randomUUID().toString()); + IIdType createdPatientId = ourClient.create().resource(patient).execute().getId(); + + RelatedArtifact relatedArtifactInternalReference = new RelatedArtifact(); + relatedArtifactInternalReference.setDisplay(UUID.randomUUID().toString()); + relatedArtifactInternalReference.setType(RelatedArtifact.RelatedArtifactType.PREDECESSOR); + relatedArtifactInternalReference.setResource(new Reference(createdPatientId.toUnqualifiedVersionless())); + + RelatedArtifact relatedArtifactExternalReference = new RelatedArtifact(); + relatedArtifactExternalReference.setDisplay(UUID.randomUUID().toString()); + relatedArtifactExternalReference.setType(RelatedArtifact.RelatedArtifactType.PREDECESSOR); + relatedArtifactExternalReference.setResource(new Reference("http://not-local-host.dk/hapi-fhir-jpaserver/fhir/Patient/2")); + + PlanDefinition planDefinition = new PlanDefinition(); + planDefinition.setStatus(Enumerations.PublicationStatus.ACTIVE); + planDefinition.setName(UUID.randomUUID().toString()); + planDefinition.setRelatedArtifact(Arrays.asList(relatedArtifactInternalReference, relatedArtifactExternalReference)); + IIdType createdPlanDefinitionId = ourClient.create().resource(planDefinition).execute().getId(); + + // Act + Bundle returnedBundle = ourClient.search() + .forResource(PlanDefinition.class) + .include(PlanDefinition.INCLUDE_ALL) + .where(PlanDefinition.NAME.matches().value(planDefinition.getName())) + .returnBundle(Bundle.class) + .execute(); + + // Assert + assertEquals(returnedBundle.getEntry().size(), 2); + assertEquals(createdPlanDefinitionId, genResourcesOfType(returnedBundle, PlanDefinition.class).get(0).getIdElement()); + assertEquals(createdPatientId, genResourcesOfType(returnedBundle, Patient.class).get(0).getIdElement()); + } + @Test public void testBundleCreateWithTypeTransaction() throws Exception { IGenericClient client = ourClient; From 969a20d9e84ce1be5231ae1214f9a3a1c879749a Mon Sep 17 00:00:00 2001 From: jamesagnew Date: Sat, 22 Aug 2020 17:01:37 -0400 Subject: [PATCH 3/4] Credit for #2049 --- .../fhir/changelog/5_2_0/2049-add-search-null-guard.yaml | 5 +++++ 1 file changed, 5 insertions(+) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2049-add-search-null-guard.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2049-add-search-null-guard.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2049-add-search-null-guard.yaml new file mode 100644 index 00000000000..d1d0ad9afea --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2049-add-search-null-guard.yaml @@ -0,0 +1,5 @@ +--- +type: fix +issue: 2049 +title: A potential NPE in the JPA server was fixed. This error could be triggered by searching for resources with + unresolvable references. Thanks to Anders Havn for the pull request! From 437e81fc65e1f22e4b23126f5ad269c6ac3db06f Mon Sep 17 00:00:00 2001 From: James Agnew Date: Tue, 25 Aug 2020 05:55:54 -0400 Subject: [PATCH 4/4] Support multiple updates of one resource in a transaction (#2050) * Support multiple updates of one resource in a transaction * Test update * Add changelog * Test fix --- ...pdates-on-one-resource-in-transaction.yaml | 5 ++++ .../ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java | 4 +++- .../jpa/dao/r4/FhirResourceDaoR4Test.java | 24 +++++++++++++++++++ .../ResourceIndexedSearchParams.java | 22 ++++++++++------- 4 files changed, 45 insertions(+), 10 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2050-mutliple-updates-on-one-resource-in-transaction.yaml diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2050-mutliple-updates-on-one-resource-in-transaction.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2050-mutliple-updates-on-one-resource-in-transaction.yaml new file mode 100644 index 00000000000..d39f2e458c6 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_2_0/2050-mutliple-updates-on-one-resource-in-transaction.yaml @@ -0,0 +1,5 @@ +--- +type: fix +issue: 2050 +title: An issue was fixed where multiple JPA server updates to the same resource within the same + database transaction would fail with a database constraint error. diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java index 7afbb87d637..bee1aa61289 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/dao/BaseHapiFhirDao.java @@ -1057,7 +1057,7 @@ public abstract class BaseHapiFhirDao extends BaseStora entity.setLanguage(((IAnyResource) theResource).getLanguageElement().getValue()); } - newParams.setParamsOn(entity); + newParams.populateResourceTableSearchParamsPresentFlags(entity); entity.setIndexStatus(INDEX_STATUS_INDEXED); populateFullTextFields(myContext, theResource, entity); } @@ -1206,6 +1206,8 @@ public abstract class BaseHapiFhirDao extends BaseStora // Synchronize search param indexes AddRemoveCount searchParamAddRemoveCount = myDaoSearchParamSynchronizer.synchronizeSearchParamsToDatabase(newParams, entity, existingParams); + newParams.populateResourceTableParamCollections(entity); + // Interceptor broadcast: JPA_PERFTRACE_INFO if (!searchParamAddRemoveCount.isEmpty()) { if (JpaInterceptorBroadcaster.hasHooks(Pointcut.JPA_PERFTRACE_INFO, myInterceptorBroadcaster, theRequest)) { diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4Test.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4Test.java index 4f156dcda3c..b87362a9ffd 100644 --- a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4Test.java +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/dao/r4/FhirResourceDaoR4Test.java @@ -887,6 +887,30 @@ public class FhirResourceDaoR4Test extends BaseJpaR4Test { } } + @Test + public void testCreateThenUpdateInSameTransaction() { + Patient initialPatient = new Patient(); + IIdType id = myPatientDao.create(initialPatient).getId().toUnqualifiedVersionless(); + + runInTransaction(() -> { + Patient p = new Patient(); + p.setId(id); + p.setActive(true); + p.addName().setFamily("FAMILY"); + myPatientDao.update(p); + + p = new Patient(); + p.setId(id); + p.setActive(false); + p.addName().setFamily("FAMILY2"); + myPatientDao.update(p); + }); + + assertEquals(1, myPatientDao.search(SearchParameterMap.newSynchronous("name", new StringParam("family2"))).size()); + assertEquals(1, myPatientDao.search(SearchParameterMap.newSynchronous("active", new TokenParam("false"))).size()); + + } + @Test public void testCreateSummaryFails() { Patient p = new Patient(); diff --git a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/extractor/ResourceIndexedSearchParams.java b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/extractor/ResourceIndexedSearchParams.java index bae7ac93b14..7ab5352f050 100644 --- a/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/extractor/ResourceIndexedSearchParams.java +++ b/hapi-fhir-jpaserver-searchparam/src/main/java/ca/uhn/fhir/jpa/searchparam/extractor/ResourceIndexedSearchParams.java @@ -105,26 +105,30 @@ public final class ResourceIndexedSearchParams { return myLinks; } - public void setParamsOn(ResourceTable theEntity) { - theEntity.setParamsString(myStringParams); + public void populateResourceTableSearchParamsPresentFlags(ResourceTable theEntity) { theEntity.setParamsStringPopulated(myStringParams.isEmpty() == false); - theEntity.setParamsToken(myTokenParams); theEntity.setParamsTokenPopulated(myTokenParams.isEmpty() == false); - theEntity.setParamsNumber(myNumberParams); theEntity.setParamsNumberPopulated(myNumberParams.isEmpty() == false); - theEntity.setParamsQuantity(myQuantityParams); theEntity.setParamsQuantityPopulated(myQuantityParams.isEmpty() == false); - theEntity.setParamsDate(myDateParams); theEntity.setParamsDatePopulated(myDateParams.isEmpty() == false); - theEntity.setParamsUri(myUriParams); theEntity.setParamsUriPopulated(myUriParams.isEmpty() == false); - theEntity.setParamsCoords(myCoordsParams); theEntity.setParamsCoordsPopulated(myCoordsParams.isEmpty() == false); theEntity.setParamsCompositeStringUniquePresent(myCompositeStringUniques.isEmpty() == false); - theEntity.setResourceLinks(myLinks); theEntity.setHasLinks(myLinks.isEmpty() == false); } + + public void populateResourceTableParamCollections(ResourceTable theEntity) { + theEntity.setParamsString(myStringParams); + theEntity.setParamsToken(myTokenParams); + theEntity.setParamsNumber(myNumberParams); + theEntity.setParamsQuantity(myQuantityParams); + theEntity.setParamsDate(myDateParams); + theEntity.setParamsUri(myUriParams); + theEntity.setParamsCoords(myCoordsParams); + theEntity.setResourceLinks(myLinks); + } + void setUpdatedTime(Date theUpdateTime) { setUpdatedTime(myStringParams, theUpdateTime); setUpdatedTime(myNumberParams, theUpdateTime);