Skip to content
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

Merged
merged 11 commits into from
Sep 9, 2018

Conversation

johnny94
Copy link
Contributor

This adds support for the put stored script API to the high level rest client.

Relates to #27205

Copy link
Contributor

@vladimirdolzhenko vladimirdolzhenko left a 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,
Copy link
Contributor

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 ?

Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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())) {
Copy link
Contributor

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();
Copy link
Contributor

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)

Copy link
Contributor Author

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!

Copy link
Contributor

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>
Copy link
Contributor

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

Copy link
Contributor

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 {
Copy link
Contributor

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

Copy link
Contributor Author

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...

Copy link
Contributor

@vladimirdolzhenko vladimirdolzhenko Jun 15, 2018

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 ?

Copy link
Member

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.

Copy link
Contributor

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

Copy link
Member

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

@johnny94
Copy link
Contributor Author

@vladimirdolzhenko Thanks for your comments! I will make some changes based on your comment.

@elasticmachine
Copy link
Collaborator

Pinging @elastic/es-core-infra

@johnny94 johnny94 force-pushed the hlrc-put-stored-script-api branch from 38a9eae to a02ba46 Compare June 15, 2018 07:56
@johnny94 johnny94 force-pushed the hlrc-put-stored-script-api branch from a02ba46 to f9da579 Compare June 15, 2018 08:33
Copy link
Member

@javanna javanna left a 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,
Copy link
Member

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));
Copy link
Member

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?

Copy link
Contributor Author

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?

Copy link
Member

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);
Copy link
Member

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
Copy link
Member

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
Copy link
Member

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]]
Copy link
Member

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

Copy link
Contributor Author

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?

Copy link
Member

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) {
Copy link
Member

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 {
Copy link
Member

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.

Copy link
Member

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?

Copy link
Contributor Author

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

https://github.com/johnny94/elasticsearch/blob/82a8611fa631d4b4d9d0b25e011a2900c5674443/client/rest-high-level/src/test/java/org/elasticsearch/client/RequestConvertersTests.java#L2092

Is this a proper way to test the code?

Copy link
Member

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.

Copy link
Contributor Author

@johnny94 johnny94 Jun 28, 2018

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.

@vladimirdolzhenko
Copy link
Contributor

@johnny94 would you be so kind as well to merge master and adjust

// TODO: change to HighLevel PutStoredScriptRequest when it will be ready
and
// TODO: change to HighLevel PutStoredScriptRequest when it will be ready
with your HL REST API part ?

@johnny94
Copy link
Contributor Author

@vladimirdolzhenko Of course! Thanks for your contribution!

@johnny94 johnny94 force-pushed the hlrc-put-stored-script-api branch from 2c956ca to a3fa752 Compare June 19, 2018 14:30
@johnny94 johnny94 force-pushed the hlrc-put-stored-script-api branch from 7e84a14 to 16d5449 Compare June 19, 2018 17:18
@johnny94 johnny94 force-pushed the hlrc-put-stored-script-api branch from 16d5449 to dd3bd38 Compare June 20, 2018 16:52
@johnny94
Copy link
Contributor Author

@javanna
Thanks for your comments, I have fixed some issues. Could you plz have another look into this?

Copy link
Contributor

@vladimirdolzhenko vladimirdolzhenko left a 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())){
Copy link
Contributor

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);
Copy link
Contributor

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));
Copy link
Contributor

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));
Copy link
Contributor

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);
Copy link
Contributor

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
Copy link
Contributor

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

Copy link
Contributor Author

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.

@javanna
Copy link
Member

javanna commented Jun 27, 2018

test this please

Copy link
Contributor

@vladimirdolzhenko vladimirdolzhenko left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@johnny94 thanks, it looks good - agree with @javanna - PutStoredScriptRequestTests has to be added - you already did PutStoredScriptResponseTests - but it is quite simple one, you can have a look to another existed tests

@@ -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
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


/**
* 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
Copy link
Contributor

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

@johnny94
Copy link
Contributor Author

johnny94 commented Jul 5, 2018

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.

Copy link
Contributor

@vladimirdolzhenko vladimirdolzhenko left a 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 {
Copy link
Contributor

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

Copy link
Contributor Author

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!

Copy link
Contributor

@vladimirdolzhenko vladimirdolzhenko Jul 10, 2018

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

@johnny94
Copy link
Contributor Author

@javanna Could you plz have a look into this again? If it looks good to you, I will fix conflicts.

Copy link
Contributor

@vladimirdolzhenko vladimirdolzhenko left a 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)

@johnny94
Copy link
Contributor Author

@vladimirdolzhenko Thanks for your comment, but I found that most of the test classes ( XXXRequestTest )in the same package extends ESTestCase. (Also GetStoredScriptRequestTests) Maybe it is okay to keep using ESTestCase for now.

And @javanna I have fixed conflict.

@vladimirdolzhenko
Copy link
Contributor

@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.

@johnny94
Copy link
Contributor Author

@vladimirdolzhenko When I trying to extends AbstractStreamableXContentTestCase I have to override number of methods. I am wondering how to override doParseInstance.

@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 lang and source when creating StoredScriptSource. I think I missed something when I wrote the method. Could you give me a hint about how to create PutStoredScriptRequest by this method?

@vladimirdolzhenko
Copy link
Contributor

vladimirdolzhenko commented Jul 25, 2018

@johnny94 I assume it has to be smth like

@Override
protected PutStoredScriptRequest doParseInstance(XContentParser parser) throws IOException {
  return PutStoredScriptRequest.fromXContent(parser);
}

all tests those extend AbstractStreamableXContentTestCase have similar doParseInstance

Hope it helps you.

@vladimirdolzhenko
Copy link
Contributor

@johnny94 you've already done a lot - have you faced any difficulties with this ticket - can I help you with smth ?

@johnny94
Copy link
Contributor Author

johnny94 commented Sep 6, 2018

@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 AbstractStreamableXContentTestCase correctly?

@vladimirdolzhenko
Copy link
Contributor

sorry for misleading you @johnny94 - PutStoredScriptRequest is not required to be parsable with XContentParser as other parameters are coming outside of xcontent body.

setRandomMasterTimeout(putStoredScriptRequest, expectedParams);
setRandomTimeout(putStoredScriptRequest::timeout, AcknowledgedRequest.DEFAULT_ACK_TIMEOUT, expectedParams);

if(randomBoolean()) {
Copy link
Contributor

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 (

@vladimirdolzhenko
Copy link
Contributor

thanks @johnny94 LGTM

@vladimirdolzhenko vladimirdolzhenko merged commit 9073dbe into elastic:master Sep 9, 2018
vladimirdolzhenko pushed a commit that referenced this pull request Sep 9, 2018
jasontedor added a commit to jasontedor/elasticsearch that referenced this pull request Sep 9, 2018
* 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)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants