4757 externalized binary npe (#4759)

* Basic test for reading from provider

* Basic test for reading from provider

* Failing test

* Add changelog

* Update hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/BinaryStorageInterceptorR4Test.java

Co-authored-by: Ken Stevens <khstevens@gmail.com>

---------

Co-authored-by: Ken Stevens <khstevens@gmail.com>

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_6_0/4757-fix-npe-in-binary-read.yaml

@@ -0,0 +1,4 @@
+---
+type: fix
+issue: 4757
+title: "Previously, if a binary resource was read via GET, and the binary content had been externalized and not reinflated, a NullPointerException would be thrown. This has been fixed."

hapi-fhir-jaxrsserver-base/src/test/java/ca/uhn/fhir/jaxrs/server/util/JaxRsResponseDstu3Test.java

@@ -86,10 +86,10 @@ public class JaxRsResponseDstu3Test {
 		boolean theAddContentLocationHeader = false;
 		Response result = (Response) RestfulServerUtils.streamResponseAsResource(request.getServer(), binary, theSummaryMode, 200, theAddContentLocationHeader, respondGzip, this.request);
 		assertEquals(200, result.getStatus());
-		assertEquals(null, result.getHeaderString(Constants.HEADER_CONTENT_TYPE));
+		assertEquals("{\n" +
-		assertEquals(null, result.getEntity());
+			"  \"resourceType\": \"Binary\"\n" +
+			"}", result.getEntity().toString());
 	}
-	
 
 	@Test
 	public void testReturnResponse() throws IOException {

hapi-fhir-jaxrsserver-base/src/test/java/ca/uhn/fhir/jaxrs/server/util/JaxRsResponseTest.java

@@ -71,8 +71,9 @@ public class JaxRsResponseTest {
 		boolean theAddContentLocationHeader = false;
 		Response result = (Response) RestfulServerUtils.streamResponseAsResource(request.getServer(), binary, theSummaryMode, 200, theAddContentLocationHeader, respondGzip, this.request);
 		assertEquals(200, result.getStatus());
-		assertEquals(null, result.getHeaderString(Constants.HEADER_CONTENT_TYPE));
+		assertEquals("{\n" +
-		assertEquals(null, result.getEntity());
+			"  \"resourceType\": \"Binary\"\n" +
+			"}", result.getEntity().toString());
 	}
 
 	@Test

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/provider/r4/BinaryStorageInterceptorR4Test.java

@@ -6,7 +6,11 @@ import ca.uhn.fhir.jpa.api.model.DaoMethodOutcome;
 import ca.uhn.fhir.jpa.binary.api.IBinaryStorageSvc;
 import ca.uhn.fhir.jpa.binary.interceptor.BinaryStorageInterceptor;
 import ca.uhn.fhir.jpa.binstore.MemoryBinaryStorageSvcImpl;
+import ca.uhn.fhir.jpa.model.entity.StorageSettings;
 import ca.uhn.fhir.jpa.provider.BaseResourceProviderR4Test;
+import ca.uhn.fhir.rest.client.api.IClientInterceptor;
+import ca.uhn.fhir.rest.client.api.IHttpRequest;
+import ca.uhn.fhir.rest.client.api.IHttpResponse;
 import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException;
 import ca.uhn.fhir.util.HapiExtensions;
 import org.hl7.fhir.instance.model.api.IIdType;
@@ -21,6 +25,8 @@ import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 import org.springframework.beans.factory.annotation.Autowired;
 
+import java.io.IOException;
+
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.CoreMatchers.notNullValue;
 import static org.hamcrest.MatcherAssert.assertThat;
@@ -36,10 +42,15 @@ import static org.junit.jupiter.api.Assertions.fail;
 public class BinaryStorageInterceptorR4Test extends BaseResourceProviderR4Test {
 
 	public static final byte[] FEW_BYTES = {4, 3, 2, 1};
-	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 = {1, 2, 3, 4, 5, 6, 7, 8, 7, 6, 5, 4, 3, 2, 1, 8, 9, 0, 10, 9};
 	public static final byte[] SOME_BYTES_2 = {6, 7, 8, 7, 6, 5, 4, 3, 2, 1, 5, 5, 5, 6};
 	private static final Logger ourLog = LoggerFactory.getLogger(BinaryStorageInterceptorR4Test.class);
 
+	@Autowired
+	private JpaStorageSettings myStorageSettings;
+	@Autowired
+	private StorageSettings myOldStorageSettings;;
+
 	@Autowired
 	private MemoryBinaryStorageSvcImpl myStorageSvc;
 	@Autowired
@@ -164,6 +175,49 @@ public class BinaryStorageInterceptorR4Test extends BaseResourceProviderR4Test {
 
 	}
 
+
+	class ContentTypeStrippingInterceptor implements IClientInterceptor {
+
+		@Override
+		public void interceptRequest(IHttpRequest theRequest) {
+			theRequest.removeHeaders("Content-Type");
+			theRequest.removeHeaders("Accept");
+		}
+
+		@Override
+		public void interceptResponse(IHttpResponse theResponse) throws IOException {
+
+		}
+	}
+	@Test
+	public void testCreateAndRetrieveExternalizedBinaryViaGetWithNoHeaders() {
+
+		myBinaryStorageInterceptor.setAllowAutoInflateBinaries(false);
+		// Create a resource with a big enough binary
+		Binary binary = new Binary();
+		binary.setId("FOO");
+		binary.setContentType("application/octet-stream");
+		binary.setData(SOME_BYTES);
+
+		DaoMethodOutcome outcome = myBinaryDao.update(binary, mySrd);
+
+		// Make sure it was externalized
+		IIdType id = outcome.getId().toUnqualifiedVersionless();
+		String encoded = myFhirContext.newJsonParser().setPrettyPrint(true).encodeResourceToString(outcome.getResource());
+		ourLog.info("Encoded: {}", encoded);
+		assertThat(encoded, containsString(HapiExtensions.EXT_EXTERNALIZED_BINARY_ID));
+		assertThat(encoded, not(containsString("\"data\"")));
+
+		// Now read it back and make sure it is not successfully de-externalized
+		ContentTypeStrippingInterceptor interceptor = new ContentTypeStrippingInterceptor();
+		myClient.registerInterceptor(interceptor);
+		Binary execute = myClient.read().resource(Binary.class).withId(id).execute();
+		myClient.unregisterInterceptor(interceptor);
+
+		assertNull(execute.getContent());
+		assertNull(execute.getDataElement().getValue());
+		assertNotNull(execute.getDataElement().getExtensionByUrl(HapiExtensions.EXT_EXTERNALIZED_BINARY_ID).getValue());
+	}
 	@Test
 	public void testCreateAndRetrieveBinary_ClientAssignedId_ExternalizedBinary() {
 

hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServerUtils.java

@@ -1062,8 +1062,10 @@ public class RestfulServerUtils {
 
 	/**
 	 * Determines whether we should stream out Binary resource content based on the content-type. Logic is:
+	 * - If the binary was externalized and has not been reinflated upstream, return false.
 	 * - If they request octet-stream, return true;
 	 * - If the content-type happens to be a match, return true.
+	 *
 	 * - Construct an EncodingEnum out of the contentType. If this matches the responseEncoding, return true.
 	 * - Otherwise, return false.
 	 *
@@ -1073,6 +1075,9 @@ public class RestfulServerUtils {
 	 */
 	private static boolean shouldStreamContents(ResponseEncoding theResponseEncoding, IBaseBinary theBinary) {
 		String contentType = theBinary.getContentType();
+		if (theBinary.getContent() == null) {
+			return false;
+		}
 		if (theResponseEncoding == null) {
 			return true;
 		}

hapi-fhir-server/src/test/java/ca/uhn/fhir/rest/server/RestfulServerUtilsTest.java

@@ -15,8 +15,9 @@ import static ca.uhn.fhir.rest.api.RequestTypeEnum.GET;
 import static org.hamcrest.CoreMatchers.is;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsString;
-import static org.hamcrest.Matchers.equalTo;
+import static org.junit.jupiter.api.Assertions.assertEquals;
-import static org.junit.jupiter.api.Assertions.*;
+import static org.junit.jupiter.api.Assertions.assertFalse;
+import static org.junit.jupiter.api.Assertions.assertTrue;
 
 public class RestfulServerUtilsTest{
 
@@ -65,6 +66,7 @@ public class RestfulServerUtilsTest{
 		assertEquals(PreferHandlingEnum.LENIENT, header.getHanding());
 	}
 
+
 	@Test
 	public void testCreateSelfLinks() {
 		//Given