Skip to content
This repository has been archived by the owner on Jun 20, 2023. It is now read-only.

Added action for adding and deleting indexed scripts #213

Merged
merged 12 commits into from
Jun 29, 2015
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions jest-common/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,11 @@
<artifactId>junit</artifactId>
</dependency>

<dependency>
<groupId>org.easytesting</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

The assertions used (contains/null/equals) really fall short of justifying the need for this additional library as we already have junit. Let's stick with junit for now.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@kramer
fest assertions works nicely together with JUnit and Hamcrest.
It offers a readable fluent API and meaningful assertion messages (if tests fails).
May I ask why stick with junit and not extending the possibilities of assertions?

Copy link
Member

Choose a reason for hiding this comment

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

I have nothing against fest I'm sure its as awesome as jest if not more 😄.

Main reason is consistency; having two test classes with a different assert syntax than the rest of the project is just awkward.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it.
I will rework to use JUnit instead of fest.

<artifactId>fest-assert</artifactId>
</dependency>

<dependency>
<groupId>commons-io</groupId>
<artifactId>commons-io</artifactId>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
package io.searchbox.indices.script;

import com.google.gson.JsonElement;
import com.google.gson.JsonObject;
import io.searchbox.action.AbstractAction;
import io.searchbox.action.GenericResultAbstractAction;

import java.io.*;
import java.net.URLEncoder;
import java.nio.charset.Charset;

import static io.searchbox.indices.script.ScriptLanguage.GROOVY;
import static java.nio.charset.Charset.defaultCharset;
Copy link
Member

Choose a reason for hiding this comment

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

I'd use the charset from AbstractAction instead (server default charsets are rarely the expected ones, at least with the other one we have a sane default).


public class CreateIndexedScript extends GenericResultAbstractAction {

private final String scriptName;
private final ScriptLanguage scriptLanguage;

public CreateIndexedScript(Builder builder) {
super(builder);
this.payload = builder.payload;
this.scriptName = builder.scriptName;
this.scriptLanguage = builder.scriptLanguage;
setURI(buildURI());
}

protected String buildURI() {
try {
return super.buildURI() + "/_scripts/" + scriptLanguage.pathParameterName + "/" + URLEncoder.encode(scriptName, CHARSET);
} catch (UnsupportedEncodingException e) {
throw new RuntimeException(e);
}
}

@Override
public String getRestMethodName() {
return "POST";
}

public String getScriptName() {
return scriptName;
}

public ScriptLanguage getScriptLanguage() {
return scriptLanguage;
}

public static class Builder extends AbstractAction.Builder<CreateIndexedScript, Builder> {

private String scriptName;
private JsonElement payload;
private ScriptLanguage scriptLanguage = GROOVY;

public Builder(String scriptName) {
this.scriptName = scriptName;
}

@Override
public CreateIndexedScript build() {
return new CreateIndexedScript(this);
}

public Builder setSource(String source) {
createPayload(source);
return this;
}

public Builder loadSource(File srcFile) 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.

Having public Builder source(String scriptSource) would be more than enough (as it has been in other actions) since we are not concerned with taking responsibility of user's streams or files at this point.

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 understand.
Can we live with this extra functionality?
What do you recommend?

Copy link
Member

Choose a reason for hiding this comment

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

Sure we can keep it. I was worried about all things that can go wrong with stream and file ops but not really a major concern after all.

return loadSource(srcFile, defaultCharset());
}

public Builder loadSource(File srcFile, Charset encoding) throws IOException {
return loadSource(new FileInputStream(srcFile), encoding);
}

public Builder loadSource(InputStream srcStream) throws IOException {
return loadSource(srcStream, defaultCharset());
}

public Builder loadSource(InputStream srcStream, Charset encoding) throws IOException {
String src = readFully(srcStream, encoding);
createPayload(src);
return this;
}

public Builder setLanguage(ScriptLanguage language) {
this.scriptLanguage = language;
return this;
}

private void createPayload(String source) {
JsonObject jsonObject = new JsonObject();
jsonObject.addProperty("script", source);
payload = jsonObject;
}

private String readFully(InputStream srcStream, Charset encoding) 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.

Since we already have guava, we can go with its CharStreams.toString utility.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it, I will rework.

byte[] buf = new byte[8192];
StringBuilder sb = new StringBuilder();
for (int read; (read = srcStream.read(buf)) > 0; ) {
sb.append(new String(buf, 0, read, encoding));
}
return sb.toString();
}
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,64 @@
package io.searchbox.indices.script;

