Fix UrlUtil.unescape() by not escaping "+" to " " if this is an "application/..." _outputFormat. (#4220)

* First commit:  Failing unit test and a TODO with a vague idea of where the bug happens.

* Don't escape "+" in a URL GET parameter if it starts with "application".

* Remove unnecessary TODO.

* Add changelog.

* Code review feedback on naming.  Also, make logic more robust by putting plus and should escape boolean && in parens.

hapi-fhir-base/src/main/java/ca/uhn/fhir/util/UrlUtil.java

@@ -488,9 +488,12 @@ public class UrlUtil {
 		if (theString == null) {
 			return null;
 		}
+		// If the user passes "_outputFormat" as a GET request parameter directly in the URL:
+		final boolean shouldEscapePlus = !theString.startsWith("application/");
+
 		for (int i = 0; i < theString.length(); i++) {
 			char nextChar = theString.charAt(i);
-			if (nextChar == '%' || nextChar == '+') {
+			if (nextChar == '%' || (nextChar == '+' && shouldEscapePlus)) {
 				try {
 					// Yes it would be nice to not use a string "UTF-8" but the equivalent
 					// method that takes Charset is JDK10+ only... sigh....

hapi-fhir-base/src/test/java/ca/uhn/fhir/util/UrlUtilTest.java

@@ -1,10 +1,12 @@
 package ca.uhn.fhir.util;
 
+import ca.uhn.fhir.rest.api.Constants;
 import org.apache.http.message.BasicNameValuePair;
 import org.junit.jupiter.api.Test;
 
 import static org.hamcrest.MatcherAssert.assertThat;
 import static org.hamcrest.Matchers.containsInAnyOrder;
+import static org.junit.jupiter.api.Assertions.assertAll;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertFalse;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -45,6 +47,20 @@ public class UrlUtilTest {
 		assertEquals("A%2BB", UrlUtil.escapeUrlParam("A+B"));
 	}
 
+	@Test
+	public void testUnescape() {
+		assertAll(
+			() -> assertEquals(Constants.CT_JSON, UrlUtil.unescape(Constants.CT_JSON)),
+			() -> assertEquals(Constants.CT_NDJSON, UrlUtil.unescape(Constants.CT_NDJSON)),
+			() -> assertEquals(Constants.CT_XML, UrlUtil.unescape(Constants.CT_XML)),
+			() -> assertEquals(Constants.CT_XML_PATCH, UrlUtil.unescape(Constants.CT_XML_PATCH)),
+			() -> assertEquals(Constants.CT_APPLICATION_GZIP, UrlUtil.unescape(Constants.CT_APPLICATION_GZIP)),
+			() -> assertEquals(Constants.CT_RDF_TURTLE, UrlUtil.unescape(Constants.CT_RDF_TURTLE)),
+			() -> assertEquals(Constants.CT_FHIR_JSON, UrlUtil.unescape(Constants.CT_FHIR_JSON)),
+			() -> assertEquals(Constants.CT_FHIR_NDJSON, UrlUtil.unescape(Constants.CT_FHIR_NDJSON))
+		);
+	}
+
 	@Test
 	public void testIsValid() {
 		assertTrue(UrlUtil.isValid("http://foo"));

hapi-fhir-docs/src/main/resources/ca/uhn/hapi/fhir/changelog/6_2_0/4218-bulk-export-get-output-format-param-string-escape-issue.yaml

@@ -0,0 +1,4 @@
+---
+type: fix
+issue: 4218
+title: "Performing a bulk export with an _outputParam value encoded in a GET request URL that contains a '+' (ex: 'application/fhir+ndjson') will result in a 400 because the '+' is replaced with a ' '.  After this fix the '+' will remain in the parameter value."

hapi-fhir-jpaserver-test-r4/src/test/java/ca/uhn/fhir/jpa/bulk/BulkDataExportProviderTest.java

@@ -67,6 +67,7 @@ import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.is;
 import static org.hamcrest.Matchers.notNullValue;
 import static org.hamcrest.Matchers.nullValue;
+import static org.junit.jupiter.api.Assertions.assertAll;
 import static org.junit.jupiter.api.Assertions.assertEquals;
 import static org.junit.jupiter.api.Assertions.assertNotNull;
 import static org.junit.jupiter.api.Assertions.assertTrue;
@@ -863,6 +864,52 @@ public class BulkDataExportProviderTest {
 		}
 	}
 
+	@Test
+	public void testGetBulkExport_outputFormat_FhirNdJson_inHeader() throws IOException {
+		// when
+		when(myJobRunner.startNewJob(any()))
+			.thenReturn(createJobStartResponse());
+
+		// call
+		final HttpGet httpGet = new HttpGet(String.format("http://localhost:%s/%s", myPort, JpaConstants.OPERATION_EXPORT));
+		httpGet.addHeader("_outputFormat", Constants.CT_FHIR_NDJSON);
+		httpGet.addHeader(Constants.HEADER_PREFER, Constants.HEADER_PREFER_RESPOND_ASYNC);
+
+		try (CloseableHttpResponse response = myClient.execute(httpGet)) {
+			ourLog.info("Response: {}", response.toString());
+			assertEquals(202, response.getStatusLine().getStatusCode());
+			assertEquals("Accepted", response.getStatusLine().getReasonPhrase());
+			assertEquals(String.format("http://localhost:%s/$export-poll-status?_jobId=%s", myPort, A_JOB_ID), response.getFirstHeader(Constants.HEADER_CONTENT_LOCATION).getValue());
+			assertTrue(IOUtils.toString(response.getEntity().getContent(), Charsets.UTF_8).isEmpty());
+		}
+
+		final BulkExportParameters params = verifyJobStart();
+		assertEquals(Constants.CT_FHIR_NDJSON, params.getOutputFormat());
+	}
+
+	@Test
+	public void testGetBulkExport_outputFormat_FhirNdJson_inUrl() throws IOException {
+		// when
+		when(myJobRunner.startNewJob(any()))
+			.thenReturn(createJobStartResponse());
+
+		// call
+		final HttpGet httpGet = new HttpGet(String.format("http://localhost:%s/%s?_outputFormat=%s", myPort, JpaConstants.OPERATION_EXPORT, Constants.CT_FHIR_NDJSON));
+		httpGet.addHeader(Constants.HEADER_PREFER, Constants.HEADER_PREFER_RESPOND_ASYNC);
+
+		try (CloseableHttpResponse response = myClient.execute(httpGet)) {
+			assertAll(
+				() -> assertEquals(202, response.getStatusLine().getStatusCode()),
+				() -> assertEquals("Accepted", response.getStatusLine().getReasonPhrase()),
+				() -> assertEquals(String.format("http://localhost:%s/$export-poll-status?_jobId=%s", myPort, A_JOB_ID), response.getFirstHeader(Constants.HEADER_CONTENT_LOCATION).getValue()),
+				() -> assertTrue(IOUtils.toString(response.getEntity().getContent(), Charsets.UTF_8).isEmpty())
+			);
+		}
+
+		final BulkExportParameters params = verifyJobStart();
+		assertEquals(Constants.CT_FHIR_NDJSON, params.getOutputFormat());
+	}
+
 	private void callExportAndAssertJobId(Parameters input, String theExpectedJobId) throws IOException {
 		HttpPost post;
 		post = new HttpPost("http://localhost:" + myPort + "/" + JpaConstants.OPERATION_EXPORT);