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

update pagination for collection-search #155

Merged
merged 16 commits into from
Jan 13, 2025

Conversation

vincentsarago
Copy link
Member

this PR adds a tests showing that the recent collection-search addition breaks the pagination code

cc @hrodmn

@vincentsarago

This comment was marked as resolved.

@hrodmn

This comment was marked as resolved.

@hrodmn

This comment was marked as resolved.

@hrodmn

This comment was marked as resolved.

@hrodmn

This comment was marked as resolved.

* handle collection paging differently

* test next link
@vincentsarago vincentsarago changed the title add failing test update pagination for collection-search Oct 8, 2024
@hrodmn

This comment was marked as resolved.

@hrodmn

This comment was marked as resolved.

@hrodmn

This comment was marked as resolved.

@vincentsarago

This comment was marked as resolved.

@@ -303,3 +305,153 @@ async def test_get_collections_search(
"/collections",
)
assert len(resp.json()["collections"]) == 2


@requires_pgstac_0_9_2
Copy link
Member Author

Choose a reason for hiding this comment

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

This will skip the test for old version of pgstac

IMO it will be a bit bold to restrict pgstac>=0.9.2 just because of the collection-pagination issue.

We will need to add a note in the documentation stating that collection-search is partially supported for 0.8.*



@requires_pgstac_0_9_2
@pytest.mark.xfail(strict=False)
Copy link
Member Author

Choose a reason for hiding this comment

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

Marking this test to fail because it might be a pgstac bug

Copy link
Member Author

Choose a reason for hiding this comment

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

@vincentsarago vincentsarago marked this pull request as ready for review January 9, 2025 09:14
@vincentsarago

This comment was marked as resolved.

…nabled (#179)

* fallback to all_collections when `CollectionSearchExtension` is not enabled

* test all_collection fallback
query=param_string,
fragment=u.fragment,
).geturl()
href = merge_params(self.url, self.prev["body"])
Copy link
Member Author

Choose a reason for hiding this comment

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

I reverted this and now we will return URLs with offset=0

assert {"root", "self", "previous"} == {link["rel"] for link in links}
prev_link = list(filter(lambda link: link["rel"] == "previous", links))[0]
# offset=0 should not be in the previous link (because it's useless)
assert "offset" not in prev_link["href"]
Copy link
Member Author

@vincentsarago vincentsarago Jan 10, 2025

Choose a reason for hiding this comment

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

in our tests we have only 2 collections, and in pgstac the limit is set to 10 so

https://github.com/stac-utils/pgstac/blob/main/src/pgstac/sql/004a_collectionsearch.sql#L106

will not be True,this is why we don't have offset=0 previous link

cc @bitner

@vincentsarago
Copy link
Member Author

This PR is ready for review 🙏

Copy link
Collaborator

@hrodmn hrodmn left a comment

Choose a reason for hiding this comment

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

Thanks for getting this over the finish line @vincentsarago!

tests/conftest.py Outdated Show resolved Hide resolved
Co-authored-by: Henry Rodman <henry.rodman@gmail.com>
@vincentsarago vincentsarago merged commit 0178a4f into main Jan 13, 2025
8 checks passed
@vincentsarago vincentsarago deleted the patch/collection-search-paging branch January 13, 2025 13:25
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.

3 participants