Add UpdateRequest support to High Level Rest client #23266
Conversation
| XContentType xContentType = null; | ||
| if (updateRequest.doc() != null) { | ||
| xContentType = updateRequest.doc().getContentType(); | ||
| } else if (updateRequest.upsertRequest() != null) { |
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.
|
|
||
| BytesRef requestBody = builder.bytes().toBytesRef(); | ||
| ContentType contentType = ContentType.create(xContentType.mediaType()); | ||
| return new ByteArrayEntity(requestBody.bytes, requestBody.offset, requestBody.length, contentType); |
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.
| ContentType contentType = ContentType.create(xContentType.mediaType()); | ||
| return new ByteArrayEntity(requestBody.bytes, requestBody.offset, requestBody.length, contentType); | ||
| } catch (IOException e) { | ||
| throw new IllegalStateException("Failed to build HTTP entity from update request", e); |
tlrx
force-pushed
the
add-update-request-to-high-level-rest-client
branch
from
a5c3424 to
0edc183
Compare
| @@ -118,6 +122,48 @@ static Request index(IndexRequest indexRequest) { | |||
| 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.