From 3349ad8ca969e5cf54c555209da5cabe9cce75f4 Mon Sep 17 00:00:00 2001 From: MykolaMedynskyiSCDR <110495727+MykolaMedynskyiSCDR@users.noreply.github.com> Date: Wed, 21 Sep 2022 11:29:32 -0400 Subject: [PATCH] 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 * 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 * 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 * Changed message * Fixed code Co-authored-by: peartree Co-authored-by: Tadgh --- .../ca/uhn/fhir/rest/api/EncodingEnum.java | 2 +- ...licy-for-response-header-content-type.yaml | 6 ++++ .../jobs/imprt/BulkImportFileServlet.java | 8 ++++- .../batch2/jobs/imprt/FetchFilesStep.java | 18 ++++++++-- .../jobs/imprt/BulkImportFileServletTest.java | 29 ++++++++++------ .../batch2/jobs/imprt/FetchFilesStepTest.java | 34 ++++++++++++++++--- 6 files changed, 79 insertions(+), 18 deletions(-) create mode 100644 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 diff --git a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/EncodingEnum.java b/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/EncodingEnum.java index 7f07a251d91..41b5dd5ebb7 100644 --- a/hapi-fhir-base/src/main/java/ca/uhn/fhir/rest/api/EncodingEnum.java +++ b/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(); diff --git a/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 b/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 new file mode 100644 index 00000000000..0fcbab7e423 --- /dev/null +++ b/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`." diff --git a/hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/imprt/BulkImportFileServlet.java b/hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/imprt/BulkImportFileServlet.java index af1c3517280..675a00fd483 100644 --- a/hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/imprt/BulkImportFileServlet.java +++ b/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 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(); } diff --git a/hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/imprt/FetchFilesStep.java b/hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/imprt/FetchFilesStep.java index 6d3cd049730..2c0138e69f6 100644 --- a/hapi-fhir-storage-batch2-jobs/src/main/java/ca/uhn/fhir/batch2/jobs/imprt/FetchFilesStep.java +++ b/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 { private static final Logger ourLog = LoggerFactory.getLogger(FetchFilesStep.class); + private static final List 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 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 substrings) { + return substrings.stream().anyMatch(str::contains); + } + + private static String getContentTypesString() { + return String.join(", ", ourValidContentTypes); + } + } diff --git a/hapi-fhir-storage-batch2-jobs/src/test/java/ca/uhn/fhir/batch2/jobs/imprt/BulkImportFileServletTest.java b/hapi-fhir-storage-batch2-jobs/src/test/java/ca/uhn/fhir/batch2/jobs/imprt/BulkImportFileServletTest.java index 400e70db2b7..df8844f2d75 100644 --- a/hapi-fhir-storage-batch2-jobs/src/test/java/ca/uhn/fhir/batch2/jobs/imprt/BulkImportFileServletTest.java +++ b/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(); diff --git a/hapi-fhir-storage-batch2-jobs/src/test/java/ca/uhn/fhir/batch2/jobs/imprt/FetchFilesStepTest.java b/hapi-fhir-storage-batch2-jobs/src/test/java/ca/uhn/fhir/batch2/jobs/imprt/FetchFilesStepTest.java index 6375700bdd0..df9931051d7 100644 --- a/hapi-fhir-storage-batch2-jobs/src/test/java/ca/uhn/fhir/batch2/jobs/imprt/FetchFilesStepTest.java +++ b/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 myJobDataSink; - @Test - public void testFetchWithBasicAuth() { + @ParameterizedTest + @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(); + } + } + }