Skip to content

Conversation

@obouchet
Copy link

@obouchet obouchet commented Dec 5, 2023

Description

When json media type has a version provided (like appication/json;version=v1), the generator ignores it and doesn't produce helper structs in response object (very useful for the parsing of the http response into a struct).

This PR adds the support for this type of json mediatype definition.

@jamietanna
Copy link
Member

Hmm, this is an interesting one - and one I've dealt with at a previous company - because technically this is non-standard behaviour - https://www.iana.org/assignments/media-types/application/json notes that it is not possible to specify a version parameter to the media type.

Not completely against it, but thinking that I'd be tempted to do more strict validation (namely via https://gitlab.com/jamietanna/content-negotiation-go/-/blob/main/mediatype.go?ref_type=heads) to make sure it truly is a JSON-compatible thing.

@obouchet
Copy link
Author

obouchet commented Dec 6, 2023

Hi @jamietanna

Thanks for the reply.
I agree that it is not "standard" but unfortunately it is accepted and used as an ok practice...
My PR is clearly a bad workaround but it does the trick before a proper validation is in place.

I'm trying to convince people at my job to not use this but it's a hard battle...

@jamietanna
Copy link
Member

Hey @obouchet thanks for this! I've raised #1386 which doesn't go quite as far as I'd initially suggested, but uses the standard library's mime.ParseMediaType to make sure that we get the params separated from the media type itself.

Would you like me to add you as a Co-authored-by in the commit, as you deserve some props for the change, but I wanted to ask if you're happy with it, first.

@obouchet
Copy link
Author

obouchet commented Dec 13, 2023

Hi @jamietanna!

Thank you so much for this. I like your solution and it's a good step to a full fledge solution if you like to go that far.
Very nice of you to offer me to be the co-author of this but I did nothing than just ask for the change and propose a quick and dirty solution for it...
You can stay the sole author for this for sure!

Thank you again!

@jamietanna jamietanna closed this Dec 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants