fix(kotlin): set content type on ktor-jvm library

[bcmedeiros]

Draft for #15811

PR checklist

• Read the contribution guidelines.
• Pull Request title clearly describes the work in the pull request and Pull Request description provides details about how to validate the work. Missing information here may result in delayed response from the community.
• Run the following to build the project and update samples:

  ./mvnw clean package 
  ./bin/generate-samples.sh
  ./bin/utils/export_docs_generators.sh
  

  Commit all changed files.
  This is important, as CI jobs will verify all generator outputs of your HEAD commit as it would merge with master.
  These must match the expectations made by your contribution.
  You may regenerate an individual generator by passing the relevant config(s) as an argument to the script, for example ./bin/generate-samples.sh bin/configs/java*.
  For Windows users, please run the script in Git BASH.
• In case you are adding a new generator, run the following additional script :

  ./bin/utils/ensure-up-to-date
  

  Commit all changed files.
• File the PR against the correct branch: master (6.3.0) (minor release - breaking changes with fallbacks), 7.0.x (breaking changes without fallbacks)
• If your PR is targeting a particular programming language, @mention the technical committee members, so they are more likely to review the pull request.

[bcmedeiros]

@wing328 This is the other issue I was referring to in #15793

generate-samples.sh is creating a lot of files, so I haven't "run" it, but I think there is something going on there. I tried to update just the templates affected by my change and everything seems all right.

[wing328]

cc @jimschubert (2017/09) ❤️, @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10) @4brunu (2019/11) @yutaka0m (2020/03) @stefankoppier (2022/06)

[wing328]

thanks for the PR.

instead of hard coding to applicaiton/json, what about updating api.mustache to use content-type defined in the sepc instead similar to what we've done in https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/kotlin-client/libraries/jvm-okhttp/api.mustache#L222 ?

[bcmedeiros]

@wing328 My first try was:

contentType(ContentType.parse("{{contentType}}"))

I also just tried:

contentType(ContentType.parse("{{{mediaType}}}"))

Both result in an empty value:

contentType(ContentType.parse(""))

Also, even if I get this correctly, I'm pretty sure we would need some conditionals to add the correct gradle dependencies in the template.

At this point I know very little about Mustache and the project layout overall.

[wing328]

can you please share your spec? or a minimal spec to reproduce the issue?

My first try was:

did you do it in api.mustache inside the operation mustache tag?

I would suggest plotting the fix in https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/kotlin-client/libraries/jvm-okhttp/api.mustache#L222 to the api.mustache for ktor-jvm library.

[bcmedeiros]

did you do it in api.mustache inside the operation mustache tag?

I see I need to be in the scope of the operation now, but anyway, I cannot set the content type there due to the current design of the operation and the jsonRequest and request abstractions in ApiClient.

The current fix is done in a method called request, whose only caller is jsonRequest, so I highly doubt that "making this right" and set the dynamic Content Type will actually work.

I'll try a little bit longer, though.

[wing328]

You can use this fix with customized template (e.g. -t option CLI) if you need this fix urgently for your use case.

[wing328]

If you look at https://github.com/OpenAPITools/openapi-generator/blob/master/modules/openapi-generator/src/main/resources/kotlin-client/libraries/jvm-okhttp/api.mustache#L222, the content-type header is set there instead of ApiClient.

can we not do the same? or you saying jvm-okhttp has the issue for your use case?

[bcmedeiros]

@wing328 not without a major redesign. We need to do it inside the client.request { block, which the current design chose to abstract in the ApiClient.

[bcmedeiros]

@wing328 is it time for a big redesign? should I give it a try?

[wing328]

I don't think I fully understand the problem/limitation that requires the redesign.

Can you please PM me via Slack to further discuss this when you've time?

[bcmedeiros]

I don't think I fully understand the problem/limitation that requires the redesign.

It's not easy to understand the problem without looking at the code. Basically, the content type is being set in a super class method that has no access to the config, IIRC.

Can you please PM me via Slack to further discuss this when you've time?

The issue ended up being fixed by #16843, so I'll close this PR.

That solution ended up getting the content type from the header, or a hardcoded value as a fallback, BTW, so we still haven't "fixed" this properly as you suggested.

[bcmedeiros]

As explained in the comment above, a similar fix was merged as part of #16843