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

Added max results limit to report index #12572

Merged
merged 1 commit into from
Feb 27, 2023

Conversation

snipe
Copy link
Owner

@snipe snipe commented Feb 27, 2023

In all other places in the API, we force the API results to return a maximum of x results per API response page, as documented here. This can be overridden via .env config, but it's designed to prevent memory crashes, as many of our endpoints are querying a LOT of tables at once.

This PR simply enforces that same constraint here.

Signed-off-by: snipe <snipe@snipe.net>
@what-the-diff
Copy link

what-the-diff bot commented Feb 27, 2023

  • The limit is now set to the config value if it's higher than the max allowed.
  • If no limit was specified, then we use a default of 50 instead of 100 (the previous hardcoded value).

@snipe snipe requested a review from uberbrady February 27, 2023 20:07
Copy link
Collaborator

@uberbrady uberbrady left a comment

Choose a reason for hiding this comment

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

This makes total sense - you can 'route around' the app.max_results by just passing in a limit of 10,000,000 - and then exhaust all memory on the server.

I think this is a totally fair solution. Good catch!

@snipe snipe merged commit b78aba7 into develop Feb 27, 2023
@snipe snipe deleted the fixes/add_max_results_limit_to_acitivity_report branch February 27, 2023 20:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants