Add UpdateRequest support to High Level Rest client#23266
Add UpdateRequest support to High Level Rest client#23266tlrx merged 4 commits intoelastic:masterfrom
Conversation
javanna
left a comment
There was a problem hiding this comment.
left a couple of comments, looks good though
There was a problem hiding this comment.
can the upsert document only be present when no partial document is provided? I think the confusing part is that we have content type coming from both, but the header is one :) maybe we just need to introduce validation that they come in the same content-type, thoughts?
There was a problem hiding this comment.
can the upsert document only be present when no partial document is provided?
Yes
maybe we just need to introduce validation that they come in the same content-type, thoughts?
Yes, I'll do that in a separate PR with the ToXContent method.
There was a problem hiding this comment.
should UpdateRequest implement ToXContent instead? we will find this in other requests too (e.g. bulk), we should decide where to put this code for all those.
There was a problem hiding this comment.
why wrapping the exception?
a5c3424 to
0edc183
Compare
javanna
left a comment
There was a problem hiding this comment.
left a couple of comments.
| return new Request(method, endpoint, parameters.getParams(), entity); | ||
| } | ||
|
|
||
| static Request update(UpdateRequest updateRequest) throws IOException { |
There was a problem hiding this comment.
wondering if we should rather catch IOException and rethrow UncheckedIOException. getting tired of all these checked exceptions, which force us to move to CheckedFunction.
There was a problem hiding this comment.
Well, I'm on the fence on this. We already use CheckedFunction for response parsers I think we could also use CheckedFunction for request converters? I don't feel like it adds a lot of boling code.
| if (updateRequest.upsertRequest() != null) { | ||
| XContentType upsertContentType = updateRequest.upsertRequest().getContentType(); | ||
| if ((xContentType != null) && (xContentType != upsertContentType)) { | ||
| throw new IllegalStateException("Update request cannot have different content types for doc [" + xContentType + "]" + |
There was a problem hiding this comment.
I think that the assumption up until now was that these methods wouldn't throw exceptions. if they do, we have to modify the async methods to catch and call the listener. I think users wouldn't expect to have exceptions thrown with async methods, even if this happens synchronously before the request gets sent :)
There was a problem hiding this comment.
if they do, we have to modify the async methods to catch and call the listener.
I think I did it in performRequestAsync() ?
There was a problem hiding this comment.
oh I had missed this. But we only catch IOException, while we can also throw IllegalStateException now?
There was a problem hiding this comment.
Right - it should catch Exception instead (and also ensure that the listener get called anyway because I just notive that performRequestAsync throws an IllegalArgumentException too.
There was a problem hiding this comment.
oh oh, I think those exceptions thrown in RestClient#performRequestAsync are a problem to be handled in the low level client. the bug is there I'm afraid.
There was a problem hiding this comment.
oh right - I didn't notice I was looking at the RestClient itself. I'll take care of changing it
|
|
||
| ResponseListener responseListener = wrapResponseListener(responseConverter, listener, ignores); | ||
| client.performRequestAsync(req.method, req.endpoint, req.params, req.entity, responseListener, headers); | ||
| } catch (Exception e) { |
There was a problem hiding this comment.
I am not sure here, I think for correctness we should only wrap the requestConverter call? the wrapResponseListener and performRequestAsync should really never throw exception. WDYT?
There was a problem hiding this comment.
Yes, that what I did first but then I wrapped everything because I mixed up the performRequestAsync methods.
|
Thanks @javanna |
This pull request adds support for UpdateRequest to the High Level Rest client.