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

Add support for manually setting a limit on results #408

Closed
wants to merge 0 commits into from

Conversation

AlexTalks
Copy link
Contributor

@AlexTalks AlexTalks commented Sep 16, 2021

  • Added a query parameter to allow user to specify max results, rather
    than a hard limit at 5000, for each index.
  • Added support for a default max result limit in the config. If unset,
    it remains at the former default of 5000 results per index.
  • Fixed issue where no results would be returned if the limit was
    exceeded - now it simply returns the results up to the limit.

Resolves #217

Copy link
Contributor

@salemhilal salemhilal left a comment

Choose a reason for hiding this comment

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

Thank you for this PR! I especially appreciate that you've added a configuration option for this setting.

I left a few minor comments. I also want to get some feedback from the rest of the team about whether or not we should allow for a limit, but still warn once that limit has been reached. If you can handle the few minor comments in the mean time, this PR should be good to merge.

api/api.go Outdated
@@ -128,11 +129,25 @@ func parseAsUintValue(sv string, min, max, def uint) uint {
return max
}
if min != 0 && uint(iv) < min {
return max
return min
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice catch 🔥

api/api.go Outdated
@@ -18,6 +18,7 @@ import (
const (
defaultLinesOfContext uint = 2
maxLinesOfContext uint = 20
maxLimit int = 100000
Copy link
Contributor

Choose a reason for hiding this comment

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

Out of curiosity, is this number informed by anything or is it just reasonably large?

@@ -59,6 +61,41 @@ func TestSearch(t *testing.T) {
}
}

func TestSearchWithLimits(t *testing.T) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you a million times over for adding tests for this.

api/api.go Outdated
}
return uint(iv)
}

func parseAsIntValue(sv string, min, max, def int) int {
Copy link
Contributor

Choose a reason for hiding this comment

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

Could I trouble you to write a few tests on the bounds checking logic for this and for parseAsUintValue?

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

Successfully merging this pull request may close these issues.

Exceed limit results
3 participants