Paginated search queries now don't return a token on the last page

[pedro-cf]

Related Issue(s):

• Paginated search queries always returns a next token even if it's the last page of the pagination. #242

Merge dependencie(s):

• Always generate links for all searches #241

Description:

• Paginated search queries now don't return a token on the last page.
• Made some fixes to the respective tests. In particular test_pagination_token_idempotent had and indentation issue
• Improved execute_search to make use of es_response["hits"]["total"]["value"]

PR Checklist:

• Code is formatted and linted (run pre-commit run --all-files)
• Tests pass (run make test)
• Documentation has been updated to reflect changes, if applicable
• Changes are added to the changelog

[StijnCaerts]

es_response["hits"]["total"]["value"] might not be accurate if there are more than 10,000 hits. Then a lower bound will be indicated with the "gte" relation in es_response["hits"]["total"]["relation"].

https://www.elastic.co/guide/en/elasticsearch/reference/current/search-your-data.html#track-total-hits

So I'd suggest to use the count from the search only if es_response["hits"]["total"]["relation"] == "eq". The async count tasks are still useful when the count exceeds the default threshold.

[jonhealy1]

Maybe we can add to this test or do something similar to make sure that the last page isn't returning a token? #244

[pedro-cf]

Maybe we can add to this test or do something similar to make sure that the last page isn't returning a token? #244

Greetings, a test for this already exists test_pagination_item_collection.

It used to test for 7 requests for 6 items, now I've fixed it for 6 requests for 6 items.

https://github.com/stac-utils/stac-fastapi-elasticsearch-opensearch/pull/243/files#diff-a38bef06b3f69317891c409bc81044f53cca52841ea63ec9a6821ea08dea98f6L593-L605

test_item_search_temporal_window_timezone_get and test_pagination_post also tests this.

These are all tests I've fixed up.

[pedro-cf]

es_response["hits"]["total"]["value"] might not be accurate if there are more than 10,000 hits. Then a lower bound will be indicated with the "gte" relation in es_response["hits"]["total"]["relation"].

https://www.elastic.co/guide/en/elasticsearch/reference/current/search-your-data.html#track-total-hits

So I'd suggest to use the count from the search only if es_response["hits"]["total"]["relation"] == "eq". The async count tasks are still useful when the count exceeds the default threshold.

Greetings @StijnCaerts , thank you so much for the feedback.

Do you think this approach is correct?

        search_task = asyncio.create_task(
            self.client.search(
                index=index_param,
                ignore_unavailable=ignore_unavailable,
                body=search_body,
                size=limit,
            )
        )

        count_task = asyncio.create_task(
            self.client.count(
                index=index_param,
                ignore_unavailable=ignore_unavailable,
                body=search.to_dict(count=True),
            )
        )

        try:
            es_response = await search_task
        except exceptions.NotFoundError:
            raise NotFoundError(f"Collections '{collection_ids}' do not exist")

        hits = es_response["hits"]["hits"]
        items = (hit["_source"] for hit in hits)

        matched = es_response["hits"]["total"]["value"]
        if es_response["hits"]["total"]["relation"] != "eq":
            if count_task.done():
                try:
                    matched = count_task.result().get("count")
                except Exception as e:
                    logger.error(f"Count task failed: {e}")
        else:
            count_task.cancel()

1. Assume matched = es_response["hits"]["total"]["value"] as default
2. In the case es_response["hits"]["total"]["relation"] != "eq" use the count_task and if the count fails it will log the error but still use the default as backup
3. Else use the default and cancel the count_task if it's still running (I assume count_task.cancel() is safe to call)

[StijnCaerts]

I think either getting the correct count or no count at all would be the preferred behaviour.
The context extension is deprecated at this point, I don't know if there is an alternative available. Maybe we should handle this in a separate PR.

https://github.com/stac-api-extensions/context
radiantearth/stac-api-spec#396

[pedro-cf]

I think either getting the correct count or no count at all would be the preferred behaviour.

Is there situations where the count can fail ? Is this why count_task.result().get("count") is surrounded in a try/except?

With the addition of the page in the token it's critical to get a matched value everytime.

[StijnCaerts]

Is there situations where the count can fail ? Is this why count_task.result().get("count") is surrounded in a try/except?

Probably the same reason a search request could fail, eg. invalid collection, bad query, ...

