Remove restriction on valid media types for links #182
Conversation
|
Please see here regarding your comments on #181 (comment) |
| for type_ in (MimeTypes.xml, "some random string", None): | ||
| Link(href="/hello/world", type=type_, rel="test") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #182 +/- ##
==========================================
- Coverage 96.59% 95.48% -1.11%
==========================================
Files 25 25
Lines 587 665 +78
==========================================
+ Hits 567 635 +68
- Misses 20 30 +10
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks! |
Resolves #181