Skip to content

Commit

Permalink
Consolidate script parsing from object (#59507)
Browse files Browse the repository at this point in the history 
The update by query action parses a script from an object (map or string). We will need to do the same for runtime fields as they are parsed as part of mappings (#59391).

This commit moves the existing parsing of a script from an object from RestUpdateByQueryAction to the Script class. It also adds tests and adjusts some error messages that are incorrect. Also, options were not parsed before and they are now. And unsupported fields are no longer silently ignored.
  • Loading branch information
@javanna
javanna authored Jul 14, 2020
1 parent b2974ae commit cc31035
Show file tree
Hide file tree
Showing 3 changed files with 194 additions and 71 deletions.
72 changes: 1 addition & 71 deletions modules/reindex/src/main/java/org/elasticsearch/index/reindex/RestUpdateByQueryAction.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,23 +19,17 @@

package org.elasticsearch.index.reindex;

import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.client.node.NodeClient;
import org.elasticsearch.common.xcontent.LoggingDeprecationHandler;
import org.elasticsearch.rest.RestRequest;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptType;

import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Iterator;
import java.util.List;
import java.util.Map;
import java.util.function.Consumer;

import static org.elasticsearch.rest.RestRequest.Method.POST;
import static org.elasticsearch.script.Script.DEFAULT_SCRIPT_LANG;

public class RestUpdateByQueryAction extends AbstractBulkByQueryRestHandler<UpdateByQueryRequest, UpdateByQueryAction> {

Expand Down Expand Up @@ -69,76 +63,12 @@ protected UpdateByQueryRequest buildRequest(RestRequest request) throws IOExcept

Map<String, Consumer<Object>> consumers = new HashMap<>();
consumers.put("conflicts", o -> internal.setConflicts((String) o));
consumers.put("script", o -> internal.setScript(parseScript(o)));
consumers.put("script", o -> internal.setScript(Script.parse(o)));
consumers.put("max_docs", s -> setMaxDocsValidateIdentical(internal, ((Number) s).intValue()));

parseInternalRequest(internal, request, consumers);

internal.setPipeline(request.param("pipeline"));
return internal;
}

@SuppressWarnings("unchecked")
private static Script parseScript(Object config) {
assert config != null : "Script should not be null";

if (config instanceof String) {
return new Script((String) config);
} else if (config instanceof Map) {
Map<String,Object> configMap = (Map<String, Object>) config;
String script = null;
ScriptType type = null;
String lang = null;
Map<String, Object> params = Collections.emptyMap();
for (Iterator<Map.Entry<String, Object>> itr = configMap.entrySet().iterator(); itr.hasNext();) {
Map.Entry<String, Object> entry = itr.next();
String parameterName = entry.getKey();
Object parameterValue = entry.getValue();
if (Script.LANG_PARSE_FIELD.match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
if (parameterValue instanceof String || parameterValue == null) {
lang = (String) parameterValue;
} else {
throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
}
} else if (Script.PARAMS_PARSE_FIELD.match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
if (parameterValue instanceof Map || parameterValue == null) {
params = (Map<String, Object>) parameterValue;
} else {
throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
}
} else if (ScriptType.INLINE.getParseField().match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
if (parameterValue instanceof String || parameterValue == null) {
script = (String) parameterValue;
type = ScriptType.INLINE;
} else {
throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
}
} else if (ScriptType.STORED.getParseField().match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
if (parameterValue instanceof String || parameterValue == null) {
script = (String) parameterValue;
type = ScriptType.STORED;
} else {
throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
}
}
}
if (script == null) {
throw new ElasticsearchParseException("expected one of [{}] or [{}] fields, but found none",
ScriptType.INLINE.getParseField().getPreferredName(), ScriptType.STORED.getParseField().getPreferredName());
}
assert type != null : "if script is not null, type should definitely not be null";

if (type == ScriptType.STORED) {
if (lang != null) {
throw new IllegalArgumentException("lang cannot be specified for stored scripts");
}

return new Script(type, null, script, null, params);
} else {
return new Script(type, lang == null ? DEFAULT_SCRIPT_LANG : lang, script, params);
}
} else {
throw new IllegalArgumentException("Script value should be a String or a Map");
}
}
}
79 changes: 79 additions & 0 deletions server/src/main/java/org/elasticsearch/script/Script.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.elasticsearch.script;

import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesArray;
Expand Down Expand Up @@ -405,6 +406,84 @@ public static Script parse(XContentParser parser, String defaultLang) throws IOE
return PARSER.apply(parser, null).build(defaultLang);
}

/**
* Parse a {@link Script} from an {@link Object}, that can either be a {@link String} or a {@link Map}.
* @see #parse(XContentParser, String)
* @param config The object to parse the script from.
* @return The parsed {@link Script}.
*/
@SuppressWarnings("unchecked")
public static Script parse(Object config) {
Objects.requireNonNull(config, "Script must not be null");
if (config instanceof String) {
return new Script((String) config);
} else if (config instanceof Map) {
Map<String,Object> configMap = (Map<String, Object>) config;
String script = null;
ScriptType type = null;
String lang = null;
Map<String, Object> params = Collections.emptyMap();
Map<String, String> options = Collections.emptyMap();
for (Map.Entry<String, Object> entry : configMap.entrySet()) {
String parameterName = entry.getKey();
Object parameterValue = entry.getValue();
if (Script.LANG_PARSE_FIELD.match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
if (parameterValue instanceof String || parameterValue == null) {
lang = (String) parameterValue;
} else {
throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
}
} else if (Script.PARAMS_PARSE_FIELD.match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
if (parameterValue instanceof Map || parameterValue == null) {
params = (Map<String, Object>) parameterValue;
} else {
throw new ElasticsearchParseException("Value must be of type Map: [" + parameterName + "]");
}
} else if (Script.OPTIONS_PARSE_FIELD.match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
if (parameterValue instanceof Map || parameterValue == null) {
options = (Map<String, String>) parameterValue;
} else {
throw new ElasticsearchParseException("Value must be of type Map: [" + parameterName + "]");
}
} else if (ScriptType.INLINE.getParseField().match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
if (parameterValue instanceof String || parameterValue == null) {
script = (String) parameterValue;
type = ScriptType.INLINE;
} else {
throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
}
} else if (ScriptType.STORED.getParseField().match(parameterName, LoggingDeprecationHandler.INSTANCE)) {
if (parameterValue instanceof String || parameterValue == null) {
script = (String) parameterValue;
type = ScriptType.STORED;
} else {
throw new ElasticsearchParseException("Value must be of type String: [" + parameterName + "]");
}
} else {
throw new ElasticsearchParseException("Unsupported field [" + parameterName + "]");
}
}
if (script == null) {
throw new ElasticsearchParseException("Expected one of [{}] or [{}] fields, but found none",
ScriptType.INLINE.getParseField().getPreferredName(), ScriptType.STORED.getParseField().getPreferredName());
}
assert type != null : "if script is not null, type should definitely not be null";

if (type == ScriptType.STORED) {
if (lang != null) {
throw new IllegalArgumentException("[" + Script.LANG_PARSE_FIELD.getPreferredName() +
"] cannot be specified for stored scripts");
}

return new Script(type, null, script, null, params);
} else {
return new Script(type, lang == null ? DEFAULT_SCRIPT_LANG : lang, script, options, params);
}
} else {
throw new IllegalArgumentException("Script value should be a String or a Map");
}
}

private final ScriptType type;
private final String lang;
private final String idOrCode;
Expand Down
114 changes: 114 additions & 0 deletions server/src/test/java/org/elasticsearch/script/ScriptTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -19,13 +19,15 @@

package org.elasticsearch.script;

import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.io.stream.InputStreamStreamInput;
import org.elasticsearch.common.io.stream.OutputStreamStreamOutput;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.xcontent.ToXContent;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentFactory;
import org.elasticsearch.common.xcontent.XContentHelper;
import org.elasticsearch.common.xcontent.XContentParser;
import org.elasticsearch.common.xcontent.XContentType;
import org.elasticsearch.test.ESTestCase;
Expand All @@ -34,6 +36,7 @@
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -96,4 +99,115 @@ public void testParse() throws IOException {
}
}
}

