Skip to content

Commit

Permalink
Deprecate accepting malformed requests in stored script API (#28939)
Browse files Browse the repository at this point in the history 
The stored scripts API today accepts malformed requests instead of throwing an exception.
This PR deprecates accepting malformed put stored script requests (requests not using the official script format).

Relates to #27612
  • Loading branch information
@sohaibiftikhar @martijnvg
sohaibiftikhar authored and martijnvg committed May 29, 2018
1 parent de43ca7 commit 30a85cc
Show file tree
Hide file tree
Showing 5 changed files with 59 additions and 25 deletions.
4 changes: 4 additions & 0 deletions modules/lang-mustache/src/test/java/org/elasticsearch/script/mustache/SearchTemplateIT.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,7 @@ public void testIndexedTemplateClient() throws Exception {

getResponse = client().admin().cluster().prepareGetStoredScript("testTemplate").get();
assertNull(getResponse.getSource());
assertWarnings("the template context is now deprecated. Specify templates in a \"script\" element.");
}

public void testIndexedTemplate() throws Exception {
Expand Down Expand Up @@ -267,6 +268,7 @@ public void testIndexedTemplate() throws Exception {
.setScript("2").setScriptType(ScriptType.STORED).setScriptParams(templateParams)
.get();
assertHitCount(searchResponse.getResponse(), 1);
assertWarnings("the template context is now deprecated. Specify templates in a \"script\" element.");
}

// Relates to #10397
Expand Down Expand Up @@ -311,6 +313,7 @@ public void testIndexedTemplateOverwrite() throws Exception {
.get();
assertHitCount(searchResponse.getResponse(), 1);
}
assertWarnings("the template context is now deprecated. Specify templates in a \"script\" element.");
}

public void testIndexedTemplateWithArray() throws Exception {
Expand Down Expand Up @@ -339,6 +342,7 @@ public void testIndexedTemplateWithArray() throws Exception {
.setScript("4").setScriptType(ScriptType.STORED).setScriptParams(arrayTemplateParams)
.get();
assertHitCount(searchResponse.getResponse(), 5);
assertWarnings("the template context is now deprecated. Specify templates in a \"script\" element.");
}

}
61 changes: 38 additions & 23 deletions server/src/main/java/org/elasticsearch/script/StoredScriptSource.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,11 @@ public class StoredScriptSource extends AbstractDiffable<StoredScriptSource> imp
*/
public static final ParseField TEMPLATE_PARSE_FIELD = new ParseField("template");

/**
* Standard {@link ParseField} for query on the inner field.
*/
public static final ParseField TEMPLATE_NO_WRAPPER_PARSE_FIELD = new ParseField("query");

/**
* Standard {@link ParseField} for lang on the inner level.
*/
Expand Down Expand Up @@ -189,6 +194,26 @@ private StoredScriptSource build(boolean ignoreEmpty) {
PARSER.declareField(Builder::setOptions, XContentParser::mapStrings, OPTIONS_PARSE_FIELD, ValueType.OBJECT);
}

private static StoredScriptSource parseRemaining(Token token, XContentParser parser) throws IOException {
try (XContentBuilder builder = XContentFactory.jsonBuilder()) {
if (token != Token.START_OBJECT) {
builder.startObject();
builder.copyCurrentStructure(parser);
builder.endObject();
} else {
builder.copyCurrentStructure(parser);
}

String source = Strings.toString(builder);

if (source == null || source.isEmpty()) {
DEPRECATION_LOGGER.deprecated("empty templates should no longer be used");
}

return new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, source, Collections.emptyMap());
}
}

