Skip to content

#4468 fix previous link offset no cache pagination #4489

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 5 commits into from
May 2, 2023
Merged

#4468 fix previous link offset no cache pagination #4489

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
65ec38e
#4468 Add test reproducing the issue
alparodev Feb 1, 2023
7d2e747
#4468 Fix previous link for no cache offset pagination
alparodev Feb 1, 2023
f6f3bf2
#4468 Use unchecked URI parsing
alparodev Feb 1, 2023
0d8ab2b
Merge branch 'master' into 4468-fix-previous-link-offset-no-cache-pag…
jamesagnew May 2, 2023
17559c4
Credit for #4489
jamesagnew May 2, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 6 additions & 0 deletions ...s/ca/uhn/hapi/fhir/changelog/6_8_0/4468-fix-previous-link-offset-no-cache-pagination.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
---
type: fix
issue: 4468
title: "When performing a search using a plain server without a cache, pagination links
could contain invalid offsets in the _prev_ link. Thanks to Aleksej Parovysnik for the
pull request!"
2 changes: 1 addition & 1 deletion ...rver/src/main/java/ca/uhn/fhir/rest/server/method/BaseResourceReturningMethodBinding.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -271,7 +271,7 @@ IBaseResource createBundleFromBundleProvider(IRestfulServer<?> theServer, Reques
}
}
if (offset != null && offset > 0) {
int start = Math.max(0, theOffset - pageSize);
int start = Math.max(0, offset - pageSize);
links.setPrev(RestfulServerUtils.createOffsetPagingLink(links, theRequest.getRequestPath(), theRequest.getTenantId(), start, pageSize, theRequest.getParameters()));
}
} else if (isNotBlank(theResult.getCurrentPageId())) {
Expand Down
77 changes: 75 additions & 2 deletions hapi-fhir-structures-r4/src/test/java/ca/uhn/fhir/rest/server/PagingTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,13 +25,16 @@
import org.junit.jupiter.api.Test;
import org.junit.jupiter.api.extension.RegisterExtension;

import java.net.URI;
import java.net.URISyntaxException;
import java.util.ArrayList;
import java.util.List;
import java.util.Optional;
import java.util.Set;
import java.util.concurrent.TimeUnit;

import static ca.uhn.fhir.rest.api.Constants.CHARSET_UTF8;
import static org.junit.jupiter.api.Assertions.assertDoesNotThrow;
import static org.junit.jupiter.api.Assertions.assertEquals;
import static org.junit.jupiter.api.Assertions.assertNotNull;
import static org.junit.jupiter.api.Assertions.assertNull;
Expand Down Expand Up @@ -146,6 +149,76 @@ public void testPreviousLinkLastPageWhenBundleSizeEqualsPageSizePlusOne() throws
}
}

@Test()
public void testLinksWhenUsingOffsetPaginationWithNoCaching() throws Exception {
initBundleProvider(10);
ourBundleProvider.setSize(30);
myServerExtension.getRestfulServer().registerProvider(new DummyPatientResourceProvider());

String nextLink;
String base = "http://localhost:" + myServerExtension.getPort();
HttpGet get = new HttpGet(base + "/Patient?_count=10");
String responseContent;
try (CloseableHttpResponse resp = ourClient.execute(get)) {
assertEquals(200, resp.getStatusLine().getStatusCode());
responseContent = IOUtils.toString(resp.getEntity().getContent(), Charsets.UTF_8);

Bundle bundle = ourContext.newJsonParser().parseResource(Bundle.class, responseContent);
assertEquals(10, bundle.getEntry().size());

assertNull(bundle.getLink(IBaseBundle.LINK_PREV));

String linkSelf = bundle.getLink(IBaseBundle.LINK_SELF).getUrl();
assertNotNull(linkSelf, "'self' link is not present");

nextLink = bundle.getLink(IBaseBundle.LINK_NEXT).getUrl();
assertNotNull(nextLink, "'next' link is not present");
checkParam(nextLink, Constants.PARAM_OFFSET, "10");
checkParam(nextLink, Constants.PARAM_COUNT, "10");
}
try (CloseableHttpResponse resp = ourClient.execute(new HttpGet(nextLink))) {
assertEquals(200, resp.getStatusLine().getStatusCode());
responseContent = IOUtils.toString(resp.getEntity().getContent(), Charsets.UTF_8);

Bundle bundle = ourContext.newJsonParser().parseResource(Bundle.class, responseContent);
assertEquals(10, bundle.getEntry().size());

String linkPrev = bundle.getLink(IBaseBundle.LINK_PREV).getUrl();
assertNotNull(linkPrev, "'previous' link is not present");
checkParam(linkPrev, Constants.PARAM_OFFSET, "0");
checkParam(linkPrev, Constants.PARAM_COUNT, "10");

String linkSelf = bundle.getLink(IBaseBundle.LINK_SELF).getUrl();
assertNotNull(linkSelf, "'self' link is not present");
checkParam(linkSelf, Constants.PARAM_OFFSET, "10");
checkParam(linkSelf, Constants.PARAM_COUNT, "10");

nextLink = bundle.getLink(IBaseBundle.LINK_NEXT).getUrl();
assertNotNull(nextLink, "'next' link is not present");
checkParam(nextLink, Constants.PARAM_OFFSET, "20");
checkParam(nextLink, Constants.PARAM_COUNT, "10");
}
try (CloseableHttpResponse resp = ourClient.execute(new HttpGet(nextLink))) {
assertEquals(200, resp.getStatusLine().getStatusCode());
responseContent = IOUtils.toString(resp.getEntity().getContent(), Charsets.UTF_8);

Bundle bundle = ourContext.newJsonParser().parseResource(Bundle.class, responseContent);
assertEquals(10, bundle.getEntry().size());

String linkPrev = bundle.getLink(IBaseBundle.LINK_PREV).getUrl();
assertNotNull(linkPrev, "'previous' link is not present");
checkParam(linkPrev, Constants.PARAM_OFFSET, "10");
checkParam(linkPrev, Constants.PARAM_COUNT, "10");

String linkSelf = bundle.getLink(IBaseBundle.LINK_SELF).getUrl();
assertNotNull(linkSelf, "'self' link is not present");
checkParam(linkSelf, Constants.PARAM_OFFSET, "20");
checkParam(linkSelf, Constants.PARAM_COUNT, "10");

assertNull(bundle.getLink(IBaseBundle.LINK_NEXT));
}
}

@Test()
public void testSendingSameRequestConsecutivelyResultsInSameResponse() throws Exception {
initBundleProvider(10);
Expand Down Expand Up @@ -177,8 +250,8 @@ public void testSendingSameRequestConsecutivelyResultsInSameResponse() throws Ex
assertEquals(0, bundle.getEntry().size());
}
}
private void checkParam(String theUri, String theCheckedParam, String theExpectedValue) {
Optional<String> paramValue = URLEncodedUtils.parse(theUri, CHARSET_UTF8).stream()
private void checkParam(String theUriString, String theCheckedParam, String theExpectedValue) {
Optional<String> paramValue = URLEncodedUtils.parse(URI.create(theUriString), CHARSET_UTF8).stream()
.filter(nameValuePair -> nameValuePair.getName().equals(theCheckedParam))
.map(NameValuePair::getValue)
.findAny();
Expand Down
5 changes: 5 additions & 0 deletions pom.xml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -868,6 +868,11 @@
<name>zachdoctolib</name>
<organization>Doctolib</organization>
</developer>
<developer>
<id>alparodev</id>
<name>Aleksej Parovysnik</name>
<organization>Doctolib</organization>
</developer>
</developers>

<licenses>
Expand Down