From 3196db96d1c485310e746833df59c0e010b5b161 Mon Sep 17 00:00:00 2001 From: James Agnew Date: Thu, 13 Jul 2017 11:48:10 -0400 Subject: [PATCH] Don't add false paging link to request --- .../fhir/rest/server/RestfulServerUtils.java | 11 +- .../BaseResourceReturningMethodBinding.java | 4 +- .../uhn/fhir/rest/server/SearchDstu3Test.java | 206 +++++++++++++++++- src/changes/changes.xml | 8 + 4 files changed, 219 insertions(+), 10 deletions(-) diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServerUtils.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServerUtils.java index 21d730d4b26..f9b5273de9e 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServerUtils.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/RestfulServerUtils.java @@ -21,6 +21,7 @@ package ca.uhn.fhir.rest.server; */ import static org.apache.commons.lang3.StringUtils.isBlank; import static org.apache.commons.lang3.StringUtils.isNotBlank; +import static org.apache.commons.lang3.StringUtils.replace; import java.io.IOException; import java.io.UnsupportedEncodingException; @@ -70,6 +71,7 @@ import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; import ca.uhn.fhir.rest.server.method.ElementsParameter; import ca.uhn.fhir.rest.server.method.SummaryEnumParameter; import ca.uhn.fhir.util.DateUtils; +import ca.uhn.fhir.util.UrlUtil; public class RestfulServerUtils { static final Pattern ACCEPT_HEADER_PATTERN = Pattern.compile("\\s*([a-zA-Z0-9+.*/-]+)\\s*(;\\s*([a-zA-Z]+)\\s*=\\s*([a-zA-Z0-9.]+)\\s*)?(,?)"); @@ -118,7 +120,7 @@ public class RestfulServerUtils { } } - public static String createPagingLink(Set theIncludes, String theServerBase, String theSearchId, int theOffset, int theCount, EncodingEnum theResponseEncoding, boolean thePrettyPrint, + public static String createPagingLink(Set theIncludes, String theServerBase, String theSearchId, int theOffset, int theCount, Map theRequestParameters, boolean thePrettyPrint, BundleTypeEnum theBundleType) { try { StringBuilder b = new StringBuilder(); @@ -136,11 +138,14 @@ public class RestfulServerUtils { b.append(Constants.PARAM_COUNT); b.append('='); b.append(theCount); - if (theResponseEncoding != null) { + String[] strings = theRequestParameters.get(Constants.PARAM_FORMAT); + if (strings != null && strings.length > 0) { b.append('&'); b.append(Constants.PARAM_FORMAT); b.append('='); - b.append(theResponseEncoding.getRequestContentType()); + String format = strings[0]; + format = replace(format, " ", "+"); + b.append(UrlUtil.escape(format)); } if (thePrettyPrint) { b.append('&'); diff --git a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java index 52c022424ca..232a66fe69a 100644 --- a/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java +++ b/hapi-fhir-server/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java @@ -349,11 +349,11 @@ public abstract class BaseResourceReturningMethodBinding extends BaseMethodBindi String linkNext = null; if (searchId != null) { if (numTotalResults == null || theOffset + numToReturn < numTotalResults) { - linkNext = (RestfulServerUtils.createPagingLink(theIncludes, serverBase, searchId, theOffset + numToReturn, numToReturn, theLinkEncoding, prettyPrint, theBundleType)); + linkNext = (RestfulServerUtils.createPagingLink(theIncludes, serverBase, searchId, theOffset + numToReturn, numToReturn, theRequest.getParameters(), prettyPrint, theBundleType)); } if (theOffset > 0) { int start = Math.max(0, theOffset - theLimit); - linkPrev = RestfulServerUtils.createPagingLink(theIncludes, serverBase, searchId, start, theLimit, theLinkEncoding, prettyPrint, theBundleType); + linkPrev = RestfulServerUtils.createPagingLink(theIncludes, serverBase, searchId, start, theLimit, theRequest.getParameters(), prettyPrint, theBundleType); } } diff --git a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/SearchDstu3Test.java b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/SearchDstu3Test.java index 59a9d82936e..23da5845f6a 100644 --- a/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/SearchDstu3Test.java +++ b/hapi-fhir-structures-dstu3/src/test/java/ca/uhn/fhir/rest/server/SearchDstu3Test.java @@ -1,16 +1,20 @@ package ca.uhn.fhir.rest.server; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.not; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNotNull; import static org.junit.Assert.assertThat; import static org.junit.Assert.fail; +import java.io.IOException; import java.nio.charset.StandardCharsets; import java.util.ArrayList; import java.util.List; import java.util.concurrent.TimeUnit; - +import ca.uhn.fhir.rest.api.Constants; import org.apache.commons.io.IOUtils; +import org.apache.http.client.ClientProtocolException; import org.apache.http.client.methods.CloseableHttpResponse; import org.apache.http.client.methods.HttpGet; import org.apache.http.impl.client.CloseableHttpClient; @@ -26,14 +30,14 @@ import org.junit.*; import ca.uhn.fhir.context.FhirContext; import ca.uhn.fhir.rest.annotation.RequiredParam; import ca.uhn.fhir.rest.annotation.Search; +import ca.uhn.fhir.rest.api.EncodingEnum; import ca.uhn.fhir.rest.api.SearchStyleEnum; import ca.uhn.fhir.rest.client.api.IGenericClient; import ca.uhn.fhir.rest.client.interceptor.LoggingInterceptor; import ca.uhn.fhir.rest.gclient.StringClientParam; import ca.uhn.fhir.rest.param.TokenAndListParam; import ca.uhn.fhir.rest.server.exceptions.InvalidRequestException; -import ca.uhn.fhir.util.PortUtil; -import ca.uhn.fhir.util.TestUtil; +import ca.uhn.fhir.util.*; public class SearchDstu3Test { @@ -80,7 +84,7 @@ public class SearchDstu3Test { ourLog.info(responseContent); assertEquals(400, status.getStatusLine().getStatusCode()); - OperationOutcome oo = (OperationOutcome) ourCtx.newXmlParser().parseResource(responseContent); + OperationOutcome oo = (OperationOutcome) ourCtx.newJsonParser().parseResource(responseContent); assertEquals( "Invalid search parameter \"identifier.chain\". Parameter contains a chain (.chain) and chains are not supported for this parameter (chaining is only allowed on reference parameters)", oo.getIssueFirstRep().getDiagnostics()); @@ -90,6 +94,191 @@ public class SearchDstu3Test { } + + @Test + public void testPagingPreservesEncodingJson() throws Exception { + HttpGet httpGet; + String linkNext; + Bundle bundle; + + // Initial search + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?identifier=foo%7Cbar&_format=json"); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.JSON); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, containsString("_format=json")); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.JSON); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, containsString("_format=json")); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.JSON); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, containsString("_format=json")); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.JSON); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, containsString("_format=json")); + + } + + @Test + public void testPagingPreservesEncodingApplicationJsonFhir() throws Exception { + HttpGet httpGet; + String linkNext; + Bundle bundle; + + // Initial search + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?identifier=foo%7Cbar&_format=" + Constants.CT_FHIR_JSON_NEW); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.JSON); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, containsString("_format=" + UrlUtil.escape(Constants.CT_FHIR_JSON_NEW))); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.JSON); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, containsString("_format=" + UrlUtil.escape(Constants.CT_FHIR_JSON_NEW))); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.JSON); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, containsString("_format=" + UrlUtil.escape(Constants.CT_FHIR_JSON_NEW))); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.JSON); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, containsString("_format=" + UrlUtil.escape(Constants.CT_FHIR_JSON_NEW))); + + } + + @Test + public void testPagingPreservesEncodingXml() throws Exception { + HttpGet httpGet; + String linkNext; + Bundle bundle; + + // Initial search + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?identifier=foo%7Cbar&_format=xml"); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.XML); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, containsString("_format=xml")); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.XML); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, containsString("_format=xml")); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.XML); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, containsString("_format=xml")); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.XML); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, containsString("_format=xml")); + + } + + @Test + public void testPagingPreservesEncodingNone() throws Exception { + HttpGet httpGet; + String linkNext; + Bundle bundle; + + // Initial search + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?identifier=foo%7Cbar"); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.JSON); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, not(containsString("_format"))); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.JSON); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, not(containsString("_format"))); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.JSON); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, not(containsString("_format"))); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.JSON); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, not(containsString("_format"))); + + } + + @Test + public void testPagingPreservesEncodingNoneWithBrowserAcceptHeader() throws Exception { + HttpGet httpGet; + String linkNext; + Bundle bundle; + + // Initial search + httpGet = new HttpGet("http://localhost:" + ourPort + "/Patient?identifier=foo%7Cbar"); + httpGet.addHeader(Constants.HEADER_ACCEPT, "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8"); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.XML); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, not(containsString("_format"))); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + httpGet.addHeader(Constants.HEADER_ACCEPT, "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8"); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.XML); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, not(containsString("_format"))); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + httpGet.addHeader(Constants.HEADER_ACCEPT, "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8"); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.XML); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, not(containsString("_format"))); + + // Fetch the next page + httpGet = new HttpGet(linkNext); + httpGet.addHeader(Constants.HEADER_ACCEPT, "text/html,application/xhtml+xml,application/xml;q=0.9,image/webp,image/apng,*/*;q=0.8"); + bundle = executeAndReturnLinkNext(httpGet, EncodingEnum.XML); + linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertThat(linkNext, not(containsString("_format"))); + + } + + private Bundle executeAndReturnLinkNext(HttpGet httpGet, EncodingEnum theExpectEncoding) throws IOException, ClientProtocolException { + CloseableHttpResponse status = ourClient.execute(httpGet); + Bundle bundle; + try { + String responseContent = IOUtils.toString(status.getEntity().getContent(), StandardCharsets.UTF_8); + ourLog.info(responseContent); + assertEquals(200, status.getStatusLine().getStatusCode()); + EncodingEnum ct = EncodingEnum.forContentType(status.getEntity().getContentType().getValue().replaceAll(";.*", "").trim()); + assertEquals(theExpectEncoding, ct); + bundle = ct.newParser(ourCtx).parseResource(Bundle.class, responseContent); + assertEquals(10, bundle.getEntry().size()); + String linkNext = bundle.getLink(Constants.LINK_NEXT).getUrl(); + assertNotNull(linkNext); + } finally { + IOUtils.closeQuietly(status.getEntity().getContent()); + } + return bundle; + } + + @Test public void testSearchWithPostAndInvalidParameters() throws Exception { IGenericClient client = ourCtx.newRestfulGenericClient("http://localhost:" + ourPort); @@ -133,6 +322,7 @@ public class SearchDstu3Test { ServletHandler proxyHandler = new ServletHandler(); RestfulServer servlet = new RestfulServer(ourCtx); + servlet.setDefaultResponseEncoding(EncodingEnum.JSON); servlet.setPagingProvider(new FifoMemoryPagingProvider(10)); servlet.setResourceProviders(patientProvider); @@ -162,7 +352,13 @@ public class SearchDstu3Test { ourLastMethod = "search"; ourIdentifiers = theIdentifiers; ArrayList retVal = new ArrayList(); - retVal.add((Patient) new Patient().addName(new HumanName().setFamily("FAMILY")).setId("1")); + + for (int i = 0; i < 200; i++) { + Patient patient = new Patient(); + patient.addName(new HumanName().setFamily("FAMILY")); + patient.getIdElement().setValue("Patient/" + i); + retVal.add((Patient) patient); + } return retVal; } diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 765e500b7dc..2d069cf0f26 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -153,6 +153,14 @@ is often the case in more recent versions of HAPI). Thanks to Sujay R for reporting! + + When the server was returning a multi-page search result where the + client did not explicitly request an encoding via the _format + parameter, a _format parameter was incorrectly added to the paging + links in the response Bundle. This would often explicitly request + XML encoding because of the browser Accept header even though + this was not what the client wanted. +