/**
* This will parse XContent into a {@link StoredScriptSource}. The following formats can be parsed:
*
Expand Down Expand Up @@ -304,38 +329,28 @@ public static StoredScriptSource parse(BytesReference content, XContentType xCon
} else {
throw new ParsingException(parser.getTokenLocation(), "unexpected token [" + token + "], expected [{, <source>]");
}
} else {
if (TEMPLATE_PARSE_FIELD.getPreferredName().equals(name)) {
token = parser.nextToken();

if (token == Token.VALUE_STRING) {
String source = parser.text();

if (source == null || source.isEmpty()) {
DEPRECATION_LOGGER.deprecated("empty templates should no longer be used");
}

return new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, source, Collections.emptyMap());
}
}
} else if (TEMPLATE_PARSE_FIELD.getPreferredName().equals(name)) {

try (XContentBuilder builder = XContentFactory.jsonBuilder()) {
if (token != Token.START_OBJECT) {
builder.startObject();
builder.copyCurrentStructure(parser);
builder.endObject();
} else {
builder.copyCurrentStructure(parser);
}
DEPRECATION_LOGGER.deprecated("the template context is now deprecated. Specify templates in a \"script\" element.");

String source = Strings.toString(builder);
token = parser.nextToken();
if (token == Token.VALUE_STRING) {
String source = parser.text();

if (source == null || source.isEmpty()) {
DEPRECATION_LOGGER.deprecated("empty templates should no longer be used");
}

return new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, source, Collections.emptyMap());
} else {
return parseRemaining(token, parser);
}
} else if (TEMPLATE_NO_WRAPPER_PARSE_FIELD.getPreferredName().equals(name)) {
DEPRECATION_LOGGER.deprecated("the template context is now deprecated. Specify templates in a \"script\" element.");
return parseRemaining(token, parser);
} else {
DEPRECATION_LOGGER.deprecated("scripts should not be stored without a context. Specify them in a \"script\" element.");
return parseRemaining(token, parser);
}
} catch (IOException ioe) {
throw new UncheckedIOException(ioe);
Expand Down
7 changes: 7 additions & 0 deletions server/src/test/java/org/elasticsearch/script/ScriptMetaDataTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -81,10 +81,12 @@ public void testGetScript() throws Exception {
XContentBuilder sourceBuilder = XContentFactory.jsonBuilder();
sourceBuilder.startObject().startObject("template").field("field", "value").endObject().endObject();
builder.storeScript("template", StoredScriptSource.parse(BytesReference.bytes(sourceBuilder), sourceBuilder.contentType()));
assertWarnings("the template context is now deprecated. Specify templates in a \"script\" element.");

sourceBuilder = XContentFactory.jsonBuilder();
sourceBuilder.startObject().field("template", "value").endObject();
builder.storeScript("template_field", StoredScriptSource.parse(BytesReference.bytes(sourceBuilder), sourceBuilder.contentType()));
assertWarnings("the template context is now deprecated. Specify templates in a \"script\" element.");

sourceBuilder = XContentFactory.jsonBuilder();
sourceBuilder.startObject().startObject("script").field("lang", "_lang").field("source", "_source").endObject().endObject();
Expand All @@ -99,14 +101,19 @@ public void testGetScript() throws Exception {
public void testDiff() throws Exception {
ScriptMetaData.Builder builder = new ScriptMetaData.Builder(null);
builder.storeScript("1", StoredScriptSource.parse(new BytesArray("{\"foo\":\"abc\"}"), XContentType.JSON));
assertWarnings("scripts should not be stored without a context. Specify them in a \"script\" element.");
builder.storeScript("2", StoredScriptSource.parse(new BytesArray("{\"foo\":\"def\"}"), XContentType.JSON));
assertWarnings("scripts should not be stored without a context. Specify them in a \"script\" element.");
builder.storeScript("3", StoredScriptSource.parse(new BytesArray("{\"foo\":\"ghi\"}"), XContentType.JSON));
assertWarnings("scripts should not be stored without a context. Specify them in a \"script\" element.");
ScriptMetaData scriptMetaData1 = builder.build();

builder = new ScriptMetaData.Builder(scriptMetaData1);
builder.storeScript("2", StoredScriptSource.parse(new BytesArray("{\"foo\":\"changed\"}"), XContentType.JSON));
assertWarnings("scripts should not be stored without a context. Specify them in a \"script\" element.");
builder.deleteScript("3");
builder.storeScript("4", StoredScriptSource.parse(new BytesArray("{\"foo\":\"jkl\"}"), XContentType.JSON));
assertWarnings("scripts should not be stored without a context. Specify them in a \"script\" element.");
ScriptMetaData scriptMetaData2 = builder.build();

ScriptMetaData.ScriptMetadataDiff diff = (ScriptMetaData.ScriptMetadataDiff) scriptMetaData2.diff(scriptMetaData1);
Expand Down
4 changes: 3 additions & 1 deletion server/src/test/java/org/elasticsearch/script/StoredScriptSourceTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,9 @@ protected StoredScriptSource createTestInstance() {
if (randomBoolean()) {
options.put(Script.CONTENT_TYPE_OPTION, xContentType.mediaType());
}
return StoredScriptSource.parse(BytesReference.bytes(template), xContentType);
StoredScriptSource source = StoredScriptSource.parse(BytesReference.bytes(template), xContentType);
assertWarnings("the template context is now deprecated. Specify templates in a \"script\" element.");
return source;
} catch (IOException e) {
throw new AssertionError("Failed to create test instance", e);
}
Expand Down
8 changes: 7 additions & 1 deletion server/src/test/java/org/elasticsearch/script/StoredScriptTests.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ public void testSourceParsing() throws Exception {
StoredScriptSource source = new StoredScriptSource("mustache", "code", Collections.emptyMap());

assertThat(parsed, equalTo(source));
assertWarnings("the template context is now deprecated. Specify templates in a \"script\" element.");
}

// complex template with wrapper template object
Expand All @@ -89,6 +90,7 @@ public void testSourceParsing() throws Exception {
StoredScriptSource source = new StoredScriptSource("mustache", code, Collections.emptyMap());

assertThat(parsed, equalTo(source));
assertWarnings("the template context is now deprecated. Specify templates in a \"script\" element.");
}

// complex template with no wrapper object
Expand All @@ -104,6 +106,7 @@ public void testSourceParsing() throws Exception {
StoredScriptSource source = new StoredScriptSource("mustache", code, Collections.emptyMap());

assertThat(parsed, equalTo(source));
assertWarnings("the template context is now deprecated. Specify templates in a \"script\" element.");
}

// complex template using script as the field name
Expand Down Expand Up @@ -223,7 +226,10 @@ public void testEmptyTemplateDeprecations() throws IOException {
StoredScriptSource source = new StoredScriptSource(Script.DEFAULT_TEMPLATE_LANG, "", Collections.emptyMap());

assertThat(parsed, equalTo(source));
assertWarnings("empty templates should no longer be used");
assertWarnings(
"the template context is now deprecated. Specify templates in a \"script\" element.",
"empty templates should no longer be used"
);
}

try (XContentBuilder builder = XContentFactory.contentBuilder(XContentType.JSON)) {
Expand Down

0 comments on commit 30a85cc

Please sign in to comment.