public void testParseFromObjectShortSyntax() {
Script script = Script.parse("doc['my_field']");
assertEquals(Script.DEFAULT_SCRIPT_LANG, script.getLang());
assertEquals("doc['my_field']", script.getIdOrCode());
assertEquals(0, script.getParams().size());
assertEquals(0, script.getOptions().size());
assertEquals(ScriptType.INLINE, script.getType());
}

public void testParseFromObject() {
Map<String, Object> map = new HashMap<>();
map.put("source", "doc['my_field']");
Map<String, Object> params = new HashMap<>();
int numParams = randomIntBetween(0, 3);
for (int i = 0; i < numParams; i++) {
params.put("param" + i, i);
}
map.put("params", params);
Map<String, String> options = new HashMap<>();
int numOptions = randomIntBetween(0, 3);
for (int i = 0; i < numOptions; i++) {
options.put("option" + i, Integer.toString(i));
}
map.put("options", options);
String lang = Script.DEFAULT_SCRIPT_LANG;;
if (randomBoolean()) {
map.put("lang", lang);
} else if(randomBoolean()) {
lang = "expression";
map.put("lang", lang);
}

Script script = Script.parse(map);
assertEquals(lang, script.getLang());
assertEquals("doc['my_field']", script.getIdOrCode());
assertEquals(ScriptType.INLINE, script.getType());
assertEquals(params, script.getParams());
assertEquals(options, script.getOptions());
}

public void testParseFromObjectFromScript() {
Map<String, Object> params = new HashMap<>();
int numParams = randomIntBetween(0, 3);
for (int i = 0; i < numParams; i++) {
params.put("param" + i, i);
}
Map<String, String> options = new HashMap<>();
int numOptions = randomIntBetween(0, 3);
for (int i = 0; i < numOptions; i++) {
options.put("option" + i, Integer.toString(i));
}
Script script = new Script(ScriptType.INLINE, Script.DEFAULT_SCRIPT_LANG, "doc['field']", options, params);
Map<String, Object> scriptObject = XContentHelper.convertToMap(XContentType.JSON.xContent(), Strings.toString(script), false);
Script parsedScript = Script.parse(scriptObject);
assertEquals(script, parsedScript);
}

public void testParseFromObjectWrongFormat() {
{
NullPointerException exc = expectThrows(
NullPointerException.class,
() -> Script.parse((Object)null)
);
assertEquals("Script must not be null", exc.getMessage());
}
{
IllegalArgumentException exc = expectThrows(
IllegalArgumentException.class,
() -> Script.parse(3)
);
assertEquals("Script value should be a String or a Map", exc.getMessage());
}
{
ElasticsearchParseException exc = expectThrows(
ElasticsearchParseException.class,
() -> Script.parse(Collections.emptyMap())
);
assertEquals("Expected one of [source] or [id] fields, but found none", exc.getMessage());
}
}

public void testParseFromObjectUnsupportedFields() {
ElasticsearchParseException exc = expectThrows(
ElasticsearchParseException.class,
() -> Script.parse(Map.of("source", "script", "unsupported", "value"))
);
assertEquals("Unsupported field [unsupported]", exc.getMessage());
}

public void testParseFromObjectWrongOptionsFormat() {
Map<String, Object> map = new HashMap<>();
map.put("source", "doc['my_field']");
map.put("options", 3);
ElasticsearchParseException exc = expectThrows(
ElasticsearchParseException.class,
() -> Script.parse(map)
);
assertEquals("Value must be of type Map: [options]", exc.getMessage());
}

public void testParseFromObjectWrongParamsFormat() {
Map<String, Object> map = new HashMap<>();
map.put("source", "doc['my_field']");
map.put("params", 3);
ElasticsearchParseException exc = expectThrows(
ElasticsearchParseException.class,
() -> Script.parse(map)
);
assertEquals("Value must be of type Map: [params]", exc.getMessage());
}
}

0 comments on commit cc31035

Please sign in to comment.