Skip to content

Commit

Permalink
Add first and last pagination parameters (#502)
Browse files Browse the repository at this point in the history
  • Loading branch information
mpardesh authored and archolewa committed Aug 25, 2017
1 parent 86548ca commit 118a4ff
Show file tree
Hide file tree
Showing 9 changed files with 74 additions and 46 deletions.
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,6 @@ private Pagination<DimensionRow> getResultsPage(Query query, PaginationParameter
throws PageNotFoundException {
int perPage = paginationParameters.getPerPage();
validatePerPage(perPage);
int requestedPageNumber = paginationParameters.getPage();

TreeSet<DimensionRow> filteredDimRows;
int documentCount;
Expand All @@ -578,11 +577,11 @@ private Pagination<DimensionRow> getResultsPage(Query query, PaginationParameter
luceneIndexSearcher,
null,
query,
perPage,
requestedPageNumber
perPage
);
hits = hitDocs.scoreDocs;
documentCount = hitDocs.totalHits;
int requestedPageNumber = paginationParameters.getPage(documentCount);
if (hits.length == 0) {
if (requestedPageNumber == 1) {
return new SinglePagePagination<>(Collections.emptyList(), paginationParameters, 0);
Expand All @@ -592,7 +591,7 @@ private Pagination<DimensionRow> getResultsPage(Query query, PaginationParameter
}
for (int currentPage = 1; currentPage < requestedPageNumber; currentPage++) {
ScoreDoc lastEntry = hits[hits.length - 1];
hits = getPageOfData(luceneIndexSearcher, lastEntry, query, perPage, requestedPageNumber).scoreDocs;
hits = getPageOfData(luceneIndexSearcher, lastEntry, query, perPage).scoreDocs;
if (hits.length == 0) {
throw new PageNotFoundException(requestedPageNumber, perPage, 0);
}
Expand Down Expand Up @@ -652,23 +651,21 @@ private void validatePerPage(int perPage) {
* search after this entry (if lastEntry is null, the indexSearcher will begin its search from the beginning)
* @param query The Lucene query used to locate the desired dimension metadata
* @param perPage The number of entries per page
* @param currentPage The desired page number
*
* @return The desired page of dimension metadata
*/
private TopDocs getPageOfData(
IndexSearcher indexSearcher,
ScoreDoc lastEntry,
Query query,
int perPage,
int currentPage
int perPage
) {
TimeLimitingCollectorManager manager = new TimeLimitingCollectorManager(searchTimeout, lastEntry, perPage);
lock.readLock().lock();
try {
return indexSearcher.search(query, manager);
} catch (IOException e) {
String errorMessage = "Unable to find dimension rows for page " + currentPage;
String errorMessage = "Unable to find dimension rows for specified page.";
LOG.error(errorMessage);
throw new RuntimeException(errorMessage);
} catch (TimeLimitingCollector.TimeExceededException e) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -332,7 +332,11 @@ public Pagination<DimensionRow> findFilteredDimensionRowsPaged(
) {
TreeSet<DimensionRow> filteredDimensionRows = applyFilters(getAllOrderedDimensionRows(), filters);
return new SinglePagePagination<>(
doPagination(filteredDimensionRows, paginationParameters.getPage(), paginationParameters.getPerPage())
doPagination(
filteredDimensionRows,
paginationParameters.getPage(filteredDimensionRows.size()),
paginationParameters.getPerPage()
)
.stream()
.collect(Collectors.toList()),
paginationParameters,
Expand All @@ -349,9 +353,10 @@ public Pagination<DimensionRow> findFilteredDimensionRowsPaged(
* @return All dimension rows that belongs to a requested page
*/
private TreeSet<DimensionRow> getAllDimensionRowsPaged(PaginationParameters paginationParameters) {
TreeSet<DimensionRow> allRows = getAllOrderedDimensionRows();
return doPagination(
getAllOrderedDimensionRows(),
paginationParameters.getPage(),
allRows,
paginationParameters.getPage(allRows.size()),
paginationParameters.getPerPage()
);
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ public class AllPagesPagination<T> implements Pagination<T> {
public AllPagesPagination(Collection<T> entireCollection, PaginationParameters paginationParameters)
throws PageNotFoundException {
this.collectionSize = entireCollection.size();
this.pageToFetch = paginationParameters.getPage();
this.pageToFetch = paginationParameters.getPage(entireCollection.size());
this.countPerPage = paginationParameters.getPerPage();
this.lastPage = (collectionSize > countPerPage) ? (collectionSize - 1) / countPerPage + 1 : 1;

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -28,7 +28,7 @@ public class SinglePagePagination<T> implements Pagination<T> {
* @param totalMatch The total number of results found. The single page collection is part of these results
*/
public SinglePagePagination(List<T> entirePage, PaginationParameters paginationParameters, int totalMatch) {
this.pageToFetch = paginationParameters.getPage();
this.pageToFetch = paginationParameters.getPage(entirePage.size());
this.countPerPage = paginationParameters.getPerPage();
this.totalMatch = totalMatch;
this.lastPage = (totalMatch > countPerPage) ? (totalMatch - 1) / countPerPage + 1 : 1;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -21,6 +21,9 @@ public class PaginationParameters {
private static final SystemConfig SYSTEM_CONFIG = SystemConfigProvider.getInstance();
private static final Logger LOG = LoggerFactory.getLogger(PaginationParameters.class);
private static final int MINIMAL_VALUE = 1;
private static final int LAST_PAGE = -1;
private static final String FIRST = "first";
private static final String LAST = "last";

private static final int DEFAULT_MAX_RESULTS_WITHOUT_FILTERS = 10000;
private static final int MAX_RESULTS_WITHOUT_FILTER = SYSTEM_CONFIG.getIntProperty(
Expand Down Expand Up @@ -53,25 +56,18 @@ public class PaginationParameters {
* @throws BadPaginationException If at least one of 'perPage' or 'page' is not a positive integer.
*/
public PaginationParameters(String perPage, String page) throws BadPaginationException {
this.perPage = parseParameter(perPage, "perPage");
this.page = parseParameter(page, "page");
validate(this.perPage, "perPage");
validate(this.page, "page");
this(parseParameter(perPage, "perPage"), parseParameter(page, "page"));
}

/**
* Constructor for already-parsed pagination parameters.
*
* @param perPage The number of rows to be displayed on each page.
* @param page The page to be displayed
*
* @throws BadPaginationException If at least one of 'perPage' or 'page' is not positive.
*/
public PaginationParameters(int perPage, int page) throws BadPaginationException {
public PaginationParameters(int perPage, int page) {
this.perPage = perPage;
this.page = page;
validate(this.perPage, "perPage");
validate(this.page, "page");
}

/**
Expand All @@ -83,14 +79,23 @@ public PaginationParameters(int perPage, int page) throws BadPaginationException
* @return the parsed integer
* @throws BadPaginationException If 'parameter' cannot be parsed as an integer.
*/
private int parseParameter (String parameter, String parameterName) throws BadPaginationException {
private static int parseParameter (String parameter, String parameterName) throws BadPaginationException {
if (parameter.equals("")) {
ErrorMessageFormat errorMessage = ErrorMessageFormat.PAGINATION_PARAMETER_MISSING;
LOG.debug(errorMessage.logFormat(parameterName));
throw new BadPaginationException(errorMessage.format(parameterName));
}
try {
return Integer.parseInt(parameter);
if (parameterName.equals("page")) {
if (parameter.equals(FIRST)) {
return 1;
} else if (parameter.equals(LAST)) {
return LAST_PAGE;
}
}
int parsedParameter = Integer.parseInt(parameter);
validate(parsedParameter, parameterName);
return parsedParameter;
} catch (NumberFormatException ignored) {
ErrorMessageFormat errorMessage = ErrorMessageFormat.PAGINATION_PARAMETER_INVALID;
LOG.debug(errorMessage.logFormat(parameterName, parameter));
Expand All @@ -106,7 +111,7 @@ private int parseParameter (String parameter, String parameterName) throws BadPa
*
* @throws BadPaginationException if 'parameter' is not greater than 0.
*/
private void validate(int parameter, String parameterName) throws BadPaginationException {
private static void validate(int parameter, String parameterName) throws BadPaginationException {
if (parameter < MINIMAL_VALUE) {
ErrorMessageFormat errorMessage = ErrorMessageFormat.PAGINATION_PARAMETER_INVALID;
LOG.debug(errorMessage.logFormat(parameterName, parameter));
Expand All @@ -118,7 +123,17 @@ public int getPerPage() {
return perPage;
}

public int getPage() {
/**
* Returns the requested page number.
*
* @param resultSize The number of rows in the result set
*
* @return The request page number
*/
public int getPage(int resultSize) {
if (page == -1) {
return resultSize == 0 ? 1 : (int) Math.ceil(((double) resultSize) / perPage);
}
return page;
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -514,7 +514,7 @@ abstract class SearchProviderSpec<T extends SearchProvider> extends Specificatio
buildFilter("animal|desc-notin[this is an owl]")
]
and: "We get the second page, where each page has two rows (so the last page has to have only one result)"
PaginationParameters parameters = new PaginationParameters(2, 2)
PaginationParameters parameters = new PaginationParameters("2", "last")

when: "We query the search provider"
Pagination<DimensionRow> resultsPage = searchProvider.findFilteredDimensionRowsPaged(filters, parameters)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -144,7 +144,7 @@ class DataPaginationSpec extends BaseDataServletComponentSpec {


@Unroll
def "Page #page of #numPages with the correct headers and #rowsPerPage rows per page of data, is returned"() {
def "Page #pageParam of #numPages with the correct headers and #rowsPerPage rows per page of data, is returned"() {
given: "A known Druid response, and the expected API response"
String druidResponse = getFakeDruidResponse(numPages * rowsPerPage)
validateJson(druidResponse)
Expand All @@ -154,21 +154,23 @@ class DataPaginationSpec extends BaseDataServletComponentSpec {
injectDruidResponse(druidResponse)

when: "We send a request"
Response response = makeAbstractRequest {getQueryParams("$rowsPerPage", "$page")}
Response response = makeAbstractRequest {getQueryParams("$rowsPerPage", pageParam)}

then:
headersAreCorrect(response.getHeaders(), ROWS_PER_PAGE, page, numPages, true)
GroovyTestUtils.compareJson(response.readEntity(String), apiResponse)

where:
rowsPerPage = ROWS_PER_PAGE
numPages | page
1 | 1
2 | 1
2 | 2
3 | 1
3 | 2
3 | 3
numPages | page | pageParam
1 | 1 | "1"
2 | 1 | "1"
2 | 2 | "2"
3 | 1 | "1"
3 | 2 | "2"
3 | 3 | "3"
3 | 1 | "first"
3 | 3 | "last"
}

@Unroll
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -349,9 +349,11 @@ class DimensionsServletComponentSpec extends Specification {
GroovyTestUtils.compareJson(result, expectedResponse)

where:
paginating | page | perPage | explicitlyNotExplicitly
true | "1" | "5" | "explicitly"
false | "" | "" | "not explicitly"
paginating | page | perPage | explicitlyNotExplicitly
true | "1" | "5" | "explicitly"
false | "" | "" | "not explicitly"
true | "first" | "5" | "explicitly"
true | "last" | "5" | "explicitly"
}

def "test dimension value endpoint filter-chain"() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,19 @@ class PaginationParametersSpec extends Specification {

then: "The handler's perPage and page are as expected"
handler.getPerPage() == expectedPerPage
handler.getPage() == expectedPage
handler.getPage(resultSize) == expectedPage

where:
perPage | page || expectedPerPage || expectedPage
"1" | "2" || 1 || 2
"2" | "1" || 2 || 1
perPage | page || expectedPerPage || expectedPage || resultSize
"1" | "2" || 1 || 2 || 10
"2" | "1" || 2 || 1 || 10
"3" | "last" || 3 || 4 || 10
"5" | "last" || 5 || 2 || 10
"2" | "first" || 2 || 1 || 10
"3" | "1" || 3 || 1 || 3
"3" | "first" || 3 || 1 || 3
"3" | "last" || 3 || 1 || 3
"3" | "last" || 3 || 1 || 0
}

@Unroll
Expand All @@ -38,11 +45,11 @@ class PaginationParametersSpec extends Specification {

where:
perPage | page || expectedError
"-1" | "-1" || errorMessage("perPage", "-1")
"-1" | "-1" || errorMessage("perPage", "-1")
"0" | "-1" || errorMessage("perPage", "0")
"1" | "-1" || errorMessage("page", "-1")
"-1" | "0" || errorMessage("perPage", "-1")
"-1" | "1" || errorMessage("perPage", "-1")
"-1" | "0" || errorMessage("perPage", "-1")
"-1" | "1" || errorMessage("perPage", "-1")
"1a" | "1a" || errorMessage("perPage", "1a")
"1a" | "1" || errorMessage("perPage", "1a")
"1" | "1a" || errorMessage("page", "1a")
Expand Down

0 comments on commit 118a4ff

Please sign in to comment.