[Kotlin] [OkHttp4] detailed requests#10201
Conversation
… that returns headers and other info
|
I'm not sure how to interpret the failing AppVeyor build, can anyone advise me on whats wrong there? :) |
|
Hey, first of all, thanks for the PR you created. The http status code and the headers are already part of the responses. Are you having issues accessing the info in the responses? |
|
I have the same appveyor problem in another kotlin related PR: #10176 |
|
Yes, as far as I can tell you can't access that info from API functions, this PR generates additional API functions that do expose that information, as was previously the case with the OkHttp2 generator |
|
Do you need them in case of error, or in case of success? |
|
In case of success, in our case there is some info in the response's headers. However, getting general info about status codes, redirect, etc. is useful for debugging, too (as info in logs) |
4brunu
left a comment
4brunu
left a comment
There was a problem hiding this comment.
I see the need for this.
I left some comments, that actually I'm not sure about what's the best approach.
| fun addPetWithHttpInfo(body: Pet) : ApiInfrastructureResponse<Unit?> { | ||
| val localVariableConfig = addPetRequestConfig(body = body) | ||
|
|
||
| return request<Pet, Unit>( |
There was a problem hiding this comment.
I'm not sure if here we should return the ApiInfrastructureResponse or if we should return only the success response, and throw the other errors, something like:
@Suppress("UNCHECKED_CAST")
@Throws(UnsupportedOperationException::class, ClientException::class, ServerException::class)
fun findPetsByStatus(status: kotlin.collections.List<kotlin.String>) : kotlin.collections.List<Pet> {
val localVarResponse = findPetsByStatusWithHttpInfo(status = status)
return localVarResponse.data
}
@Suppress("UNCHECKED_CAST")
@Throws(UnsupportedOperationException::class, ClientException::class, ServerException::class)
fun findPetsByStatusWithHttpInfo(status: kotlin.collections.List<kotlin.String>) : Success<kotlin.collections.List<Pet>> {
val localVariableConfig = findPetsByStatusRequestConfig(status = status)
val localVarResponse = request<Unit, kotlin.collections.List<Pet>>(
localVariableConfig
)
return when (localVarResponse.responseType) {
ResponseType.Success -> localVarResponse
ResponseType.Informational -> throw UnsupportedOperationException("Client does not support Informational responses.")
ResponseType.Redirection -> throw UnsupportedOperationException("Client does not support Redirection responses.")
ResponseType.ClientError -> {
val localVarError = localVarResponse as ClientError<*>
throw ClientException("Client error : ${localVarError.statusCode} ${localVarError.message.orEmpty()}", localVarError.statusCode, localVarResponse)
}
ResponseType.ServerError -> {
val localVarError = localVarResponse as ServerError<*>
throw ServerException("Server error : ${localVarError.statusCode} ${localVarError.message.orEmpty()}", localVarError.statusCode, localVarResponse)
}
}
}
There was a problem hiding this comment.
I guess that works for my use case, but why not enable custom use cases by returning everything? In general I favour passing information unchanged if possible
There was a problem hiding this comment.
@wing328 what do you think about this? Should we just return the raw response or do some king of error handling? What are the other generators doing in this case?
There was a problem hiding this comment.
Maybe we should just keep the @wegendt-bosch implementation of returning the raw response because that's the more flexible approach?
| * @throws ServerException If the API returns a server error response | ||
| */ | ||
| @Throws(UnsupportedOperationException::class, ClientException::class, ServerException::class) | ||
| fun addPetWithHttpInfo(body: Pet) : ApiInfrastructureResponse<Unit?> { |
There was a problem hiding this comment.
I'm not also sure about the name withHttpInfo
There was a problem hiding this comment.
Detailed? I used WithHttpInfo since that's what it was in the past, but I'm fine with anything :)
There was a problem hiding this comment.
Please keep WithHttpInfo for the time being as that's what we've been using in other clients (e.g. C#, Ruby, etc)
|
CI build failed with the following errors: Can you please take a look when you've time? |
|
should be fixed now, added |
|
Now I see a different errors: Please take a look when you've time. |
| {{/isDeprecated}} | ||
| val localVariableConfig = {{operationId}}RequestConfig({{#allParams}}{{{paramName}}} = {{{paramName}}}{{^-last}}, {{/-last}}{{/allParams}}) | ||
|
|
||
| return request<{{#hasBodyParam}}{{#bodyParams}}{{{dataType}}}{{/bodyParams}}{{/hasBodyParam}}{{^hasBodyParam}}{{^hasFormParams}}Unit{{/hasFormParams}}{{#hasFormParams}}Map<String, Any?>{{/hasFormParams}}{{/hasBodyParam}}, {{{returnType}}}{{^returnType}}Unit{{/returnType}}>( |
There was a problem hiding this comment.
@wegendt-bosch I think the problem is here.
It should be return{{^doNotUseRxAndCoroutines}}{{#useCoroutines}}@withContext{{/useCoroutines}}{{/doNotUseRxAndCoroutines}} request<...
|
Sorry was on holiday, will get on it ASAP :) |
|
Sorry, I didn't know about that, when you have time :) |
|
It seems CI fails because of Haskell dependency HTTP errors? I'll push an empty commit later to retry |
|
@wegendt-bosch can you please merge the latest master into your branch to address the CI failure? |
…99-add-with-http-info
|
@wegendt-bosch Hi, did you got a chance to look in this? Thanks |
|
Yeah I did that ages ago :) |
|
Oh, sorry, I missed that. |
| {{/useCoroutines}} | ||
| {{/doNotUseRxAndCoroutines}} | ||
| import {{packageName}}.infrastructure.ApiClient | ||
| import {{packageName}}.infrastructure.ApiInfrastructureResponse |
There was a problem hiding this comment.
I'll rename ApiInfrastructureResponse to ApiResponse in another PR later to make it consistent with other clients (e.g. Java, C#, etc)
There was a problem hiding this comment.
Can you please create a typealias to make the transition easier?
@Deprecated("Renamed to ApiResponse", ReplaceWith("ApiResponse"))
typealias ApiInfrastructureResponse = ApiResponse
|
kotlin tests (with updated samples) passed via #10869 |
|
🎉 |
|
@wegendt-bosch sorry for the delay. We missed it as there are too many PRs... |
|
no problem :) |
4 participants
Extract from generated function per operation one that returns headers and other info. See #10199.
Example output:
before:
new:
cc @jimschubert (2017/09) ❤️, @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10) @4brunu (2019/11) @yutaka0m (2020/03) :)
PR checklist
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.
master(5.3.0),6.0.x