-
Notifications
You must be signed in to change notification settings - Fork 31
Default and max allowed returned values for collections and items #482
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
base: main
Are you sure you want to change the base?
Changes from all commits
77df588
9d58438
ec29983
20fa813
c6df9f1
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -270,34 +270,31 @@ async def all_collections( | |
""" | ||
base_url = str(request.base_url) | ||
|
||
# Get the global limit from environment variable | ||
global_limit = None | ||
env_limit = os.getenv("STAC_ITEM_LIMIT") | ||
if env_limit: | ||
try: | ||
global_limit = int(env_limit) | ||
except ValueError: | ||
# Handle invalid integer in environment variable | ||
pass | ||
|
||
# Apply global limit if it exists | ||
if global_limit is not None: | ||
# If a limit was provided, use the smaller of the two | ||
if limit is not None: | ||
limit = min(limit, global_limit) | ||
else: | ||
limit = global_limit | ||
global_max_limit = ( | ||
int(os.getenv("STAC_GLOBAL_COLLECTION_MAX_LIMIT")) | ||
if os.getenv("STAC_GLOBAL_COLLECTION_MAX_LIMIT") | ||
else None | ||
) | ||
default_limit = int(os.getenv("STAC_DEFAULT_COLLECTION_LIMIT", 300)) | ||
query_limit = request.query_params.get("limit") | ||
|
||
body_limit = None | ||
try: | ||
if request.method == "POST" and request.body(): | ||
body_data = await request.json() | ||
body_limit = body_data.get("limit") | ||
except Exception: | ||
pass | ||
|
||
if body_limit is not None: | ||
limit = int(body_limit) | ||
elif query_limit: | ||
limit = int(query_limit) | ||
else: | ||
# No global limit, use provided limit or default | ||
if limit is None: | ||
query_limit = request.query_params.get("limit") | ||
if query_limit: | ||
try: | ||
limit = int(query_limit) | ||
except ValueError: | ||
limit = 10 | ||
else: | ||
limit = 10 | ||
limit = default_limit | ||
|
||
if global_max_limit is not None: | ||
limit = min(limit, global_max_limit) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. When we do this - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Maybe test these cases - let me know what you think: Test with no global max limit set (should allow any limit). There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It was agreed that we should set a global limit for There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @jonhealy1 I have added the tests, removed not relevant test: Test with limit higher than global max items/collections (should be capped). - test_global_collection_max_limit_set(), test_default_collection_limit() Test with limit lower than global max (should be respected). - not sure if we need this test. Test with no global max limit set (should allow any limit). - not relevant, unless agree to allow any limit. Also, overpopulating the test collections/items slows down the test. Test with global max limit set for items/collection (should cap at that limit). - test_global_collection_max_limit_set(), test_global_item_max_limit_set() There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. How can the value be overridden by an env var? Maybe I am missing something here? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. "Test with no global max limit set (should allow any limit). - not relevant, unless agree to allow any limit. Also, overpopulating the test collections/items slows down the test" @YuriZmytrakov No one envisions having an api returning unlimited items. You set the default max limit for Items to 100. If a User wants to return 110 items, they are unable to (unless I am missing something, please explain). There is no reason to place these restrictions. People who use this project expect flexibility. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can you test this by showing how the limit param filters down to execute_search - test what its final value is - without ingesting over 100 Items? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Discussed with the team — yes, we should not restrict the number of items, collections that can be returned by default. This was fixed by removing the default values items=100, collections=300. For this reason, I’m adding proper tests to verify that when no global max limit is set (any limit is allowed) populated 20 items, collections and querying to ensure any limit is allowed. |
||
|
||
# Get token from query params only if not already provided (for GET requests) | ||
if token is None: | ||
|
@@ -569,7 +566,7 @@ async def item_collection( | |
request (Request): FastAPI Request object. | ||
bbox (Optional[BBox]): Optional bounding box filter. | ||
datetime (Optional[str]): Optional datetime or interval filter. | ||
limit (Optional[int]): Optional page size. Defaults to env ``STAC_ITEM_LIMIT`` when unset. | ||
limit (Optional[int]): Optional page size. Defaults to env `STAC_DEFAULT_ITEM_LIMIT` when unset. | ||
sortby (Optional[str]): Optional sort specification. Accepts repeated values | ||
like ``sortby=-properties.datetime`` or ``sortby=+id``. Bare fields (e.g. ``sortby=id``) | ||
imply ascending order. | ||
|
@@ -660,15 +657,12 @@ async def get_search( | |
q (Optional[List[str]]): Free text query to filter the results. | ||
intersects (Optional[str]): GeoJSON geometry to search in. | ||
kwargs: Additional parameters to be passed to the API. | ||
|
||
Returns: | ||
ItemCollection: Collection of `Item` objects representing the search results. | ||
|
||
Raises: | ||
HTTPException: If any error occurs while searching the catalog. | ||
""" | ||
limit = int(request.query_params.get("limit", os.getenv("STAC_ITEM_LIMIT", 10))) | ||
|
||
base_args = { | ||
"collections": collections, | ||
"ids": ids, | ||
|
@@ -743,6 +737,25 @@ async def post_search( | |
Raises: | ||
HTTPException: If there is an error with the cql2_json filter. | ||
""" | ||
global_max_limit = ( | ||
int(os.getenv("STAC_GLOBAL_ITEM_MAX_LIMIT")) | ||
if os.getenv("STAC_GLOBAL_ITEM_MAX_LIMIT") | ||
else None | ||
) | ||
default_limit = int(os.getenv("STAC_DEFAULT_ITEM_LIMIT", 10)) | ||
|
||
requested_limit = getattr(search_request, "limit", None) | ||
|
||
if requested_limit is None: | ||
limit = default_limit | ||
else: | ||
limit = requested_limit | ||
|
||
if global_max_limit: | ||
limit = min(limit, global_max_limit) | ||
|
||
search_request.limit = limit | ||
|
||
base_url = str(request.base_url) | ||
|
||
search = self.database.make_search() | ||
|
@@ -819,7 +832,6 @@ async def post_search( | |
if hasattr(search_request, "sortby") and getattr(search_request, "sortby"): | ||
sort = self.database.populate_sort(getattr(search_request, "sortby")) | ||
|
||
limit = 10 | ||
if search_request.limit: | ||
limit = search_request.limit | ||
|
||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of using
request.query_params.get("limit")
orbody_data.get("limit")
, can you just uselimit
from the function params?