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

api: limit group queries to a max of 100 results #4878

Merged
merged 1 commit into from Feb 10, 2017

Conversation

mattrobenolt
Copy link
Contributor

This was entirely unbounded and would result in requests timing out.

dcramer
dcramer previously approved these changes Feb 7, 2017
Copy link
Member

@dcramer dcramer left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for some reason I thought we had an explicit hard cap on max limit in the paginator -- we should probably add that. I'm ok with it being 100 items.

@mattrobenolt
Copy link
Contributor Author

Do you want me to make the paginator entirely limited? I can do that instead.

@dcramer
Copy link
Member

dcramer commented Feb 7, 2017

Yeah -- the only question I have is it feels like we should raise an error when limit > max, but that might regress for some people. Still probably safer.

@mattrobenolt
Copy link
Contributor Author

I've definitely seen other APIs just silently cap. If you try to do somethign too large, it instead just returns the max results without erroring. In theory, this should all be ok if we don't hard error since if someone were following the cursor, it should work correctly. They'd just have to page more than anticipated.

@mattrobenolt mattrobenolt dismissed dcramer’s stale review February 7, 2017 20:02

Going to apply limit at paginator

@dcramer
Copy link
Member

dcramer commented Feb 7, 2017

will defer to whatever you all think is best here, but +1 on hard cap in paginator (but making it a param so if an endpoint wants to allow a higher cap it can)

@mattrobenolt mattrobenolt force-pushed the limit-group-query branch 2 times, most recently from d3796b3 to d2db99b Compare February 7, 2017 20:30
@mattrobenolt mattrobenolt merged commit f1bb4a1 into master Feb 10, 2017
@mattrobenolt mattrobenolt deleted the limit-group-query branch February 10, 2017 20:47
@github-actions github-actions bot locked and limited conversation to collaborators Dec 23, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants