[kotlin][client] improve debug experience - #5286
Conversation
|
👍 Thanks for opening this issue! The team will review the labels and make any necessary changes. |
| ResponseType.ServerError -> { | ||
| val localVarError = localVarResponse as ServerError<*> | ||
| throw ServerException(localVarError.message ?: "Server error", localVarError.statusCode) | ||
| throw ServerException(localVarError.body as? String ?: "Server error", localVarError.statusCode) |
There was a problem hiding this comment.
I think we might want to rethink using response bodies here, as it could expose these clients to reflection and log injection attacks. I think it makes sense to expose the body of the error, but not as the message which the user is likely logging.
I'm wondering as well, why would this ever be null and require some change here? Wouldn't it previously have been Server error?
There was a problem hiding this comment.
It is like this since I use OpenAPI, but I only use it for a few months.
I understand your concern, this already happens with the client error.
I open this PR, because when a server error happens, error message is not very helpful.
On the other hand with the ClientError, it's really easy to debug a problem.
Do you have any idea on how can we achieve a better debugging experience?
If you don't want to to put the body as the exception message, maybe we can create an extra variable, like body in server and client error?
There was a problem hiding this comment.
I wonder if the ServerException and ClientException could have a property holding their respective errors? This way, the exception's message is still clearly a message and additional details are still present for debugging purposes?
I think the easiest way to do this without breaking existing clients might be to create a marker interface on ApiInfrastructureResponse<T> such as:
interface Response
abstract class ApiInfrastructureResponse<T>(val responseType: ResponseType) {
abstract val statusCode: Int
abstract val headers: Map<String,List<String>>
}: Response
Then, the client/server exceptions could add ClientError and ServerError as properties with null defaults:
open class ClientException(message: kotlin.String? = null, val statusCode: Int = -1, val response: Response? = null) : RuntimeException(message) {
companion object {
private const val serialVersionUID: Long = 123L
}
}
open class ServerException(message: kotlin.String? = null, val statusCode: Int = -1, val response: Response? = null) : RuntimeException(message) {
companion object {
private const val serialVersionUID: Long = 456L
}
}
I think that would allow us to define Client/Server exceptions more cleanly by assigning response.body to the Response? property.
It wouldn't protect from log injection if someone is logging exception.response, but that would be on the consumer and no fault of the generated code.
What are your thoughts?
There was a problem hiding this comment.
It looks good to me.
I have just a few questions.
Why do you want an interface Response? To hide ApiInfrastructureResponse?
And what value would we put in the message of the client and server exceptions?
A plain string "Client Error" and "Server Error" to avoid exposing the body?
There was a problem hiding this comment.
The Response interface would be needed because Kotlin doesn't support wildcard type parameters in the same way as Java. If you were to carry this type info, it would mean making the exceptions generic.
I think the message for server exception should be "Server Error" for 500 and probably "Server Error : name" for other 5xx where name is the human readable value of the status code (e.g. Bad Gateway).
For Client Errors, we could probably use similar Code Name format, but they're technically not exceptional behavior. Probably something like "Client returned a 403 Forbidden"?
There was a problem hiding this comment.
How can I translate the status code into Bad Gateway for example?
There was a problem hiding this comment.
It's done, what do you think of it?
There was a problem hiding this comment.
Sorry for the delayed response. Looks excellent! Seems that your fixes now address both of our concerns.
4brunu
changed the title
| ) : ApiInfrastructureResponse<T>(ResponseType.Redirection) | ||
|
|
||
| {{#nonPublicApi}}internal {{/nonPublicApi}}class ClientError<T>( | ||
| val message: String? = null, |
There was a problem hiding this comment.
note that this shouldn't be a breaking change because it's an internal type.
* [kotlin][client] remove null message on server error * [kotlin][client] avoid breaking change * [kotlin][client] add response to client and server exceptions * [kotlin][client] improve message on ClientException and ServerException
2 participants
Currently when a server error occurs, it always returns a null message.
Since the message is always null, it shows the message "Server Error" which it's not very helpful for debugging proposes.
In this PR, I propose that we return the server response instead of the null message.
PR checklist
./bin/(or Windows batch scripts under.\bin\windows) to update Petstore samples related to your fix. This is important, as CI jobs will verify all generator outputs of your HEAD commit, and these must match the expectations made by your contribution. You only need to run./bin/{LANG}-petstore.sh,./bin/openapi3/{LANG}-petstore.shif updating the code or mustache templates for a language ({LANG}) (e.g. php, ruby, python, etc).master,4.3.x,5.0.x. Default:master.@jimschubert (2017/09) ❤️, @dr4ke616 (2018/08) @karismann (2019/03) @Zomzog (2019/04) @andrewemery (2019/10) @4brunu (2019/11)