Fix incorrect placement of header=present in versions endpoint

[ml-evs]

Closes #418

[ml-evs]

Hmmm, I don't like that this changes the OpenAPI schemas, perhaps there is a way smarter way to pass these kind of parameters.

[ml-evs]

Hmmm, I don't like that this changes the OpenAPI schemas, perhaps there is a way smarter way to pass these kind of parameters.

Okay, replaced it with a slightly gross hack, think it should be stable enough.

[codecov]

Codecov Report

Merging #419 into master will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master     #419   +/-   ##
=======================================
  Coverage   90.99%   90.99%           
=======================================
  Files          55       55           
  Lines        2522     2522           
=======================================
  Hits         2295     2295           
  Misses        227      227           

Flag	Coverage Δ
#unittests	90.99% <100.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
optimade/server/routers/versions.py	100.00% <100.00%> (ø)

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d8443d6...ca28a84. Read the comment docs.

[CasperWA]

I was just about to suggest the option you went for in the initial commit.

I will just ping @rartino, @merkys and @sauliusg on their opinion on whether the headers MIME type parameter should be added to the OpenAPI response type value or not.

[sauliusg]

I will just ping @rartino, @merkys and @sauliusg on their opinion on whether the headers MIME type parameter should be added to the OpenAPI response type value or not.

I would say yes, headers=present should be added, since our OPTIMADE specs mandate that header MUST always be present (to be future-proof), and RFC 4180 says "the presence or absence of the header line should be indicated via the optional "header" parameter of this MIME type".

[rartino]

I agree with @sauliusg, the most correct thing to do is to include header=present. Technically I don't think leaving it out is a strict violation of RFC 4180, but it certainly is better to include it. However, note the singular header not headers.

[ml-evs]

I agree with @sauliusg, the most correct thing to do is to include header=present. Technically I don't think leaving it out is a strict violation of RFC 4180, but it certainly is better to include it. However, note the singular header not headers.

Just to clarify, we will definitely include it in the response (and even in the correct place this time), it's just whether it should be included in the OpenAPI schema, which would look like this:

"content": {
    "text/csv; header=present": {
        "schema": {
            "type": "string"
        }
}

This looks a bit funny to me, but I'm not sure of any other way of doing it. This also excludes the other parameter charset=utf-8 that gets added to the Content-Type in a response.

[CasperWA]

I think, let's add it to the OpenAPI as well. Being explicit.
While the charset parameter is not added, the header parameter is much more CSV-specific, and hence I think it's fine to not include charset, while including header.

[ml-evs]

I think, let's add it to the OpenAPI as well. Being explicit.
While the charset parameter is not added, the header parameter is much more CSV-specific, and hence I think it's fine to not include charset, while including header.

Fair enough, it certainly makes the fix easier!

[CasperWA]

Great catch with this one @ml-evs !