From 5ee823e8293322c98080c77393e900d9e4925327 Mon Sep 17 00:00:00 2001 From: katiesmilecdr <88786813+katiesmilecdr@users.noreply.github.com> Date: Wed, 27 Oct 2021 15:52:57 -0400 Subject: [PATCH] [3109] fix bug (#3112) * [3109] fix bug * [3109] add unit tests for BinaryAccessProvider --- ...-binary-access-write-operation-broken.yaml | 4 + .../jpa/binstore/BinaryAccessProvider.java | 33 +- .../binstore/BinaryAccessProviderTest.java | 341 ++++++++++++++++++ 3 files changed, 374 insertions(+), 4 deletions(-) create mode 100644 hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3109-binary-access-write-operation-broken.yaml create mode 100644 hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/binstore/BinaryAccessProviderTest.java diff --git a/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3109-binary-access-write-operation-broken.yaml b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3109-binary-access-write-operation-broken.yaml new file mode 100644 index 00000000000..7a6ac229e07 --- /dev/null +++ b/hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/5_6_0/3109-binary-access-write-operation-broken.yaml @@ -0,0 +1,4 @@ +--- +type: fix +issue: 3109 +title: "`$binary-access-write` operation returns a DocumentReference with no content. This has been corrected." diff --git a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/BinaryAccessProvider.java b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/BinaryAccessProvider.java index cb7e4d5a7f1..773a1ae0aea 100644 --- a/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/BinaryAccessProvider.java +++ b/hapi-fhir-jpaserver-base/src/main/java/ca/uhn/fhir/jpa/binstore/BinaryAccessProvider.java @@ -19,7 +19,6 @@ package ca.uhn.fhir.jpa.binstore; * limitations under the License. * #L% */ - import ca.uhn.fhir.context.BaseRuntimeElementDefinition; import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.jpa.api.dao.DaoRegistry; @@ -38,6 +37,7 @@ import ca.uhn.fhir.util.AttachmentUtil; import ca.uhn.fhir.util.BinaryUtil; import ca.uhn.fhir.util.DateUtils; import ca.uhn.fhir.util.HapiExtensions; +import com.google.common.annotations.VisibleForTesting; import org.apache.commons.io.IOUtils; import org.apache.commons.lang3.StringUtils; import org.apache.commons.lang3.Validate; @@ -77,6 +77,8 @@ public class BinaryAccessProvider { @Autowired(required = false) private IBinaryStorageSvc myBinaryStorageSvc; + private Boolean addTargetAttachmentIdForTest = false; + /** * $binary-access-read */ @@ -93,8 +95,13 @@ public class BinaryAccessProvider { IBaseResource resource = dao.read(theResourceId, theRequestDetails, false); IBinaryTarget target = findAttachmentForRequest(resource, path, theRequestDetails); - Optional attachmentId = target.getAttachmentId(); + + //for unit test only + if (addTargetAttachmentIdForTest){ + attachmentId = Optional.of("1"); + } + if (attachmentId.isPresent()) { @SuppressWarnings("unchecked") @@ -123,7 +130,6 @@ public class BinaryAccessProvider { theServletResponse.getOutputStream().close(); } else { - String contentType = target.getContentType(); contentType = StringUtils.defaultIfBlank(contentType, Constants.CT_OCTET_STREAM); @@ -189,7 +195,7 @@ public class BinaryAccessProvider { } if (blobId == null) { - byte[] bytes = IOUtils.toByteArray(theRequestDetails.getInputStream()); + byte[] bytes = theRequestDetails.loadRequestContents(); size = bytes.length; target.setData(bytes); } else { @@ -357,4 +363,23 @@ public class BinaryAccessProvider { } + @VisibleForTesting + public void setDaoRegistryForUnitTest(DaoRegistry theDaoRegistry) { + myDaoRegistry = theDaoRegistry; + } + + @VisibleForTesting + public void setBinaryStorageSvcForUnitTest(IBinaryStorageSvc theBinaryStorageSvc) { + myBinaryStorageSvc = theBinaryStorageSvc; + } + + @VisibleForTesting + public void setFhirContextForUnitTest(FhirContext theCtx) { + myCtx = theCtx; + } + + @VisibleForTesting + public void setTargetAttachmentIdForUnitTest(Boolean theTargetAttachmentIdForTest) { + addTargetAttachmentIdForTest = theTargetAttachmentIdForTest; + } } diff --git a/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/binstore/BinaryAccessProviderTest.java b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/binstore/BinaryAccessProviderTest.java new file mode 100644 index 00000000000..ed929083e25 --- /dev/null +++ b/hapi-fhir-jpaserver-base/src/test/java/ca/uhn/fhir/jpa/binstore/BinaryAccessProviderTest.java @@ -0,0 +1,341 @@ +package ca.uhn.fhir.jpa.binstore; + + +import ca.uhn.fhir.context.FhirContext; +import ca.uhn.fhir.interceptor.api.IInterceptorBroadcaster; +import ca.uhn.fhir.jpa.api.dao.DaoRegistry; +import ca.uhn.fhir.jpa.api.dao.IFhirResourceDao; + +import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome; +import ca.uhn.fhir.mdm.util.MessageHelper; +import ca.uhn.fhir.rest.server.RestfulServer; +import org.hl7.fhir.instance.model.api.IBaseResource; +import org.hl7.fhir.r4.model.DocumentReference; +import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; +import ca.uhn.fhir.rest.server.servlet.ServletRequestDetails; + +import org.hl7.fhir.r4.model.StringType; + +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.Spy; +import org.mockito.junit.jupiter.MockitoExtension; +import org.springframework.beans.factory.annotation.Autowired; +import javax.servlet.ServletInputStream; +import javax.servlet.ServletOutputStream; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; +import java.io.IOException; +import java.util.Date; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.anyBoolean; +import static org.mockito.ArgumentMatchers.eq; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.times; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + + +@ExtendWith(MockitoExtension.class) +public class BinaryAccessProviderTest { + public static final byte[] SOME_BYTES = {1, 2, 3, 4, 5, 6, 7, 8, 7, 6, 5, 4, 3, 2, 1}; + public static final byte[] SOME_BYTES_2 = {6, 7, 8, 7, 6, 5, 4, 3, 2, 1, 5, 5, 5, 6}; + + BinaryAccessProvider myBinaryAccessProvider; + protected FhirContext myCtx; + + @Spy + protected ServletRequestDetails myRequestDetails; + @Spy + protected HttpServletRequest theServletRequest; + @Spy + protected HttpServletResponse theServletResponse; + @Mock + private DaoRegistry myDaoRegistry; + @Spy + private IFhirResourceDao myResourceDao; + @Spy + protected IBinaryStorageSvc myBinaryStorageSvc; + @Autowired + private MessageHelper myMessageHelper; + @Autowired + private IInterceptorBroadcaster myInterceptorBroadcaster; + + + @BeforeEach + public void before() { + myBinaryAccessProvider = new BinaryAccessProvider(); + myCtx = FhirContext.forR4(); + myBinaryAccessProvider.setFhirContextForUnitTest(myCtx); + myBinaryAccessProvider.setDaoRegistryForUnitTest(myDaoRegistry); + myBinaryAccessProvider.setBinaryStorageSvcForUnitTest(myBinaryStorageSvc); + myBinaryAccessProvider.setTargetAttachmentIdForUnitTest(false); + RestfulServer server = spy(RestfulServer.class); + server.setFhirContext(myCtx); + myRequestDetails.setServer(server); + } + + @BeforeEach + public void beforeSetRequestDetails() { + myRequestDetails = new ServletRequestDetails(myInterceptorBroadcaster); + } + + @AfterEach + public void after() throws IOException { + myBinaryAccessProvider.setTargetAttachmentIdForUnitTest(false); + } + + @Test + public void testBinaryAccessRead_WithAttachmentId_BlobId() throws IOException { + DocumentReference docRef = createDocRef(); + docRef.getIdElement().setParts(null, "DocumentReference", "123", null); + + StoredDetails blobDetails = spy(StoredDetails.class); + blobDetails.setPublished(new Date()); + ServletOutputStream sos = spy(ServletOutputStream.class); + when(myDaoRegistry.getResourceDao(eq("DocumentReference"))).thenReturn(myResourceDao); + when(myResourceDao.read(any(), any(), anyBoolean())).thenReturn(docRef); + when(myBinaryStorageSvc.fetchBlobDetails(any(), any())).thenReturn(blobDetails); + when(theServletResponse.getOutputStream()).thenReturn(sos); + myBinaryAccessProvider.setTargetAttachmentIdForUnitTest(true); + + try { + myBinaryAccessProvider.binaryAccessRead(docRef.getIdElement(), new StringType("DocumentReference.content.attachment"), myRequestDetails, theServletRequest, theServletResponse); + } catch (IOException e) { + } + verify(myBinaryStorageSvc, times(1)).fetchBlobDetails(any(), any()); + verify(myBinaryStorageSvc, times(1)).writeBlob(any(), any(), any()); + verify(theServletResponse, times(1)).setStatus(200); + verify(theServletResponse, times(1)).setContentType(any()); + verify(theServletResponse, times(1)).setContentLength(0); + } + + @Test + public void testBinaryAccessRead_WithAttachmentId_UnknownBlobId() throws IOException { + DocumentReference docRef = createDocRef(); + docRef.getIdElement().setParts(null, "DocumentReference", "123", null); + + when(myDaoRegistry.getResourceDao(eq("DocumentReference"))).thenReturn(myResourceDao); + when(myResourceDao.read(any(), any(), anyBoolean())).thenReturn(docRef); + myBinaryAccessProvider.setTargetAttachmentIdForUnitTest(true); + + try { + myBinaryAccessProvider.binaryAccessRead(docRef.getIdElement(), new StringType("DocumentReference.content.attachment"), myRequestDetails, theServletRequest, theServletResponse); + } catch (InvalidRequestException | IOException e) { + assertEquals("Can not find the requested binary content. It may have been deleted.", e.getMessage()); + } + verify(myBinaryStorageSvc, times(1)).fetchBlobDetails(any(), any()); + } + + @Test + public void testBinaryAccessRead_WithoutAttachmentId() throws IOException { + DocumentReference docRef = createDocRef(); + docRef.getIdElement().setParts(null, "DocumentReference", "123", null); + + ServletOutputStream sos = spy(ServletOutputStream.class); + when(myDaoRegistry.getResourceDao(eq("DocumentReference"))).thenReturn(myResourceDao); + when(myResourceDao.read(any(), any(), anyBoolean())).thenReturn(docRef); + when(theServletResponse.getOutputStream()).thenReturn(sos); + + try { + myBinaryAccessProvider.binaryAccessRead(docRef.getIdElement(), new StringType("DocumentReference.content.attachment"), myRequestDetails, theServletRequest, theServletResponse); + } catch (IOException e) { + } + verify(theServletResponse, times(1)).setStatus(200); + verify(theServletResponse, times(1)).setContentType("application/octet-stream"); + verify(theServletResponse, times(1)).setContentLength(15); + verify(sos, times(1)).write(SOME_BYTES); + } + + @Test + public void testBinaryAccessRead_WithoutAttachmentId_NullData() throws IOException { + DocumentReference docRef = new DocumentReference(); + DocumentReference.DocumentReferenceContentComponent content = docRef.addContent(); + content.getAttachment().setContentType("application/octet-stream"); + content.getAttachment().setData(null); + docRef.getIdElement().setParts(null, "DocumentReference", "123", null); + + when(myDaoRegistry.getResourceDao(eq("DocumentReference"))).thenReturn(myResourceDao); + when(myResourceDao.read(any(), any(), anyBoolean())).thenReturn(docRef); + + try { + myBinaryAccessProvider.binaryAccessRead(docRef.getIdElement(), new StringType("DocumentReference.content.attachment"), myRequestDetails, theServletRequest, theServletResponse); + } catch (InvalidRequestException | IOException e) { + assertEquals(String.format("The resource with ID %s has no data at path: DocumentReference.content.attachment", docRef.getId()), e.getMessage()); + } + } + + @Test + public void testBinaryAccessRead_EmptyContent() { + DocumentReference docRef = new DocumentReference(); + DocumentReference.DocumentReferenceContentComponent content = docRef.addContent(); + content.getAttachment().setContentType("application/octet-stream"); + docRef.getIdElement().setParts(null, "DocumentReference", "123", null); + try { + myBinaryAccessProvider.binaryAccessRead(docRef.getIdElement(), new StringType("DocumentReference.content.attachment"), myRequestDetails, theServletRequest, theServletResponse); + } catch (InvalidRequestException | IOException e) { + assertEquals("Unknown/unsupported resource type: DocumentReference", e.getMessage()); + } + } + + @Test + public void testBinaryAccessRead_EmptyResourceType() { + DocumentReference docRef = createDocRef(); + try { + myBinaryAccessProvider.binaryAccessRead(docRef.getIdElement(), new StringType("DocumentReference.content.attachment"), myRequestDetails, theServletRequest, theServletResponse); + } catch (InvalidRequestException | IOException e) { + assertEquals("No resource type specified", e.getMessage()); + } + } + + @Test + public void testBinaryAccessRead_EmptyPath() { + DocumentReference docRef = createDocRef(); + docRef.getIdElement().setParts(null, "DocumentReference", "123", null); + try { + myBinaryAccessProvider.binaryAccessRead(docRef.getIdElement(), new StringType(""), myRequestDetails, theServletRequest, theServletResponse); + } catch (InvalidRequestException | IOException e) { + assertEquals("No path specified", e.getMessage()); + } + } + + @Test + public void testBinaryAccessRead_EmptyId() { + DocumentReference docRef = createDocRef(); + docRef.getIdElement().setParts(null, "DocumentReference", null, null); + try { + myBinaryAccessProvider.binaryAccessRead(docRef.getIdElement(), new StringType(""), myRequestDetails, theServletRequest, theServletResponse); + } catch (InvalidRequestException | IOException e) { + assertEquals("No ID specified", e.getMessage()); + } + } + + @Test + public void testBinaryAccessWrite_WithoutContentLength0() { + DocumentReference docRef = createDocRef(); + docRef.getIdElement().setParts(null, "DocumentReference", "123", null); + + DaoMethodOutcome daoOutcome = new DaoMethodOutcome(); + daoOutcome.setResource(docRef); + when(myDaoRegistry.getResourceDao(eq("DocumentReference"))).thenReturn(myResourceDao); + when(myResourceDao.read(any(), any(), anyBoolean())).thenReturn(docRef); + when(myResourceDao.update(docRef, myRequestDetails)).thenReturn(daoOutcome); + when(theServletRequest.getContentType()).thenReturn("1"); + myRequestDetails.setRequestContents(SOME_BYTES); + + try { + IBaseResource outcome = myBinaryAccessProvider.binaryAccessWrite(docRef.getIdElement(), new StringType("DocumentReference.content.attachment"), myRequestDetails, theServletRequest, theServletResponse); + assertNotNull(outcome); + assertEquals(docRef.getId(), outcome.getIdElement().getValue()); + } catch (IOException e) { + } + } + + @Test + public void testBinaryAccessWrite_WithContentLength15() throws IOException { + DocumentReference docRef = createDocRef(); + docRef.getIdElement().setParts(null, "DocumentReference", "123", null); + + DaoMethodOutcome daoOutcome = new DaoMethodOutcome(); + daoOutcome.setResource(docRef); + ServletInputStream sis = spy(ServletInputStream.class); + StoredDetails sd = spy(StoredDetails.class); + sd.setBlobId("123"); + when(myDaoRegistry.getResourceDao(eq("DocumentReference"))).thenReturn(myResourceDao); + when(myResourceDao.read(any(), any(), anyBoolean())).thenReturn(docRef); + when(myResourceDao.update(docRef, myRequestDetails)).thenReturn(daoOutcome); + when(theServletRequest.getContentType()).thenReturn("Integer"); + when(theServletRequest.getContentLength()).thenReturn(15); + when(myBinaryStorageSvc.shouldStoreBlob(15, docRef.getIdElement(), "Integer")).thenReturn(true); + when(theServletRequest.getInputStream()).thenReturn(sis); + myRequestDetails.setServletRequest(theServletRequest); + when(myBinaryStorageSvc.storeBlob(docRef.getIdElement(), null, "Integer", myRequestDetails.getInputStream())).thenReturn(sd); + myRequestDetails.setRequestContents(SOME_BYTES); + + try { + IBaseResource outcome = myBinaryAccessProvider.binaryAccessWrite(docRef.getIdElement(), new StringType("DocumentReference.content.attachment"), myRequestDetails, theServletRequest, theServletResponse); + assertNotNull(outcome); + assertEquals(docRef.getId(), outcome.getIdElement().getValue()); + } catch (IOException e) { + } + verify(myBinaryStorageSvc, times(1)).storeBlob(any(), any(), any(), any()); + } + + @Test + public void testBinaryAccessWrite_EmptyContentType() { + DocumentReference docRef = createDocRef(); + docRef.getIdElement().setParts(null, "DocumentReference", "123", null); + + when(myDaoRegistry.getResourceDao(eq("DocumentReference"))).thenReturn(myResourceDao); + when(myResourceDao.read(any(), any(), anyBoolean())).thenReturn(docRef); + when(theServletRequest.getContentType()).thenReturn(""); + + try { + myBinaryAccessProvider.binaryAccessWrite(docRef.getIdElement(), new StringType("DocumentReference.content.attachment"), myRequestDetails, theServletRequest, theServletResponse); + } catch (InvalidRequestException | IOException e) { + assertEquals("No content-target supplied", e.getMessage()); + } + } + + @Test + public void testBinaryAccessWrite_EmptyContent() { + DocumentReference docRef = new DocumentReference(); + DocumentReference.DocumentReferenceContentComponent content = docRef.addContent(); + content.getAttachment().setContentType("application/octet-stream"); + docRef.getIdElement().setParts(null, "DocumentReference", "123", null); + try { + myBinaryAccessProvider.binaryAccessWrite(docRef.getIdElement(), new StringType("DocumentReference.content.attachment"), myRequestDetails, theServletRequest, theServletResponse); + } catch (InvalidRequestException | IOException e) { + assertEquals("Unknown/unsupported resource type: DocumentReference", e.getMessage()); + } + } + + @Test + public void testBinaryAccessWrite_EmptyResourceType() { + DocumentReference docRef = createDocRef(); + try { + myBinaryAccessProvider.binaryAccessWrite(docRef.getIdElement(), new StringType("DocumentReference.content.attachment"), myRequestDetails, theServletRequest, theServletResponse); + } catch (InvalidRequestException | IOException e) { + assertEquals("No resource type specified", e.getMessage()); + } + } + + @Test + public void testBinaryAccessWrite_EmptyPath() { + DocumentReference docRef = createDocRef(); + docRef.getIdElement().setParts(null, "DocumentReference", "123", null); + try { + myBinaryAccessProvider.binaryAccessWrite(docRef.getIdElement(), new StringType(""), myRequestDetails, theServletRequest, theServletResponse); + } catch (InvalidRequestException | IOException e) { + assertEquals("No path specified", e.getMessage()); + } + } + + @Test + public void testBinaryAccessWrite_EmptyId() { + DocumentReference docRef = createDocRef(); + docRef.getIdElement().setParts(null, "DocumentReference", null, null); + try { + myBinaryAccessProvider.binaryAccessWrite(docRef.getIdElement(), new StringType(""), myRequestDetails, theServletRequest, theServletResponse); + } catch (InvalidRequestException | IOException e) { + assertEquals("No ID specified", e.getMessage()); + } + } + + private DocumentReference createDocRef() { + DocumentReference docRef = new DocumentReference(); + DocumentReference.DocumentReferenceContentComponent content = docRef.addContent(); + content.getAttachment().setContentType("application/octet-stream"); + content.getAttachment().setData(SOME_BYTES); + DocumentReference.DocumentReferenceContentComponent content2 = docRef.addContent(); + content2.getAttachment().setContentType("application/octet-stream"); + content2.getAttachment().setData(SOME_BYTES_2); + return docRef; + } +}