search

astropy / pyvo

An Astropy affiliated package providing access to remote data and services of the Virtual Observatory (VO) using Python.
https://pyvo.readthedocs.io/en/latest
BSD 3-Clause "New" or "Revised" License
74 stars 50 forks source link

Modified sia up-casing behavior #544

Closed d-giles closed 2 months ago

d-giles commented 2 months ago

Resolves an issue where the SIA service up-cases the format parameter inappropriately.

msdemlei commented 2 months ago

First off, case folding is cursed in general. Don't do it in new standards.

Then, sect. 4.1.4 of SIA 1.0 does not specify the case-sensitivity of the FORMAT parameter. I would hence argue that it is intended to preserve case, and hence I'm not particularly wild about current upcasing in pyVO. On the other hand, RFC 2045ff media types are (except for parameter values, strictly speaking) case-insensitive, and hence implementations should not care about the case in the FORMAT parameter either when there are media types in there. On the third hand, I'm pretty sure that implementations, if they support FORMAT in some meaningful way at all, will not do the right thing in terms of case folding.

So: It's a mess, as always when people start normalising case, even in those cases when we don't leave cosy ASCII-land.

Given that, I suppose we should be treading lightly, and this PR (did you close it on purpose?) goes some way towards that.

So: I'm all for stopping case-normalising media types. Neither clients nor servers will expect that, and we'll needlessly trigger bugs the fixing of which helps (presumably) nobody.

I'm neutral on normalising the magic values as long as we document what people should be passing in there; I'm not 100% sure the magic values should be exposed to users at all, though. IMHO they have no business to mess with METADATA, at least. If we want to expose that functionality, it should be behind an API.

If we do case folding on the magic values, though, we should do that on all of them, which, I think, are ALL, GRAPHIC, METADATA, GRAPHIC-ALL (the latter being optional).

There is also the odd example "GRAPHIC-jpeg,png,gif" in the spec. I doubt that's in use anywhere. Let's pretend it's not there.

d-giles commented 2 months ago

I did close it on purpose, the PR was premature and, as you mentioned, failed to account for all use cases. The "GRAPHIC-jpeg,png,gif" doesn't work even properly type cased.

Delving deeper into the issue, it does seem that in order to make things work properly (at least in this instantiation), case normalization is necessary: either all upper or all lower. One problem to the next.