Skip to content

Remove restriction on valid media types for links #182

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

Merged
merged 4 commits into from
Jun 18, 2025
Merged

Remove restriction on valid media types for links #182

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
7e7e463
Remove restriction on valid media types for links
mishaschwartz Jun 4, 2025
42456f1
add test
mishaschwartz Jun 4, 2025
e72e74a
add back explicit reference to mimetypes
mishaschwartz Jun 16, 2025
5b56754
typing for old python
mishaschwartz Jun 17, 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
2 changes: 2 additions & 0 deletions CHANGELOG.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,8 @@

## Unreleased

- Remove restriction on valid media types for links.

## 3.3.1 (2025-06-01)

- Add back `SearchDatetime` in `stac_pydantic.api.search` to avoid breaking change
Expand Down
4 changes: 2 additions & 2 deletions stac_pydantic/links.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
from enum import auto
from typing import Iterator, List, Optional
from typing import Iterator, List, Optional, Union
from urllib.parse import urljoin

from pydantic import ConfigDict, Field, RootModel
Expand All @@ -15,7 +15,7 @@ class Link(StacBaseModel):

href: str = Field(..., alias="href", min_length=1)
rel: str = Field(..., alias="rel", min_length=1)
type: Optional[MimeTypes] = None
type: Optional[Union[MimeTypes, str]] = None
title: Optional[str] = None

# Label extension
Expand Down
5 changes: 5 additions & 0 deletions tests/api/test_links.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,3 +80,8 @@ def test_resolve_pagination_link():
for link in links.link_iterator():
if isinstance(link, PaginationLink):
assert link.href == "http://base_url.com/next/page"


def test_link_types():
for type_ in (MimeTypes.xml, "some random string", None):
Link(href="/hello/world", type=type_, rel="test")
Comment on lines +86 to +87
Copy link
Contributor

@fmigneault fmigneault Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

I believe this test is inappropriate, or at least should not "encourage" it with explicitly validating erroneous values.

Ideally, href should have a protocol, specifically file:// if local files makes any sense here.
Though, that has other implications in STAC (eg: stac-utils/pystac#1347), so I would let them decide about this.

Media-type should at least validate that [\w-]+/[\w-.]+.* are handled as bare minimum (ie: RFC6838 section 4.2).
It could be more strict to consider only valid RFC2046 types if desired, and could define the specific ; and parameters regexes if really invested in validating the specific pattern, but they have to minimally be permitted.

Copy link
Member

Choose a reason for hiding this comment

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

explicitly validating erroneous values

I don't think there's anything technically "erroneous" about some random string ... while the provided RFC6838 link provides a syntax requirement for "registered media types", we're explicitly allowing non-registered media types in this library, so the syntax requirement doesn't apply.

I'd be ok w/ warning if the media type doesn't fit some sort of expectation (either in our list, or that regex, or whatever), but I think an error is too strong.

W.r.t. the href, there's a lot of "absolute file system" hrefs in the wild, so I'm ok with including it here. If we wanted to be "correct" we could update it to ./hello/world.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wouldn't application/x-[something] or application/vdn.[something] be required for unregistered types? Won't most libraries break if there is not at least a <type>/<subtype> split?

Copy link
Member

@gadomski gadomski Jun 6, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Maybe? I guess I'm just curious what we're trying to gain by validating others' media types. I generally favor "permissive reads, strict writes" for STAC tooling

For this library, I think that means warning on "surprising" (not erroring) media types and providing string constants for "correct" ones (which we do).

Regarding the split question, I've seen a lot of libraries do direct string matching (not splitting) , which means subtypes or profiles (like cloud-optimized) break matching anyways.

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm fine if that's the decision. My understanding was that it was the opposite with strict validation given that the (too)specific set of MediaTypes enum was used instead of str from the start. As mentioned, I think the bare minimum would be [\w-]+/[\w-.]+.* that would allow most flexibility while at least avoiding basic mistakes of malformed media-types. Note that the pattern includes .* specifically to allow the profile=cloud-optimized or other similar parameters.