Skip to content

Configurable MIME type for mustache template encoding on set processor #65314

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.

Sign up for GitHub

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

Merged
merged 4 commits into from
Jan 7, 2021
Merged

Configurable MIME type for mustache template encoding on set processor #65314

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
ea64cc8
configurable MIME type for mustache template encoding
danhermann Nov 19, 2020
133bef8
add unit test and DRY up constants
danhermann Nov 24, 2020
fa616b6
Merge branch 'master' into 65115_set_processor_escape_chars
elasticmachine Nov 24, 2020
aefd9dc
checkstyle
danhermann Nov 24, 2020
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
7 changes: 4 additions & 3 deletions modules/ingest-common/src/main/java/org/elasticsearch/ingest/common/SetProcessor.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,7 @@
import org.elasticsearch.ingest.IngestDocument;
import org.elasticsearch.ingest.Processor;
import org.elasticsearch.ingest.ValueSource;
import org.elasticsearch.script.Script;
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.script.TemplateScript;

Expand Down Expand Up @@ -110,10 +111,11 @@ public SetProcessor create(Map<String, Processor.Factory> registry, String proce
String description, Map<String, Object> config) throws Exception {
String field = ConfigurationUtils.readStringProperty(TYPE, processorTag, config, "field");
String copyFrom = ConfigurationUtils.readOptionalStringProperty(TYPE, processorTag, config, "copy_from");
String mimeType = ConfigurationUtils.readMimeTypeProperty(TYPE, processorTag, config, "mime_type", "application/json");
Copy link
Contributor

Choose a reason for hiding this comment

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

are we sure we want mime_type as a name here? I thought mime was replace with media_type
but wouldn't content_type be more consistent with what we have in mustache templating engine?

Copy link
Contributor

@pgomulka pgomulka Jan 7, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

or we could consider to rename the content_type to media_type (in mustache template engine)?

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'm happy to rename to whatever is most appropriate. I went with mime_type because that is what was used in CustomMustacheFactory but I would agree that media_type or possibly content_type would be better.

Copy link
Contributor

Choose a reason for hiding this comment

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

+1 to follow up with a change to media_type to be more correct.

ValueSource valueSource = null;
if (copyFrom == null) {
Object value = ConfigurationUtils.readObject(TYPE, processorTag, config, "value");
valueSource = ValueSource.wrap(value, scriptService);
valueSource = ValueSource.wrap(value, scriptService, Map.of(Script.CONTENT_TYPE_OPTION, mimeType));
} else {
Object value = config.remove("value");
if (value != null) {
Expand All @@ -123,8 +125,7 @@ public SetProcessor create(Map<String, Processor.Factory> registry, String proce
}

boolean overrideEnabled = ConfigurationUtils.readBooleanProperty(TYPE, processorTag, config, "override", true);
TemplateScript.Factory compiledTemplate = ConfigurationUtils.compileTemplate(TYPE, processorTag,
"field", field, scriptService);
TemplateScript.Factory compiledTemplate = ConfigurationUtils.compileTemplate(TYPE, processorTag, "field", field, scriptService);
boolean ignoreEmptyValue = ConfigurationUtils.readBooleanProperty(TYPE, processorTag, config, "ignore_empty_value", false);

return new SetProcessor(
Expand Down
25 changes: 25 additions & 0 deletions ...ingest-common/src/test/java/org/elasticsearch/ingest/common/SetProcessorFactoryTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,15 +21,18 @@

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.ingest.ConfigurationUtils;
import org.elasticsearch.ingest.TestTemplateService;
import org.elasticsearch.test.ESTestCase;
import org.junit.Before;

import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.Map;

import static org.hamcrest.CoreMatchers.equalTo;
import static org.hamcrest.Matchers.containsString;

public class SetProcessorFactoryTests extends ESTestCase {

Expand Down Expand Up @@ -133,4 +136,26 @@ public void testCreateWithCopyFromAndValue() throws Exception {
() -> factory.create(null, processorTag, null, config));
assertThat(exception.getMessage(), equalTo("[copy_from] cannot set both `copy_from` and `value` in the same processor"));
}

public void testMimeType() throws Exception {
// valid mime type
String expectedMimeType = randomFrom(ConfigurationUtils.VALID_MIME_TYPES);
Map<String, Object> config = new HashMap<>();
config.put("field", "field1");
config.put("value", "value1");
config.put("mime_type", expectedMimeType);
String processorTag = randomAlphaOfLength(10);
SetProcessor setProcessor = factory.create(null, processorTag, null, config);
assertThat(setProcessor.getTag(), equalTo(processorTag));

// invalid mime type
expectedMimeType = randomValueOtherThanMany(m -> Arrays.asList(ConfigurationUtils.VALID_MIME_TYPES).contains(m),
() -> randomAlphaOfLengthBetween(5, 9));
final Map<String, Object> config2 = new HashMap<>();
config2.put("field", "field1");
config2.put("value", "value1");
config2.put("mime_type", expectedMimeType);
ElasticsearchException e = expectThrows(ElasticsearchException.class, () -> factory.create(null, processorTag, null, config2));
assertThat(e.getMessage(), containsString("property does not contain a supported MIME type [" + expectedMimeType + "]"));
}
}
25 changes: 25 additions & 0 deletions ...t-with-all-dependencies/src/test/java/org/elasticsearch/ingest/ValueSourceMustacheIT.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

package org.elasticsearch.ingest;

import org.elasticsearch.script.Script;

import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
Expand Down Expand Up @@ -72,4 +74,27 @@ public void testAccessSourceViaTemplate() {
assertThat(ingestDocument.hasField("index"), is(false));
}

public void testWithConfigurableEncoders() {
Map<String, Object> model = new HashMap<>();
model.put("log_line", "10.10.1.1 - - [17/Nov/2020:04:59:43 +0000] \"GET /info HTTP/1.1\" 200 6229 \"-\" \"-\" 2");

// default encoder should be application/json
ValueSource valueSource = ValueSource.wrap("{{log_line}}", scriptService);
Object result = valueSource.copyAndResolve(model);
assertThat(result,
equalTo("10.10.1.1 - - [17/Nov/2020:04:59:43 +0000] \\\"GET /info HTTP/1.1\\\" 200 6229 \\\"-\\\" \\\"-\\\" 2"));

// text/plain encoder
var scriptOptions = Map.of(Script.CONTENT_TYPE_OPTION, "text/plain");
valueSource = ValueSource.wrap("{{log_line}}", scriptService, scriptOptions);
result = valueSource.copyAndResolve(model);
assertThat(result, equalTo("10.10.1.1 - - [17/Nov/2020:04:59:43 +0000] \"GET /info HTTP/1.1\" 200 6229 \"-\" \"-\" 2"));

// application/x-www-form-urlencoded encoder
scriptOptions = Map.of(Script.CONTENT_TYPE_OPTION, "application/x-www-form-urlencoded");
valueSource = ValueSource.wrap("{{log_line}}", scriptService, scriptOptions);
result = valueSource.copyAndResolve(model);
assertThat(result, equalTo("10.10.1.1+-+-+%5B17%2FNov%2F2020%3A04%3A59%3A43+%2B0000%5D+%22GET+%2Finfo+HTTP%2F1.1%22+200" +
"+6229+%22-%22+%22-%22++2"));
}
}
13 changes: 13 additions & 0 deletions server/src/main/java/org/elasticsearch/ingest/ConfigurationUtils.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -49,6 +49,7 @@ public final class ConfigurationUtils {

public static final String TAG_KEY = "tag";
public static final String DESCRIPTION_KEY = "description";
public static final String[] VALID_MIME_TYPES = {"application/json", "text/plain", "application/x-www-form-urlencoded"};
Copy link
Contributor

Choose a reason for hiding this comment

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

do we want the se hardcoded? What if someone specifies application/json; charset=utf-8 ?
Would it be of any help if this bit was unified with what CustomMustacheFactory defines? I made an attempt here https://github.com/elastic/elasticsearch/pull/66857/files#diff-207cacc1e4a5b24109a693c0b63dd762156452bb5984b5ce04499b28e6c191e8R28

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 agree that unifying this set of options would be good. I can do that after you merge that PR. I don't believe that we want to allow the specification of a particular character set for the use case of the ingest processor, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Now that #66857 has been closed in favor of #67677, I believe the only improvement that can be made here is changing content_type to media_type. I'll go ahead and open a PR for that.


private ConfigurationUtils() {
}
Expand Down Expand Up @@ -305,6 +306,18 @@ public static Object readObject(String processorType, String processorTag, Map<S
return value;
}

public static String readMimeTypeProperty(String processorType, String processorTag, Map<String, Object> configuration,
String propertyName, String defaultValue) {
String mimeType = readStringProperty(processorType, processorTag, configuration, propertyName, defaultValue);

if (Arrays.asList(VALID_MIME_TYPES).contains(mimeType) == false) {
throw newConfigurationException(processorType, processorTag, propertyName,
"property does not contain a supported MIME type [" + mimeType + "]");
}

return mimeType;
}

public static ElasticsearchException newConfigurationException(String processorType, String processorTag,
String propertyName, String reason) {
String msg;
Expand Down
11 changes: 7 additions & 4 deletions server/src/main/java/org/elasticsearch/ingest/ValueSource.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@

import java.util.ArrayList;
import java.util.Arrays;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;
Expand All @@ -50,21 +49,25 @@ public interface ValueSource {
Object copyAndResolve(Map<String, Object> model);

static ValueSource wrap(Object value, ScriptService scriptService) {
return wrap(value, scriptService, Map.of());
}

static ValueSource wrap(Object value, ScriptService scriptService, Map<String, String> scriptOptions) {

if (value instanceof Map) {
@SuppressWarnings("unchecked")
Map<Object, Object> mapValue = (Map) value;
Map<ValueSource, ValueSource> valueTypeMap = new HashMap<>(mapValue.size());
for (Map.Entry<Object, Object> entry : mapValue.entrySet()) {
valueTypeMap.put(wrap(entry.getKey(), scriptService), wrap(entry.getValue(), scriptService));
valueTypeMap.put(wrap(entry.getKey(), scriptService, scriptOptions), wrap(entry.getValue(), scriptService, scriptOptions));
}
return new MapValue(valueTypeMap);
} else if (value instanceof List) {
@SuppressWarnings("unchecked")
List<Object> listValue = (List) value;
List<ValueSource> valueSourceList = new ArrayList<>(listValue.size());
for (Object item : listValue) {
valueSourceList.add(wrap(item, scriptService));
valueSourceList.add(wrap(item, scriptService, scriptOptions));
}
return new ListValue(valueSourceList);
} else if (value == null || value instanceof Number || value instanceof Boolean) {
Expand All @@ -76,7 +79,7 @@ static ValueSource wrap(Object value, ScriptService scriptService) {
// installed for use by REST tests. `value` will not be
// modified if templating is not available
if (scriptService.isLangSupported(DEFAULT_TEMPLATE_LANG) && ((String) value).contains("{{")) {
Script script = new Script(ScriptType.INLINE, DEFAULT_TEMPLATE_LANG, (String) value, Collections.emptyMap());
Script script = new Script(ScriptType.INLINE, DEFAULT_TEMPLATE_LANG, (String) value, scriptOptions, Map.of());
return new TemplatedValue(scriptService.compile(script, TemplateScript.CONTEXT));
} else {
return new ObjectValue(value);
Expand Down
32 changes: 32 additions & 0 deletions server/src/test/java/org/elasticsearch/ingest/ConfigurationUtilsTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,7 @@

package org.elasticsearch.ingest;

import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.script.ScriptService;
import org.elasticsearch.script.TemplateScript;
Expand All @@ -32,6 +33,7 @@
import java.util.List;
import java.util.Map;

import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;
import static org.hamcrest.Matchers.is;
Expand Down Expand Up @@ -114,6 +116,36 @@ public void testReadStringOrIntPropertyInvalidType() {
}
}

public void testReadMimeProperty() {
// valid mime type
String expectedMimeType = randomFrom(ConfigurationUtils.VALID_MIME_TYPES);
config.put("mime_type", expectedMimeType);
String readMimeType = ConfigurationUtils.readMimeTypeProperty(null, null, config, "mime_type", "");
assertThat(readMimeType, equalTo(expectedMimeType));

// missing mime type with valid default
expectedMimeType = randomFrom(ConfigurationUtils.VALID_MIME_TYPES);
config.remove("mime_type");
readMimeType = ConfigurationUtils.readMimeTypeProperty(null, null, config, "mime_type", expectedMimeType);
assertThat(readMimeType, equalTo(expectedMimeType));

// invalid mime type
expectedMimeType = randomValueOtherThanMany(m -> Arrays.asList(ConfigurationUtils.VALID_MIME_TYPES).contains(m),
() -> randomAlphaOfLengthBetween(5, 9));
config.put("mime_type", expectedMimeType);
ElasticsearchException e = expectThrows(ElasticsearchException.class,
() -> ConfigurationUtils.readMimeTypeProperty(null, null, config, "mime_type", ""));
assertThat(e.getMessage(), containsString("property does not contain a supported MIME type [" + expectedMimeType + "]"));

// missing mime type with invalid default
final String invalidDefaultMimeType = randomValueOtherThanMany(m -> Arrays.asList(ConfigurationUtils.VALID_MIME_TYPES).contains(m),
() -> randomAlphaOfLengthBetween(5, 9));
config.remove("mime_type");
e = expectThrows(ElasticsearchException.class,
() -> ConfigurationUtils.readMimeTypeProperty(null, null, config, "mime_type", invalidDefaultMimeType));
assertThat(e.getMessage(), containsString("property does not contain a supported MIME type [" + invalidDefaultMimeType + "]"));
}

public void testReadProcessors() throws Exception {
Processor processor = mock(Processor.class);
Map<String, Processor.Factory> registry =
Expand Down