Skip to content
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.

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

Pagination is broken when requesting non-existent page number #15

Open
maxceem opened this issue Mar 6, 2020 · 11 comments
Open

Pagination is broken when requesting non-existent page number #15

maxceem opened this issue Mar 6, 2020 · 11 comments

Comments

@maxceem
Copy link
Contributor

maxceem commented Mar 6, 2020

If we request a page for devices which doesn't exist, the API returns all the data and doesn't return pagination headers:

https://api.topcoder-dev.com/v5/lookups/devices?page=9999

image

I guess the expected response should be no data, as we request a page that doesn't have any records.

@maxceem
Copy link
Contributor Author

maxceem commented Mar 6, 2020

Possible this comment describes the reason for this issue:

There is an edge case that the function list (criteria) in lookups-api\src\services\DeviceService.js#104` possibly misses ES and return records from DB which does not have pagination info.

@callmekatootie
Copy link
Collaborator

The issue here is that ES throws an error and we move on to DynamoDB which does not support pagination and thus returns all data.

Let's check if the error is due to the number of pages exceeding what ES can support and if yes, we simply return with empty array. Otherwise, for any other error, let us proceed as we do currently - to fetch ALL records from dynamodb

@callmekatootie
Copy link
Collaborator

Assigned to @meshde

@meshde
Copy link
Contributor

meshde commented Apr 5, 2020

ES does not return any error in this case. It simply returns an object of the form:

{
  "total": 3,
  "perPage": 1,
  "page": 999,
  "results": []
}

Since results is an empty array, this condition evaluates to false and the code moves on to fetching data from Dynamo.

As shown in the sample return object above, the code has access to pagination related data at this point. This can be used to calculate whether the empty result is due to a non-existent page (as compared to data in ES being out of sync) and the empty result can be returned then and there, without having to fetch data from Dynamo.

@callmekatootie Please confirm whether this condition check is to be added or the logic is to be left as it is.

@callmekatootie
Copy link
Collaborator

@maxceem This is now resolved in develop branch / development instance. Let me know if we are good to push to master / production

@maxceem
Copy link
Contributor Author

maxceem commented Apr 21, 2020

@callmekatootie looks like there are still some issues.

I'm trying to retrieve page 999 and get the error:

image

  • The expected result is to retrieve 20 records (default perPage value is 20).
  • And if there is no data on page 999 then retrieve an empty array [].

@callmekatootie
Copy link
Collaborator

This error is intentional - you tried to fetch more than the allowed number of records - it is a bad request (http status code 400).

Try this instead - https://api.topcoder-dev.com/v5/lookups/devices?page=499 - it is under the limit of 10,000 records and you will get an empty array. I imagine that you will get an empty array much earlier (as in, with a lesser value for page) and thus you will stop paginating beyond that.

@maxceem
Copy link
Contributor Author

maxceem commented Apr 21, 2020

This error is intentional - you tried to fetch more than the allowed number of records - it is a bad request (http status code 400).

Hmm, I was trying to request page 999 which is much less than 10,000 or the limit is 1,000?

Also, request https://api.topcoder-dev.com/v5/lookups/devices?page=999 suppose to return 20 records (not 999), which is also less than 10,000. Or the limit for pages also?

@maxceem
Copy link
Contributor Author

maxceem commented Apr 21, 2020

Try this instead - https://api.topcoder-dev.com/v5/lookups/devices?page=499 - it is under the limit of 10,000 records and you will get an empty array. I imagine that you will get an empty array much earlier (as in, with a lesser value for page) and thus you will stop paginating beyond that.

Great, I think this should be enough for real-world usage.

@callmekatootie
Copy link
Collaborator

999 is the page number. perPage query parameter decides number of records per page. It is 20 by default.

So request with query param ?page=999 requested for 999 * 20 - the 20 records after the 19,980th record - hence the error.

Let me know if that did not make sense.

@maxceem
Copy link
Contributor Author

maxceem commented Apr 21, 2020

Thanks, @callmekatootie it makes sense.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants