try/catch when attempting to deserialize ServerError from response #3466
Conversation
This commit adds a try/catch block around attempting to deserialize a ServerError from the body, since IElasticsearchResponse types withn Elasticsearch.Net set/attempt to set the response body, irrespective of the response status code. Move BytesResponseTests and StringResponseTests from Tests.Reproduce into main Tests project. Allow byte array to be passed to FixedResponseClient. Fixes #3378
| using(var stream = new MemoryStream(this.Body)) | ||
| serverError = ServerError.Create(stream); | ||
| return true; | ||
| try |
There was a problem hiding this comment.
Should the try catch happen inside ServerError.Create()? Can we use the response mimetype as a hint as well here? e.g in the nginx proxy case we already know what we got back from the server is not json.
There was a problem hiding this comment.
++ to using the MIME type.
Should the try catch happen inside ServerError.Create()
I think the try/catch should be in the Try* method, with the potential for ServerError.Create() to throw an exception if a ServerError cannot be created from the Stream. To my mind, this aligns with BCL Try* method semantics.
There was a problem hiding this comment.
TryX pattern would be even better 👍
There was a problem hiding this comment.
I'm in two minds about using MIME type, thinking about it further. On the one hand, if the response is JSON, then the |Content-Type header should be application/json, but I wonder if there are scenarios where it may not e.g. a proxy in front that doesn't change the header. I'm thinking it may be safer to just try irrespective of Content-Type header and catch the exception.
There was a problem hiding this comment.
Its more about the case where the MIME type informs us the response is not JSON we can omit the TryCreate() all together.
There was a problem hiding this comment.
So I understand correctly, are you suggesting
- if the MIME type is "application/json", call
ServerError.Create();insideBytesResponse/StringResponse - if the MIME type is anything else, use
ServerError.TryCreate()
?
There was a problem hiding this comment.
- Call trycreate
- Omit all together? A bad proxy could break this but a bad proxy is broken anyways.
There was a problem hiding this comment.
Have now added that check. Would you mind taking another look?
|
backported to 6.x in 42414ec |
|
does not need to be ported to 5.x as 5.x has a different implementation |
This commit adds a try/catch block around attempting to deserialize a ServerError
from the body, since IElasticsearchResponse types withn Elasticsearch.Net
set/attempt to set the response body, irrespective of the response status code.
Move BytesResponseTests and StringResponseTests from Tests.Reproduce into
main Tests project.
Allow byte array to be passed to FixedResponseClient.
Fixes #3378