search

openmobilityfoundation / mobility-data-specification

A data standard to enable right-of-way regulation and two-way communication between mobility companies and local governments.
https://www.openmobilityfoundation.org/about-mds/
Other
676 stars 232 forks source link

Reports - Accept header media-type #810

Closed wellorder closed 1 year ago

wellorder commented 1 year ago

Explain pull request

The spec currently requires the /reports endpoint to be queried with media-type application/vnd.mds+json in the Accept header. But the endpoint returns a CSV, not JSON, so this is a bit confusing. This PR suggests a different approach (though it has its own drawback since csv is not a standard media-type suffix).

Is this a breaking change

This is a breaking change. The intent is for this to be included in MDS 2.0.

Impacted Spec

Which spec(s) will this pull request impact?

Additional context

See the discussion from https://github.com/openmobilityfoundation/mobility-data-specification/wiki/Web-conference-notes,-2022.07.07-(MDS-Working-Group) and the discussion at https://github.com/openmobilityfoundation/mobility-data-specification/issues/672

schnuerle commented 1 year ago

I think the only reason not to do this is like you said:

csv is not a standard media-type suffix

Some including myself wanted to use this anyway, but there was some concern for not using a standard media-type suffix. I would be open to accepting your changes.

wellorder commented 1 year ago

It had been long enough that I had forgotten parts of the working group discussion we had, but the notes have as an action item

add csv in reports header with note saying we know it’s not in the acceptable list

so I went with that here, and am fine with it as a solution. I'd also be open to just not using a suffix, since csv is not on the list. Using json for non-json data definitely sits poorly with me though.

schnuerle commented 1 year ago

Can we just use application/vnd.mds? Is that valid?

wellorder commented 1 year ago

This is where I have to admit to not being an expert on these conventions, I just noticed when implementing /reports that a CSV wasn't JSON and that started me looking. Reading some more, I see that text/csv is a recognized media type, so I think the appropriate thing would be text/vnd.mds+csv if I understand the conventions correctly. This would even save us from having to include a "we know this isn't standard" disclaimer. But it would be nice if someone a bit more familiar with these considerations could chime in to say that looks right :)

schnuerle commented 1 year ago

Agree and maybe @marie-x @avatarneil or @thekaveman could help with how to handle csv output.

avatarneil commented 1 year ago

I have been summoned! text/csv is definitely the correct media type to use typically for CSV downloads, so I'd say that text/vnd.mds+csv would definitely be the way to go in my opinion.