Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
wants to merge 1 commit into from
Closed

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .../src/main/resources/kotlin-client/libraries/jvm-ktor/infrastructure/ApiClient.kt.mustache
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import io.ktor.client.request.parameter
import io.ktor.client.request.request
import io.ktor.client.request.setBody
import io.ktor.client.statement.HttpResponse
import io.ktor.http.ContentType
import io.ktor.http.contentType
import io.ktor.http.HttpHeaders
import io.ktor.http.HttpMethod
Expand Down Expand Up @@ -196,6 +197,7 @@ import {{packageName}}.auth.*
this.method = requestConfig.method.httpMethod
headers.filter { header -> !UNSAFE_HEADERS.contains(header.key) }.forEach { header -> this.header(header.key, header.value) }
if (requestConfig.method in listOf(RequestMethod.PUT, RequestMethod.POST, RequestMethod.PATCH)) {
contentType(ContentType.Application.Json)
Copy link
Member

@wing328 wing328 Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@bcmedeiros bcmedeiros Jun 12, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

setBody(body)
}
}
Expand Down
2 changes: 2 additions & 0 deletions .../kotlin-jvm-ktor-gson/src/main/kotlin/org/openapitools/client/infrastructure/ApiClient.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import io.ktor.client.request.parameter
import io.ktor.client.request.request
import io.ktor.client.request.setBody
import io.ktor.client.statement.HttpResponse
import io.ktor.http.ContentType
import io.ktor.http.contentType
import io.ktor.http.HttpHeaders
import io.ktor.http.HttpMethod
Expand Down Expand Up @@ -156,6 +157,7 @@ open class ApiClient(
this.method = requestConfig.method.httpMethod
headers.filter { header -> !UNSAFE_HEADERS.contains(header.key) }.forEach { header -> this.header(header.key, header.value) }
if (requestConfig.method in listOf(RequestMethod.PUT, RequestMethod.POST, RequestMethod.PATCH)) {
contentType(ContentType.Application.Json)
setBody(body)
}
}
Expand Down
2 changes: 2 additions & 0 deletions ...tlin-jvm-ktor-jackson/src/main/kotlin/org/openapitools/client/infrastructure/ApiClient.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import io.ktor.client.request.parameter
import io.ktor.client.request.request
import io.ktor.client.request.setBody
import io.ktor.client.statement.HttpResponse
import io.ktor.http.ContentType
import io.ktor.http.contentType
import io.ktor.http.HttpHeaders
import io.ktor.http.HttpMethod
Expand Down Expand Up @@ -164,6 +165,7 @@ open class ApiClient(
this.method = requestConfig.method.httpMethod
headers.filter { header -> !UNSAFE_HEADERS.contains(header.key) }.forEach { header -> this.header(header.key, header.value) }
if (requestConfig.method in listOf(RequestMethod.PUT, RequestMethod.POST, RequestMethod.PATCH)) {
contentType(ContentType.Application.Json)
setBody(body)
}
}
Expand Down
2 changes: 2 additions & 0 deletions ...kotlinx_serialization/src/main/kotlin/org/openapitools/client/infrastructure/ApiClient.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ import io.ktor.client.request.parameter
import io.ktor.client.request.request
import io.ktor.client.request.setBody
import io.ktor.client.statement.HttpResponse
import io.ktor.http.ContentType
import io.ktor.http.contentType
import io.ktor.http.HttpHeaders
import io.ktor.http.HttpMethod
Expand Down Expand Up @@ -147,6 +148,7 @@ open class ApiClient(
this.method = requestConfig.method.httpMethod
headers.filter { header -> !UNSAFE_HEADERS.contains(header.key) }.forEach { header -> this.header(header.key, header.value) }
if (requestConfig.method in listOf(RequestMethod.PUT, RequestMethod.POST, RequestMethod.PATCH)) {
contentType(ContentType.Application.Json)
setBody(body)
}
}
Expand Down