Without an accurate count, it is impossible to tell if we're on the last page. The only case where you are sure you're on the last page is when the current page size is less than the limit.

[jonhealy1]

I don't know why it's in a try except myself? A lot of the db stuff was done by an old contributor.

[jonhealy1]

es_response["hits"]["total"]["value"] is accurate up to 10,000 results. If the actual count_task fails - which is probably unlikely - we can use this maybe because most people are not going to paginate through more than 10,000 results.

[StijnCaerts]

Indeed, if you are paging through more than 10,000 hits, I think there is no harm in 1 extra request with an empty response
😉

[StijnCaerts]

Another option would be to implement this workaround: https://stackoverflow.com/a/67200853/9339603

Pro's:

• uniform handling regardless of number of hits ($.hits.total.relation: eq / gte)
• no need track count in the pagination token

Con's:

• edge cases like limit=0
• performance impact?

[pedro-cf]

Another option would be to implement this workaround: https://stackoverflow.com/a/67200853/9339603

@StijnCaerts I've tried implementing this approach

        search_after = None

        if token:
            search_after = urlsafe_b64decode(token.encode()).decode().split(",")

        query = search.query.to_dict() if search.query else None

        index_param = indices(collection_ids)

        search_task = asyncio.create_task(
            self.client.search(
                index=index_param,
                ignore_unavailable=ignore_unavailable,
                query=query,
                sort=sort or DEFAULT_SORT,
                search_after=search_after,
                size=limit + 1,  # Fetch one more result than the limit
            )
        )

        count_task = asyncio.create_task(
            self.client.count(
                index=index_param,
                ignore_unavailable=ignore_unavailable,
                body=search.to_dict(count=True),
            )
        )

        try:
            es_response = await search_task
        except exceptions.NotFoundError:
            raise NotFoundError(f"Collections '{collection_ids}' do not exist")

        hits = es_response["hits"]["hits"]
        items = (hit["_source"] for hit in hits[:limit])

        next_token = None
        if len(hits) > limit:
            if hits and (sort_array := hits[limit - 1].get("sort")):
                next_token = urlsafe_b64encode(
                    ",".join([str(x) for x in sort_array]).encode()
                ).decode()

        matched = None
        if count_task.done():
            try:
                matched = count_task.result().get("count")
            except Exception as e:
                logger.error(f"Count task failed: {e}")

        return items, matched, next_token

but I'm getting these errors on these tests:

FAILED stac_fastapi/tests/api/test_api.py::test_app_query_extension_limit_gt10000 - elasticsearch.BadRequestError: BadRequestError(400, 'search_phase_execution_exception', 'Result window is too large, from + size must be less than or e...
FAILED stac_fastapi/tests/api/test_api.py::test_app_query_extension_limit_10000 - elasticsearch.BadRequestError: BadRequestError(400, 'search_phase_execution_exception', 'Result window is too large, from + size must be less than or e...

[pedro-cf]

As I understand elasticsearch itself doesn't allow limits above 10,000 (by default), so when you pass 10,000 +1 it responds with a bad request.

I was a bit confused from why these tests weren't failing on the main branch but it's because of this from stac-fastapi changelog:

• Limit values above 10,000 are now replaced with 10,000 instead of returning a 400 error (#526)

[pedro-cf]

Ended up applying this #243 (comment) with edge case handling, in this case the 10,000.

[pedro-cf]

what do you think @jonhealy1 ?

I don't really like this hardcoded 10,000, but it's basically set by:

• elasticsearch setting which only allows requests with up to 10,000 by default.
• stac-fastapi.api replaces values above limit with limit value (10,000)

[jonhealy1]

Can we import the max limit from stac-fastapi?

[pedro-cf]

Can we import the max limit from stac-fastapi?

Yes, we can use this:

import stac_fastapi.types.search
max_result_window =  stac_fastapi.types.search.Limit.le

or would you prefer this:

from stac_fastapi.types.search import Limit
max_result_window = Limit.le

I prefer option 1 since it makes it clear where the limit comes from.

[jonhealy1]

I prefer option one too

[pedro-cf]

I prefer option one too

added

[jonhealy1]

Nice work here Pedro. I will wait until tomorrow to merge just in case anyone else has any thoughts.

[StijnCaerts]

I just left one small remark. Otherwise it looks good to me, thanks for the contribution!

[jonhealy1]

Approved!