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

Modified sia up-casing behavior #544

Closed
wants to merge 1 commit into from
Closed

Conversation

d-giles
Copy link
Contributor

@d-giles d-giles commented May 1, 2024

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

@bsipocz bsipocz added the bug label May 1, 2024
@bsipocz bsipocz added this to the v1.5.2 milestone May 1, 2024
@d-giles d-giles closed this May 1, 2024
@msdemlei
Copy link
Contributor

msdemlei commented May 2, 2024

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
Copy link
Contributor Author

d-giles commented May 2, 2024

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.

@bsipocz bsipocz removed this from the v1.5.2 milestone May 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants