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.

Sign up for GitHub

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

remove child links (collections) in landing page response #798

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

remove child links (collections) in landing page response #798

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
d3fd61d
remove child links (collections) in landing page response
vincentsarago Feb 6, 2025
8e657ec
fix changelog
vincentsarago Feb 6, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 7 additions & 1 deletion CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,11 @@

## [Unreleased]


### Changed

- remove `child` links (`collections`) in landing page response

## [5.0.2] - 2025-01-30

### Fixed
Expand Down Expand Up @@ -575,7 +580,8 @@ Full changelog: https://stac-utils.github.io/stac-fastapi/migrations/v3.0.0/#cha

* First PyPi release!

[Unreleased]: <https://github.com/stac-utils/stac-fastapi/compare/5.0.1..main>
[Unreleased]: <https://github.com/stac-utils/stac-fastapi/compare/5.0.2..main>
[5.0.2]: <https://github.com/stac-utils/stac-fastapi/compare/5.0.1..5.0.2>
[5.0.1]: <https://github.com/stac-utils/stac-fastapi/compare/5.0.0..5.0.1>
[5.0.0]: <https://github.com/stac-utils/stac-fastapi/compare/4.0.1..5.0.0>
[4.0.1]: <https://github.com/stac-utils/stac-fastapi/compare/4.0.0..4.0.1>
Expand Down
26 changes: 0 additions & 26 deletions stac_fastapi/types/stac_fastapi/types/core.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -415,19 +415,6 @@ def landing_page(self, **kwargs) -> stac.LandingPage:
]
)

# Add Collections links
collections = self.all_collections(request=kwargs["request"])

for collection in collections["collections"]:
landing_page["links"].append(
{
"rel": Relations.child.value,
"type": MimeTypes.json.value,
"title": collection.get("title") or collection.get("id"),
"href": urljoin(base_url, f"collections/{collection['id']}"),
}
)

Comment on lines -418 to -430
Copy link
Member

Choose a reason for hiding this comment

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

How much extra complexity does it add to make this configurable? Smaller catalogs might want to keep these links — I know that I like seeing them when I hit an unfamiliar API, since the /collections endpoint can be kinda huge.

Copy link
Member Author

@vincentsarago vincentsarago Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

what we could do is to not show the collections if the collection-search extension is enables 🤷‍♂️

with the assumption that smaller catalog do not need collection-search 😅

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 should also point out that removing the all_collections call makes it easier to customize the all_collections method https://github.com/developmentseed/eoapi-devseed/pull/17/files#diff-035276bc260c2e34aa0886659327fe1950d424a2342a58879aa180f4e62e4d77R210-R223

Copy link
Member

Choose a reason for hiding this comment

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

As a user, those two things aren't obviously related — that feels like a pgstac implementation detail leaking into this general API framework library?

Copy link
Member

Choose a reason for hiding this comment

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

with the assumption that smaller catalog do not need collection-search 😅

That's a fair point. I think as long as it was documented it'd make sense.

Copy link
Contributor

@m-mohr m-mohr Feb 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

The childs added to the landing page is kinda inconsistent anyway, or does the implementation also add all the item links to the collections, too? As the latter is not done (for good reasons), the former should also not be done by default, I think. It's a somewhat weird mix of API and static for an unknown usecase ;-) Ideally it's configurable though.

Copy link
Member Author

Choose a reason for hiding this comment

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

that feels like a pgstac implementation detail leaking into this general API framework library?

I agree but at the same time if your catalog is big (whatever your backend) the landing page will have way too much link.

I feel will be really easy to customize your client to add back the links if you want them

import attr
from fastapi import Request
from stac_fastapi.pgstac.core import CoreCrudClient
from stac_fastapi.types.stac import LandingPage

@attr.s
class CustomClient(CoreCrudClient):

    async def landing_page(
        self,
        request: Request,
        **kwargs,
    ) -> LandingPage:
        landing_page = super().landing_page(request, **kwargs)

        collections = self.all_collections(request=kwargs["request"])

        for collection in collections["collections"]:
            landing_page["links"].append(
                {
                    "rel": Relations.child.value,
                    "type": MimeTypes.json.value,
                    "title": collection.get("title") or collection.get("id"),
                    "href": urljoin(base_url, f"collections/{collection['id']}"),
                }
            )

        return landing_page

While if we keep it like it is right now, to remove the call to the all_collections() method, implementer need to re-write the whole method 🤷

Copy link
Member Author

Choose a reason for hiding this comment

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

so I'm not sure what we should do ...

IMO

  • make it configurable will add complexity to stac-fastapi
  • removing it enable customization of the all_collections method
  • keep it as it is now can be annoying for large catalog

Copy link
Member

Choose a reason for hiding this comment

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

removing it enable customization of the all_collections method

👍🏼

# Add OpenAPI URL
landing_page["links"].append(
{
Expand Down Expand Up @@ -639,19 +626,6 @@ async def landing_page(self, **kwargs) -> stac.LandingPage:
]
)

# Add Collections links
collections = await self.all_collections(request=kwargs["request"])

for collection in collections["collections"]:
landing_page["links"].append(
{
"rel": Relations.child.value,
"type": MimeTypes.json.value,
"title": collection.get("title") or collection.get("id"),
"href": urljoin(base_url, f"collections/{collection['id']}"),
}
)

# Add OpenAPI URL
landing_page["links"].append(
{
Expand Down