-
Notifications
You must be signed in to change notification settings - Fork 25k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add put stored script support to high-level rest client #31323
Add put stored script support to high-level rest client #31323
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you @johnny94 so much for you code - I really depend on it (for get script
and delete script
parts). I left couple comments for structural improvement.
* @return the response | ||
* @throws IOException in case there is a problem sending the request or parsing back the response | ||
*/ | ||
public PutStoredScriptResponse putStoredScript(PutStoredScriptRequest putStoredScriptRequest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this API call doesn't belong to any category and is described at https://github.com/elastic/elasticsearch/blob/master/rest-api-spec/src/main/resources/rest-api-spec/api/put_script.json - and it looks like it has to go to main RestHighLevelClient
( e.g. in comparison to https://github.com/elastic/elasticsearch/blob/master/rest-api-spec/src/main/resources/rest-api-spec/api/indices.create.json that is for category indices
)
Could you please move those methods to RestHighLevelClient
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also remove the stored part from the method names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should I also remove the stored parts from the class names?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no those have to stay as they are as it's already existing classes that people have been using with the transport client.
PutStoredScriptRequest request = new PutStoredScriptRequest() | ||
.id("script1"); | ||
|
||
try (XContentBuilder builder = XContentBuilder.builder(xContentType.xContent())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be great to encapsulate all that in PutStoredScriptRequest.toXContent(XContentBuilder builder, Params params)
} | ||
|
||
{ | ||
PutStoredScriptRequest request = new PutStoredScriptRequest(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same - pls move it to PutStoredScriptRequest.toXContent(XContentBuilder builder, Params params)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@vladimirdolzhenko Thanks for your advice. But I am curious about that is it necessary to move the code to PutStoredScriptRequest.toXContent(XContentBuilder builder, Params params)
. I found there are lots of test code written like this.
And as far as I know, toXContent
will build a XContentBuilder
object based on the content of a request, and it will be used in RequestConverters
. So I am not really sure how to put this into PutStoredScriptRequest.toXContent
. If you have any idea how to do that, please let me know and I will fix this as soon as possible. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, my fault - I thought there is a method to set source
directly - you've already added PutStoredScriptRequest.toXContent
that creates nested script
object with source itself - so that part is good.
new ActionListener<PutStoredScriptResponse>() { | ||
@Override | ||
public void onResponse(PutStoredScriptResponse response) { | ||
// <1> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'd rather prefer to store response
in some kind of ref and to check it after that
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nevermind - the same style is used in other documentation examples
@@ -1201,4 +1205,31 @@ public void testGetIndexTemplate() throws Exception { | |||
new GetIndexTemplatesRequest().names("the-template-*"), client.indices()::getTemplate, client.indices()::getTemplateAsync)); | |||
assertThat(notFound.status(), equalTo(RestStatus.NOT_FOUND)); | |||
} | |||
|
|||
public void testPutStoredScript() throws Exception { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the same applicable for tests for this REST API call - RestHighLevelClientTests
is better place for it as this call doesn't belong to any category
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe it is a good idea to move putStoredScript
to other place, but I am not really sure whether RestHighLevelClient
is a good place or not.
But according to #27205 it seems that script API should be categorized as Indices API. I am confused...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good point...
in fact in #27205 it is under indices api while rest api spec says it's out of any category...
in my PR #31355 I put them into StoredScriptsIT
- to group them reasonably...
@javanna do you have any objections / concerns / suggestion ?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
RestHighLevelClientTests is a unit test while this is an integ test, adding them to a new StoredScriptsIT sounds good to me. I will move the API to the right category in the meta issue, it should not be under indices, the transport client even exposed it up until now under cluster, but we should follow our rest spec so we are aligned with the other language clients.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
and I assume the same is applicable for StoredScriptsDocumentationIT
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes that sounds good too
@vladimirdolzhenko Thanks for your comments! I will make some changes based on your comment. |
Pinging @elastic/es-core-infra |
38a9eae
to
a02ba46
Compare
a02ba46
to
f9da579
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
left some comments too, thanks @johnny94 !
* @return the response | ||
* @throws IOException in case there is a problem sending the request or parsing back the response | ||
*/ | ||
public PutStoredScriptResponse putStoredScript(PutStoredScriptRequest putStoredScriptRequest, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you also remove the stored part from the method names?
Params params = new Params(request); | ||
params.withTimeout(putStoredScriptRequest.timeout()); | ||
params.withMasterTimeout(putStoredScriptRequest.masterNodeTimeout()); | ||
request.setEntity(createEntity(putStoredScriptRequest, REQUEST_BODY_CONTENT_TYPE)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't we also support a context
parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
sorry for asking that, I do not really understand what does context
mean here. After searching documentation I only found two docs:
https://www.elastic.co/guide/en/elasticsearch/painless/master/painless-execute-api.html
https://www.elastic.co/guide/en/elasticsearch/reference/master/modules-scripting-security.html
where should I put the parameter?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
don't be sorry, I can explain! at REST, we also accept the context
query_string parameter. It is a string and PutStoredScriptRequest already supports it, you just have to read it from the request and set the corresponding parameter so that we pass it through to the REST layer.
|
||
Map<String, String> expectedParams = new HashMap<>(); | ||
setRandomMasterTimeout(putStoredScriptRequest, expectedParams); | ||
setRandomTimeout(putStoredScriptRequest::timeout, AcknowledgedRequest.DEFAULT_ACK_TIMEOUT, expectedParams); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
here we should test the context param too?
include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[put-stored-script-content-mustache] | ||
-------------------------------------------------- | ||
<1> Specify a mustache script and provided as `XContentBuilder` object. | ||
Note that value of source can be direvtly provided as a JSON string |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
directly
include-tagged::{doc-tests}/IndicesClientDocumentationIT.java[put-stored-script-content-painless] | ||
-------------------------------------------------- | ||
<1> Specify a painless script and provided as `XContentBuilder` object. | ||
Note that the builder need to be passed as a `BytesReference` object |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
needs
<1> Specify a mustache script and provided as `XContentBuilder` object. | ||
Note that value of source can be direvtly provided as a JSON string | ||
|
||
[[java-rest-high-put-stored-script-sync]] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that we need to add the Optional parameters section with the 3 optional parameters that are supported
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this mean context
, xContentType
and source
in PutStoredScriptRequest
class?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
more like context, and the two timeouts.
|
||
@Override | ||
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { | ||
if (source == null) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
when is this if exercised? I think that you can make the blank constructor package protected and assume that once toXContent is called the source has already a value. that has the nice effect that we don't modify the state of the request when toXContent is called which is not ideal.
@@ -166,4 +168,16 @@ public String toString() { | |||
(context != null ? ", context [" + context + "]" : "") + | |||
", content [" + source + "]}"; | |||
} | |||
|
|||
@Override | |||
public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
given that you added this method, I would also add a unit test for it. I would probably also move the respective parsing code from RestPutStoredScriptAction here as a fromXContent method so that testing is also easier.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
can you add a unit test for this method please?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was trying to find a better way to add a unit test for this method. After reading some classes I found that those classes which have toXContent
method also have assertToXContentBody
assertion. so I add something like that in RequestConvertersTests.java
Is this a proper way to test the code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that is part of it, but it only tests that the client takes the request body into account and prints it out. It would also be nice to add a test to the server-side code, which verifies the output itself. We usually do this by adding a request test. You can just add a new testToXContent method to PutStoredScriptRequestTests
which tests the xcontent output. Something static (not randomized) is fine for this type of test.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your hint! I added a test for the method.
@johnny94 would you be so kind as well to merge elasticsearch/client/rest-high-level/src/test/java/org/elasticsearch/client/StoredScriptsIT.java Line 53 in 04e4e44
elasticsearch/client/rest-high-level/src/test/java/org/elasticsearch/client/StoredScriptsIT.java Line 79 in 04e4e44
|
@vladimirdolzhenko Of course! Thanks for your contribution! |
2c956ca
to
a3fa752
Compare
7e84a14
to
16d5449
Compare
16d5449
to
dd3bd38
Compare
@javanna |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @johnny94 for the change - a left few minor comments
Params params = new Params(request); | ||
params.withTimeout(putStoredScriptRequest.timeout()); | ||
params.withMasterTimeout(putStoredScriptRequest.masterNodeTimeout()); | ||
if(Strings.hasText(putStoredScriptRequest.context())){ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
minor - spaces between if
and (Strings
, and space between ){
at the end of line
|
||
GetStoredScriptRequest getRequest = new GetStoredScriptRequest("calculate-score"); | ||
PutStoredScriptRequest request = | ||
new PutStoredScriptRequest(id, "search", new BytesArray("{}"), XContentType.JSON, scriptSource); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💯
new PutStoredScriptRequest(id, "search", new BytesArray("{}"), XContentType.JSON, scriptSource); | ||
PutStoredScriptResponse putResponse = execute(request, highLevelClient()::putScript, | ||
highLevelClient()::putScriptAsync); | ||
assertThat(putResponse.isAcknowledged(), equalTo(true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
recently I learnt about assertAcked
- you can easily use it (and inline putResponse
there as well)
new PutStoredScriptRequest(id, "search", new BytesArray("{}"), XContentType.JSON, scriptSource); | ||
PutStoredScriptResponse putResponse = execute(request, highLevelClient()::putScript, | ||
highLevelClient()::putScriptAsync); | ||
assertThat(putResponse.isAcknowledged(), equalTo(true)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same - assertAcked
RestHighLevelClient client = highLevelClient(); | ||
|
||
{ | ||
createIndex("index1", Settings.EMPTY); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
no need to create any index for stored scripts
include-tagged::{doc-tests}/StoredScriptsDocumentationIT.java[put-stored-script-request] | ||
-------------------------------------------------- | ||
<1> The id of the script | ||
<2> The content of the script |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
couple missed optional properties of request - master_timeout
and timeout
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks. I have added this two arguments in Optional arguments
section.
test this please |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -712,6 +714,35 @@ public void deleteScriptAsync(DeleteStoredScriptRequest request, RequestOptions | |||
DeleteStoredScriptResponse::fromXContent, listener, emptySet()); | |||
} | |||
|
|||
/** | |||
* Puts an stored script using the Scripting API. | |||
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/6.2/modules-scripting.html"> Scripting API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
pls refer to current
instead of 6.2
: https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-scripting.html
|
||
/** | ||
* Asynchronously puts an stored script using the Scripting API. | ||
* See <a href="https://www.elastic.co/guide/en/elasticsearch/reference/6.2/modules-scripting.html"> Scripting API |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
same here - pls refer to current
sorry for tagging again @vladimirdolzhenko @javanna Could have a look into this again? If it okay to both of you, I am planning to fix file conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks @johnny94 for the update - you should not apologize. There is a recommendation for the test class improvement.
I see you did change a week ago - and nobody had a look into your changes - sorry.
I'd like to ask you just ping reviewers with a comment and/or assign reviewer for the review again - in this case we do receive notification.
@@ -48,4 +51,30 @@ public void testSerialization() throws IOException { | |||
} | |||
} | |||
} | |||
|
|||
public void testToXContent() throws IOException { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it would be reasonable PutStoredScriptRequestTests
to extend AbstractStreamableXContentTestCase<PutStoredScriptRequest>
- it gives more randomization and general test flow for requests/responses - like you do for PutStoredScriptResponseTests
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for your comment. But I don't know the comment for ping reviewers. Is there any documentation about this? Or could you help me to assign reviewer for the review again... Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johnny94 I meant to leave a comment like hey @reviewer - could you pls have a look
@javanna Could you plz have a look into this again? If it looks good to you, I will fix conflicts. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@johnny94 look good - the last resort is left as I already mentioned - it would be reasonable to extend PutStoredScriptRequestTests (to extend AbstractStreamableXContentTestCase - it gives more randomization and general test flow for requests/responses - like you do for PutStoredScriptResponseTests)
@vladimirdolzhenko Thanks for your comment, but I found that most of the test classes ( XXXRequestTest )in the same package extends And @javanna I have fixed conflict. |
@johnny94 I would like to recommend to have a look for the functionality of tests for other requests - more coverage is definitely better - that's why I think it is worth to extend test. |
@vladimirdolzhenko When I trying to extends AbstractStreamableXContentTestCase I have to override number of methods. I am wondering how to override @Override
protected PutStoredScriptRequest doParseInstance(XContentParser parser) throws IOException {
return new PutStoredScriptRequest("bar", "context", new BytesArray("{}"), XContentType.JSON,
StoredScriptSource.fromXContent(parser, false));
} This the method I wrote, and I got a test failure said that must specify lang for stored script. This is because it is necessary to specify |
@johnny94 I assume it has to be smth like
all tests those extend Hope it helps you. |
@johnny94 you've already done a lot - have you faced any difficulties with this ticket - can I help you with smth ? |
@vladimirdolzhenko Thanks for your kind message. I found it is not just to simply extend test class then all things work well. Do you have any idea about how to extends |
sorry for misleading you @johnny94 - PutStoredScriptRequest is not required to be parsable with |
setRandomMasterTimeout(putStoredScriptRequest, expectedParams); | ||
setRandomTimeout(putStoredScriptRequest::timeout, AcknowledgedRequest.DEFAULT_ACK_TIMEOUT, expectedParams); | ||
|
||
if(randomBoolean()) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: space between if
and (
thanks @johnny94 LGTM |
* master: CORE: Make Pattern Exclusion Work with Aliases (elastic#33518) Reverse logic for CCR license checks (elastic#33549) Add latch countdown on failure in CCR license tests (elastic#33548) HLRC: Add put stored script support to high-level rest client (elastic#31323) Create temporary directory if needed in CCR test Add license checks for auto-follow implementation (elastic#33496) Bootstrap a new history_uuid when force allocating a stale primary (elastic#33432) INGEST: Remove Outdated TODOs (elastic#33458) Logging: Clean up skipping test Logging: Skip test if it'd fail CRUD: AwaitsFix entire wait_for_refresh close test Painless: Add Imported Static Method (elastic#33440)
This adds support for the put stored script API to the high level rest client.
Relates to #27205