3929 batch2 jobs file fetching step strict policy for response header content type (#3945)

* Adding failing tests cases.

* What was done:
-Accepting text/plain and application/json as import response content type.
-Added change log.

* Fixed NDJSON EncodingEnum

* Added content type application/json+fhir to the test

* Fixed code

* Update hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/3929-batch2-jobs-file-fetching-step-strict-policy-for-response-header-content-type.yaml

Co-authored-by: Tadgh <garygrantgraham@gmail.com>

* Update hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/3929-batch2-jobs-file-fetching-step-strict-policy-for-response-header-content-type.yaml

Co-authored-by: Tadgh <garygrantgraham@gmail.com>

* Update hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/3929-batch2-jobs-file-fetching-step-strict-policy-for-response-header-content-type.yaml

Co-authored-by: Tadgh <garygrantgraham@gmail.com>

* Changed message

* Fixed code

Co-authored-by: peartree <etienne.poirier@smilecdr.com>
Co-authored-by: Tadgh <garygrantgraham@gmail.com>

hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/EncodingEnum.java

@@ -53,7 +53,7 @@ public enum EncodingEnum {
 		}
 	},
 
-        NDJSON(Constants.CT_FHIR_NDJSON, Constants.CT_FHIR_NDJSON, Constants.FORMAT_NDJSON) {
+	NDJSON(Constants.CT_FHIR_NDJSON, Constants.CT_FHIR_NDJSON, Constants.FORMAT_NDJSON) {
 		@Override
 		public IParser newParser(FhirContext theContext) {
 			return theContext.newNDJsonParser();

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/3929-batch2-jobs-file-fetching-step-strict-policy-for-response-header-content-type.yaml

@@ -0,0 +1,6 @@
+---
+type: fix
+issue: 3929
+title: "During bulk import, fetching resource files from a client server would fail if the server response specified a
+ `content-type` that was not `application/fhir+json`. Validation has been loosened to accept content-type values of
+ `text/plain` and `application/json`."

hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/imprt/BulkImportFileServlet.java

@@ -56,6 +56,8 @@ public class BulkImportFileServlet extends HttpServlet {
 	private static final Logger ourLog = LoggerFactory.getLogger(BulkImportFileServlet.class);
 	private final Map<String, IFileSupplier> myFileIds = new HashMap<>();
 
+	public static final String DEFAULT_HEADER_CONTENT_TYPE = CT_FHIR_NDJSON + CHARSET_UTF8_CTSUFFIX;
+
 	@Override
 	protected void doGet(HttpServletRequest theRequest, HttpServletResponse theResponse) throws ServletException, IOException {
 		try {
@@ -95,7 +97,7 @@ public class BulkImportFileServlet extends HttpServlet {
 
 		ourLog.info("Serving Bulk Import NDJSON file index: {}", indexParam);
 
-		theResponse.addHeader(Constants.HEADER_CONTENT_TYPE, CT_FHIR_NDJSON + CHARSET_UTF8_CTSUFFIX);
+		theResponse.addHeader(Constants.HEADER_CONTENT_TYPE, getHeaderContentType());
 
 		IFileSupplier supplier = myFileIds.get(indexParam);
 		if (supplier.isGzip()) {
@@ -113,6 +115,10 @@ public class BulkImportFileServlet extends HttpServlet {
 
 	}
 
+	public String getHeaderContentType(){
+		return DEFAULT_HEADER_CONTENT_TYPE;
+	}
+
 	public void clearFiles() {
 		myFileIds.clear();
 	}

hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/imprt/FetchFilesStep.java

@@ -27,6 +27,7 @@ import ca.uhn.fhir.batch2.api.RunOutcome;
 import ca.uhn.fhir.batch2.api.StepExecutionDetails;
 import ca.uhn.fhir.batch2.api.VoidModel;
 import ca.uhn.fhir.i18n.Msg;
+import ca.uhn.fhir.rest.api.Constants;
 import ca.uhn.fhir.rest.api.EncodingEnum;
 import ca.uhn.fhir.rest.client.impl.HttpBasicAuthInterceptor;
 import ca.uhn.fhir.rest.server.exceptions.InternalErrorException;
@@ -47,12 +48,15 @@ import java.io.IOException;
 import java.io.InputStream;
 import java.io.InputStreamReader;
 import java.nio.charset.StandardCharsets;
+import java.util.Arrays;
 import java.util.List;
 
 import static org.apache.commons.lang3.StringUtils.isNotBlank;
 
 public class FetchFilesStep implements IFirstJobStepWorker<BulkImportJobParameters, NdJsonFileJson> {
 	private static final Logger ourLog = LoggerFactory.getLogger(FetchFilesStep.class);
+	private static final List<String> ourValidContentTypes = Arrays.asList(Constants.CT_APP_NDJSON,  Constants.CT_FHIR_NDJSON, Constants.CT_FHIR_JSON, Constants.CT_FHIR_JSON_NEW, Constants.CT_JSON, Constants.CT_TEXT);
+	private static final List<String> ourValidNonNdJsonContentTypes = Arrays.asList(Constants.CT_FHIR_JSON, Constants.CT_FHIR_JSON_NEW, Constants.CT_JSON, Constants.CT_TEXT);
 
 	@Nonnull
 	@Override
@@ -82,8 +86,10 @@ public class FetchFilesStep implements IFirstJobStepWorker<BulkImportJobParamete
 					}
 
 					String contentType = response.getEntity().getContentType().getValue();
-					EncodingEnum encoding = EncodingEnum.forContentType(contentType);
+					Validate.isTrue(hasMatchingSubstring(contentType, ourValidContentTypes), "Received content type \"%s\" from URL: %s. This format is not one of the supported content type: %s", contentType, nextUrl, getContentTypesString());
-					Validate.isTrue(encoding == EncodingEnum.NDJSON, "Received non-NDJSON content type \"%s\" from URL: %s", contentType, nextUrl);
+					if (hasMatchingSubstring(contentType, ourValidNonNdJsonContentTypes)) {
+						ourLog.info("Received non-NDJSON content type \"{}\" from URL: {}. It will be processed but it may not complete correctly if the actual data is not NDJSON.", contentType, nextUrl);
+					}
 
 					try (InputStream inputStream = response.getEntity().getContent()) {
 						try (LineIterator lineIterator = new LineIterator(new InputStreamReader(inputStream, StandardCharsets.UTF_8))) {
@@ -150,4 +156,12 @@ public class FetchFilesStep implements IFirstJobStepWorker<BulkImportJobParamete
 		return builder.build();
 	}
 
+	private static boolean hasMatchingSubstring(String str, List<String> substrings) {
+		return substrings.stream().anyMatch(str::contains);
+	}
+
+	private static String getContentTypesString() {
+		return String.join(", ", ourValidContentTypes);
+	}
+
 }

hapi-fhir-storage-batch2-jobs/src/test/java/ca/uhn/fhir/batch2/jobs/imprt/BulkImportFileServletTest.java

@@ -10,17 +10,17 @@ import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.RegisterExtension;
 
 import java.io.IOException;
-import java.io.StringReader;
 import java.nio.charset.StandardCharsets;
 
-import static org.hamcrest.MatcherAssert.assertThat;
-import static org.hamcrest.Matchers.containsString;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 
 public class BulkImportFileServletTest {
 
 	private BulkImportFileServlet mySvc = new BulkImportFileServlet();
 
+	static final String ourInput = "{\"resourceType\":\"Patient\", \"id\": \"A\", \"active\": true}\n" +
+		"{\"resourceType\":\"Patient\", \"id\": \"B\", \"active\": false}";
+
 	@RegisterExtension
 	private HttpServletExtension myServletExtension = new HttpServletExtension()
 		.withServlet(mySvc)
@@ -34,21 +34,30 @@ public class BulkImportFileServletTest {
 
 	@Test
 	public void testDownloadFile() throws IOException {
-		String input = "{\"resourceType\":\"Patient\", \"id\": \"A\", \"active\": true}\n" +
-			"{\"resourceType\":\"Patient\", \"id\": \"B\", \"active\": false}";
-		String index = mySvc.registerFileByContents(input);
 
-		CloseableHttpClient client = myServletExtension.getHttpClient();
+		String index = mySvc.registerFileByContents(ourInput);
 
 		String url = myServletExtension.getBaseUrl() + "/download?index=" + index;
-		try (CloseableHttpResponse response = client.execute(new HttpGet(url))) {
-			assertEquals(200, response.getStatusLine().getStatusCode());
 
+		executeBulkImportAndCheckReturnedContentType(url);
+
+	}
+
+
+	private void executeBulkImportAndCheckReturnedContentType(String theUrl)  throws IOException{
+		CloseableHttpClient client = myServletExtension.getHttpClient();
+
+		try (CloseableHttpResponse response = client.execute(new HttpGet(theUrl))) {
 			String responseBody = IOUtils.toString(response.getEntity().getContent(), StandardCharsets.UTF_8);
-			assertEquals(input, responseBody);
+			String responseHeaderContentType = response.getFirstHeader("content-type").getValue();
+
+			assertEquals(200, response.getStatusLine().getStatusCode());
+			assertEquals(BulkImportFileServlet.DEFAULT_HEADER_CONTENT_TYPE, responseHeaderContentType);
+			assertEquals(ourInput, responseBody);
 		}
 	}
 
+
 	@Test
 	public void testInvalidRequests() throws IOException {
 		CloseableHttpClient client = myServletExtension.getHttpClient();

hapi-fhir-storage-batch2-jobs/src/test/java/ca/uhn/fhir/batch2/jobs/imprt/FetchFilesStepTest.java

@@ -9,12 +9,21 @@ import ca.uhn.fhir.test.utilities.server.HttpServletExtension;
 import org.junit.jupiter.api.Test;
 import org.junit.jupiter.api.extension.ExtendWith;
 import org.junit.jupiter.api.extension.RegisterExtension;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 import org.mockito.Mock;
 import org.mockito.junit.jupiter.MockitoExtension;
 import org.springframework.util.Base64Utils;
 
 import java.nio.charset.StandardCharsets;
+import java.util.Objects;
 
+import static ca.uhn.fhir.rest.api.Constants.CT_APP_NDJSON;
+import static ca.uhn.fhir.rest.api.Constants.CT_FHIR_JSON;
+import static ca.uhn.fhir.rest.api.Constants.CT_FHIR_JSON_NEW;
+import static ca.uhn.fhir.rest.api.Constants.CT_FHIR_NDJSON;
+import static ca.uhn.fhir.rest.api.Constants.CT_JSON;
+import static ca.uhn.fhir.rest.api.Constants.CT_TEXT;
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.hasItem;
 import static org.junit.jupiter.api.Assertions.assertEquals;
@@ -30,7 +39,7 @@ public class FetchFilesStepTest {
 	public static final JobInstance ourTestInstance = JobInstance.fromInstanceId(INSTANCE_ID);
 	public static final String CHUNK_ID = "chunk-id";
 
-	private final BulkImportFileServlet myBulkImportFileServlet = new BulkImportFileServlet();
+	private final ContentTypeHeaderModifiableBulkImportFileServlet myBulkImportFileServlet = new ContentTypeHeaderModifiableBulkImportFileServlet();
 	@RegisterExtension
 	private final HttpServletExtension myHttpServletExtension = new HttpServletExtension()
 		.withServlet(myBulkImportFileServlet);
@@ -39,11 +48,12 @@ public class FetchFilesStepTest {
 	@Mock
 	private IJobDataSink<NdJsonFileJson> myJobDataSink;
 
-	@Test
+	@ParameterizedTest
-	public void testFetchWithBasicAuth() {
+	@ValueSource(strings = {CT_FHIR_NDJSON, CT_FHIR_JSON, CT_FHIR_JSON_NEW, CT_APP_NDJSON, CT_JSON, CT_TEXT})
+	public void testFetchWithBasicAuth(String theHeaderContentType) {
 
 		// Setup
-
+		myBulkImportFileServlet.setHeaderContentTypeValue(theHeaderContentType);
 		String index = myBulkImportFileServlet.registerFileByContents("{\"resourceType\":\"Patient\"}");
 
 		BulkImportJobParameters parameters = new BulkImportJobParameters()
@@ -106,4 +116,20 @@ public class FetchFilesStepTest {
 
 		assertThrows(JobExecutionFailedException.class, () -> mySvc.run(details, myJobDataSink));
 	}
+
+	public static class ContentTypeHeaderModifiableBulkImportFileServlet extends BulkImportFileServlet{
+
+		public String myContentTypeValue;
+
+
+		public void setHeaderContentTypeValue(String theContentTypeValue) {
+			myContentTypeValue = theContentTypeValue;
+		}
+
+		@Override
+		public String getHeaderContentType() {
+			return Objects.nonNull(myContentTypeValue) ? myContentTypeValue : super.getHeaderContentType();
+		}
+	}
+
 }