import io.searchbox.action.AbstractAction;
import io.searchbox.action.GenericResultAbstractAction;

import java.io.UnsupportedEncodingException;
import java.net.URLEncoder;

import static io.searchbox.indices.script.ScriptLanguage.GROOVY;

public class DeleteIndexedScript extends GenericResultAbstractAction {

private final String scriptName;
private final ScriptLanguage scriptLanguage;

public DeleteIndexedScript(Builder builder) {
super(builder);
this.scriptName = builder.scriptName;
this.scriptLanguage = builder.scriptLanguage;
setURI(buildURI());
}

protected String buildURI() {
try {
return super.buildURI() + "/_scripts/" + scriptLanguage.pathParameterName + "/" + URLEncoder.encode(scriptName, CHARSET);
} catch (UnsupportedEncodingException e) {
throw new RuntimeException(e);
Copy link
Member

Choose a reason for hiding this comment

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

Different behaviour than super.buildURI (and well actually than all places where UnsupportedEncodingException is thrown)... Please keep it consistent - either change this or all other ones. I'm not sure which way to go as this only expected if the class is extended and charset is overridden.

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 wasn't sure, if its useful to URL encode the scripts name.
I opted for doing so, to be more safe.
Because of your comment, I'm still struggling.
Under the assumption, that users (hopefully) know that a script name should be choosen safely (a-z,0-9 for example), we could remove the encoding part.
This would be consistent with the other code.
The consequences are clear: it will still work for 99% of all cases.
And just in case someone will use special characters (e.g. slash or hash ...),
the action is likely to fail. The user is able to fix this by himself, by changing the name.

Thinking all over that, I tend to remove the encoding now.
What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

we could remove the encoding part.
This would be consistent with the other code.

We are (supposed to be) url-encoding everything user can pass into URL because well handling the HTTP communication related stuff is kinda jest's responsibility (otoh ES server should be the one deciding and replying if it is valid as an index/type/scriptname etc.), if you found a place where we are not doing that please let me know so I can fix it.
My comment was about re-throwing it as RuntimeException; in other cases of UnsupportedEncodingException we just log it as an error and move on.

}
}

@Override
public String getRestMethodName() {
return "DELETE";
}

public String getScriptName() {
return scriptName;
}

public ScriptLanguage getScriptLanguage() {
return scriptLanguage;
}

public static class Builder extends AbstractAction.Builder<DeleteIndexedScript, Builder> {

private String scriptName;
private ScriptLanguage scriptLanguage = GROOVY;

public Builder(String scriptName) {
this.scriptName = scriptName;
}

public Builder setLanguage(ScriptLanguage scriptLanguage) {
this.scriptLanguage = scriptLanguage;
return this;
}

@Override
public DeleteIndexedScript build() {
return new DeleteIndexedScript(this);
}

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,22 @@
package io.searchbox.indices.script;

/**
* As described in Elasticsearch Reference
* <a href="https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-scripting.html">
* https://www.elastic.co/guide/en/elasticsearch/reference/current/modules-scripting.html
* </a>
*/
public enum ScriptLanguage {
GROOVY("groovy"),
EXPRESSION("expression"),
MUSTACHE("mustache"),
MVEL("mvel"),
JAVASCRIPT("javascript"),
PYTHON("python");

public final String pathParameterName;

ScriptLanguage(String pathParameterName) {
this.pathParameterName = pathParameterName;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,86 @@
package io.searchbox.indices.script;

import com.google.gson.Gson;
import com.google.gson.JsonObject;
import com.google.gson.JsonParser;
import org.junit.Before;
import org.junit.Test;

import java.io.File;
import java.io.FileWriter;
import java.io.IOException;

import static io.searchbox.indices.script.ScriptLanguage.GROOVY;
import static io.searchbox.indices.script.ScriptLanguage.JAVASCRIPT;
import static org.fest.assertions.Assertions.assertThat;

public class CreateIndexedScriptTest {
Copy link
Member

Choose a reason for hiding this comment

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

No integration test(s)? :)

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, I overlooked that.
Good point - I will add one - thanks.


private static final String A_NAME = "a_name";
private CreateIndexedScript script;
private CreateIndexedScript.Builder builder;
private String groovysnippet;

@Before
public void setUp() throws Exception {
builder = new CreateIndexedScript.Builder(A_NAME).setLanguage(JAVASCRIPT);
script = builder.build();
groovysnippet = "def test_a=123\n" +
"def test_b=\"$test_a\"\n";
}

@Test
public void defaultScriptingLanguageIsGroovy() throws Exception {
CreateIndexedScript script = new CreateIndexedScript.Builder(A_NAME).build();

assertThat(script.getScriptLanguage()).isEqualTo(GROOVY);
assertThat(script.buildURI()).contains(GROOVY.pathParameterName);
}

@Test
public void scriptingLanguageIsSetIntoPath() throws Exception {

assertThat(script.buildURI()).contains("/_scripts/" + JAVASCRIPT.pathParameterName + "/");
}

@Test
public void nameOfTheScriptIsSetIntoPath() throws Exception {

assertThat(script.buildURI()).contains("/_scripts/" + JAVASCRIPT.pathParameterName + "/" + A_NAME);
}

@Test
public void scriptSourceIsValidJsonString() throws Exception {
builder.setSource(groovysnippet);

script = builder.build();

JsonObject jsonPayload = parseAsGson(script.getData(new Gson()));
assertThat(jsonPayload.get("script")).isNotNull();
assertThat(jsonPayload.get("script").getAsString()).isEqualTo(groovysnippet);
}

@Test
public void fileSourceIsValidJsonString() throws Exception {
builder.loadSource(createTempGroovySnippetFile());

script = builder.build();

JsonObject jsonPayload = parseAsGson(script.getData(new Gson()));
assertThat(jsonPayload.get("script")).isNotNull();
assertThat(jsonPayload.get("script").getAsString()).isEqualTo(groovysnippet);
}

private File createTempGroovySnippetFile() throws IOException {
File file = File.createTempFile("test", ".groovy");
file.deleteOnExit();
FileWriter writer = new FileWriter(file);
writer.write(groovysnippet);
writer.close();
return file;
}

private JsonObject parseAsGson(String data) {
return new JsonParser().parse(data).getAsJsonObject();
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
package io.searchbox.indices.script;

import org.junit.Before;
import org.junit.Test;

import static io.searchbox.indices.script.ScriptLanguage.GROOVY;
import static io.searchbox.indices.script.ScriptLanguage.JAVASCRIPT;
import static org.fest.assertions.Assertions.assertThat;

public class DeleteIndexedScriptTest {

private static final String A_NAME = "a_name";
private DeleteIndexedScript script;

@Before
public void setUp() throws Exception {
DeleteIndexedScript.Builder builder = new DeleteIndexedScript.Builder(A_NAME).setLanguage(JAVASCRIPT);
script = builder.build();
}

@Test
public void defaultScriptingLanguageIsGroovy() throws Exception {
DeleteIndexedScript script = new DeleteIndexedScript.Builder(A_NAME).build();

assertThat(script.getScriptLanguage()).isEqualTo(GROOVY);
assertThat(script.buildURI()).contains(GROOVY.pathParameterName);
}

@Test
public void scriptingLanguageIsSetIntoPath() throws Exception {

assertThat(script.buildURI()).contains("/_scripts/" + JAVASCRIPT.pathParameterName + "/");
}

@Test
public void nameOfTheScriptIsSetIntoPath() throws Exception {

assertThat(script.buildURI()).contains("/_scripts/" + JAVASCRIPT.pathParameterName + "/" + A_NAME);
}

}
7 changes: 7 additions & 0 deletions pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -89,6 +89,7 @@
<commonsLang.version>3.4</commonsLang.version>
<commons-io.version>2.4</commons-io.version>
<junit.version>4.12</junit.version>
<fest-assert.version>1.4</fest-assert.version>

<plugin.surefire.version>2.18.1</plugin.surefire.version>
</properties>
Expand Down Expand Up @@ -255,6 +256,12 @@
<version>${junit.version}</version>
<scope>test</scope>
</dependency>
<dependency>
<groupId>org.easytesting</groupId>
<artifactId>fest-assert</artifactId>
<version>${fest-assert.version}</version>
<scope>test</scope>
</dependency>

<dependency>
<groupId>commons-io</groupId>
Expand Down