fix: escape quoted media type directives#2217
fix: escape quoted media type directives#2217mromaszewicz merged 6 commits intooapi-codegen:mainfrom
Conversation
|
And of course, as soon as I open the PR I realize I didn't actually enable the strict-server generation 🤦♂️ |
| writer := multipart.NewWriter(w) | ||
| {{end -}} | ||
| w.Header().Set("Content-Type", {{if eq .NameTag "Multipart"}}{{if eq .ContentType "multipart/form-data"}}writer.FormDataContentType(){{else}}mime.FormatMediaType("{{.ContentType}}", map[string]string{"boundary": writer.Boundary()}){{end}}{{else if .HasFixedContentType }}"{{.ContentType}}"{{else}}response.ContentType{{end}}) | ||
| w.Header().Set("Content-Type", {{if eq .NameTag "Multipart"}}{{if eq .ContentType "multipart/form-data"}}writer.FormDataContentType(){{else}}mime.FormatMediaType("{{.ContentType}}", map[string]string{"boundary": writer.Boundary()}){{end}}{{else if .HasFixedContentType }}{{.ContentType | toGoString}}{{else}}response.ContentType{{end}}) |
There was a problem hiding this comment.
This line is not easy to read. This change was as minimal as I could make it.
At first, I ignored the use of {{.ContentType}} in the call to mime.FormatMediaType, but now that I'm reviewing this more, the same sort of bug can arise when
eq .NameTag "Multipart"is true, andeq .ContentType "multipart/form-data"is false
It's not immediately clear to me what part of the oapi spec the NameTag field refers to. I'll figure it out and include a fix and test later this evening.
There was a problem hiding this comment.
I've looked into the use of mime.FormatMediaType("{{.ContentType}}", ....
Testing locally, I found that changing it to use the toGoString function will result in function name collisions when multiple content types are defined. I managed to work around that by changing these lines to:
case strings.HasPrefix(contentType, "multipart/"):
tag = mediaTypeToCamelCase(contentType)However that caused name changes for multiple generated functions, which I think counts as a breaking change.
It seems unlikely that someone would have several possible multipart responses using quoted directives, so I don't think it's worth changing this mime.FormatMediaType call in this PR. Better to make that change in a separate PR, so greater focus can be given to possible breaking changes, if that's even something yall want to entertain.
|
Make sure that |
mromaszewicz
dismissed
their stale review
CI is finding lots of issues, need to manually review again.
|
I'm learning a lot about the projects CI setup this morning 😁 |
|
Most of the failures seemed to be small things I missed with the tests. Like a Makefile being expected in the fiber tests directory because it had a go mod file, then correctly including the oapi-codegen tool definition in go mod, and then subsequently failing because matrix tests covered versions of go that don't support the tool directive. Definitely several silly mistakes on my part, but mostly test related. I had hoped that rebasing via the github ui would keep the commit history somewhat cleaner, but looks like it represented rebase the same as if I had force pushed from my local. So, sorry about that |
|
I've really stumbled my way through the go version matrix tests- sorry about the mess of commits |
3 participants
Fixes #1529
Added tests
I appreciate yall keeping this project going- happy to make any requested changes!