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.

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

Mark all scripted field queries as expensive #59658

Merged
merged 2 commits into from
Jul 16, 2020
Merged

Mark all scripted field queries as expensive #59658

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
04b320d
Mark all scripted field queries as expensive
nik9000 Jul 15, 2020
01aa69d
Update exception type
nik9000 Jul 16, 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
17 changes: 17 additions & 0 deletions ...main/java/org/elasticsearch/xpack/runtimefields/mapper/RuntimeKeywordMappedFieldType.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import java.util.Set;

import static java.util.stream.Collectors.toSet;
import static org.elasticsearch.search.SearchService.ALLOW_EXPENSIVE_QUERIES;

public final class RuntimeKeywordMappedFieldType extends MappedFieldType {

Expand Down Expand Up @@ -77,8 +78,17 @@ private StringScriptFieldScript.LeafFactory leafFactory(QueryShardContext contex
return scriptFactory.newFactory(script.getParams(), context.lookup());
}

private void checkAllowExpensiveQueries(QueryShardContext context) {
if (context.allowExpensiveQueries() == false) {
throw new IllegalArgumentException(
"queries cannot be executed against [script] fields while [" + ALLOW_EXPENSIVE_QUERIES.getKey() + "] is set to [false]."
Copy link
Member

Choose a reason for hiding this comment

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

nit: use the constant from the mapper? content type I think it is called

Copy link
Member

Choose a reason for hiding this comment

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

should we rather throw ElasticsearchException also, and why doesn't the context expose a check method instead that accepts for instance a message? :)

Copy link
Member Author

Choose a reason for hiding this comment

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

should we rather throw ElasticsearchException also, and why doesn't the context expose a check method instead that accepts for instance a message? :)

That would probably be a good choice.

I have no idea why we were throwing ElasticsearchException when we were. It feels like this is what IAE is for.

Copy link
Member Author

Choose a reason for hiding this comment

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

I have no idea why we were throwing ElasticsearchException when we were. It feels like this is what IAE is for.

I figured it out - we translate all exceptions thrown when building a query into a QueryShardException which is always a 400.

Copy link
Member Author

Choose a reason for hiding this comment

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

why doesn't the context expose a check method instead that accepts for instance a message? :)

It really is a good idea. I took a look at it and, ironically, testing it is kind of a pain. I'll hold off on it.

Copy link
Member

Choose a reason for hiding this comment

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

shall we open issues for possible follow-up tasks that we don't want to spend time on right now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Let me give it another go this morning!

);
}
}

@Override
public Query existsQuery(QueryShardContext context) {
checkAllowExpensiveQueries(context);
Copy link
Member

Choose a reason for hiding this comment

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

maybe one day we will a base class for runtime mapper field types that does this in one place.

Copy link
Member Author

Choose a reason for hiding this comment

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

probably!

return new StringScriptFieldExistsQuery(script, leafFactory(context), name());
}

Expand All @@ -91,6 +101,7 @@ public Query fuzzyQuery(
boolean transpositions,
QueryShardContext context
) {
checkAllowExpensiveQueries(context);
return StringScriptFieldFuzzyQuery.build(
script,
leafFactory(context),
Expand All @@ -104,6 +115,7 @@ public Query fuzzyQuery(

@Override
public Query prefixQuery(String value, RewriteMethod method, org.elasticsearch.index.query.QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new StringScriptFieldPrefixQuery(script, leafFactory(context), name(), value);
}

Expand All @@ -118,6 +130,7 @@ public Query rangeQuery(
DateMathParser parser,
QueryShardContext context
) {
checkAllowExpensiveQueries(context);
return new StringScriptFieldRangeQuery(
script,
leafFactory(context),
Expand All @@ -131,22 +144,26 @@ public Query rangeQuery(

@Override
public Query regexpQuery(String value, int flags, int maxDeterminizedStates, RewriteMethod method, QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new StringScriptFieldRegexpQuery(script, leafFactory(context), name(), value, flags, maxDeterminizedStates);
}

@Override
public Query termQuery(Object value, QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new StringScriptFieldTermQuery(script, leafFactory(context), name(), BytesRefs.toString(Objects.requireNonNull(value)));
}

@Override
public Query termsQuery(List<?> values, QueryShardContext context) {
checkAllowExpensiveQueries(context);
Set<String> terms = values.stream().map(v -> BytesRefs.toString(Objects.requireNonNull(v))).collect(toSet());
return new StringScriptFieldTermsQuery(script, leafFactory(context), name(), terms);
}

@Override
public Query wildcardQuery(String value, RewriteMethod method, QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new StringScriptFieldWildcardQuery(script, leafFactory(context), name(), value);
}

Expand Down
67 changes: 67 additions & 0 deletions ...java/org/elasticsearch/xpack/runtimefields/mapper/RuntimeKeywordMappedFieldTypeTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -42,6 +42,7 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.List;
import java.util.function.BiConsumer;

import static java.util.Collections.emptyMap;
import static org.hamcrest.Matchers.equalTo;
Expand Down Expand Up @@ -104,6 +105,10 @@ public void testExistsQuery() throws IOException {
}
}

public void testExistsQueryIsExpensive() throws IOException {
checkExpensiveQuery(RuntimeKeywordMappedFieldType::existsQuery);
}

public void testFuzzyQuery() throws IOException {
try (Directory directory = newDirectory(); RandomIndexWriter iw = new RandomIndexWriter(random(), directory)) {
iw.addDocument(List.of(new StoredField("_source", new BytesRef("{\"foo\": \"cat\"}")))); // No edits, matches
Expand All @@ -121,6 +126,19 @@ public void testFuzzyQuery() throws IOException {
}
}

public void testFuzzyQueryIsExpensive() throws IOException {
checkExpensiveQuery(
(ft, ctx) -> ft.fuzzyQuery(
randomAlphaOfLengthBetween(1, 1000),
randomFrom(Fuzziness.AUTO, Fuzziness.ZERO, Fuzziness.ONE, Fuzziness.TWO),
randomInt(),
randomInt(),
randomBoolean(),
ctx
)
);
}

public void testPrefixQuery() throws IOException {
try (Directory directory = newDirectory(); RandomIndexWriter iw = new RandomIndexWriter(random(), directory)) {
iw.addDocument(List.of(new StoredField("_source", new BytesRef("{\"foo\": \"cat\"}"))));
Expand All @@ -133,6 +151,10 @@ public void testPrefixQuery() throws IOException {
}
}

public void testPrefixQueryIsExpensive() throws IOException {
checkExpensiveQuery((ft, ctx) -> ft.prefixQuery(randomAlphaOfLengthBetween(1, 1000), null, ctx));
}

public void testRangeQuery() throws IOException {
try (Directory directory = newDirectory(); RandomIndexWriter iw = new RandomIndexWriter(random(), directory)) {
iw.addDocument(List.of(new StoredField("_source", new BytesRef("{\"foo\": \"cat\"}"))));
Expand All @@ -148,6 +170,21 @@ public void testRangeQuery() throws IOException {
}
}

public void testRangeQueryIsExpensive() throws IOException {
checkExpensiveQuery(
(ft, ctx) -> ft.rangeQuery(
"a" + randomAlphaOfLengthBetween(0, 1000),
"b" + randomAlphaOfLengthBetween(0, 1000),
randomBoolean(),
randomBoolean(),
null,
null,
null,
ctx
)
);
}

public void testRegexpQuery() throws IOException {
try (Directory directory = newDirectory(); RandomIndexWriter iw = new RandomIndexWriter(random(), directory)) {
iw.addDocument(List.of(new StoredField("_source", new BytesRef("{\"foo\": \"cat\"}"))));
Expand All @@ -165,6 +202,10 @@ public void testRegexpQuery() throws IOException {
}
}

public void testRegexpQueryIsExpensive() throws IOException {
checkExpensiveQuery((ft, ctx) -> ft.regexpQuery(randomAlphaOfLengthBetween(1, 1000), randomInt(0xFFFF), randomInt(), null, ctx));
}

public void testTermQuery() throws IOException {
try (Directory directory = newDirectory(); RandomIndexWriter iw = new RandomIndexWriter(random(), directory)) {
iw.addDocument(List.of(new StoredField("_source", new BytesRef("{\"foo\": 1}"))));
Expand All @@ -176,6 +217,10 @@ public void testTermQuery() throws IOException {
}
}

public void testTermQueryIsExpensive() throws IOException {
checkExpensiveQuery((ft, ctx) -> ft.termQuery(randomAlphaOfLengthBetween(1, 1000), ctx));
}

public void testTermsQuery() throws IOException {
try (Directory directory = newDirectory(); RandomIndexWriter iw = new RandomIndexWriter(random(), directory)) {
iw.addDocument(List.of(new StoredField("_source", new BytesRef("{\"foo\": 1}"))));
Expand All @@ -189,6 +234,10 @@ public void testTermsQuery() throws IOException {
}
}

public void testTermsQueryIsExpensive() throws IOException {
checkExpensiveQuery((ft, ctx) -> ft.termsQuery(randomList(100, () -> randomAlphaOfLengthBetween(1, 1000)), ctx));
}

public void testWildcardQuery() throws IOException {
try (Directory directory = newDirectory(); RandomIndexWriter iw = new RandomIndexWriter(random(), directory)) {
iw.addDocument(List.of(new StoredField("_source", new BytesRef("{\"foo\": \"aab\"}"))));
Expand All @@ -200,6 +249,10 @@ public void testWildcardQuery() throws IOException {
}
}

public void testWildcardQueryIsExpensive() throws IOException {
checkExpensiveQuery((ft, ctx) -> ft.wildcardQuery(randomAlphaOfLengthBetween(1, 1000), null, ctx));
}

private RuntimeKeywordMappedFieldType build(String code) throws IOException {
Script script = new Script(code);
PainlessPlugin painlessPlugin = new PainlessPlugin();
Expand All @@ -218,9 +271,23 @@ public <T> List<T> loadExtensions(Class<T> extensionPointType) {
}

private QueryShardContext mockContext() {
return mockContext(true);
}

private QueryShardContext mockContext(boolean allowExpensiveQueries) {
MapperService mapperService = mock(MapperService.class);
QueryShardContext context = mock(QueryShardContext.class);
when(context.allowExpensiveQueries()).thenReturn(allowExpensiveQueries);
when(context.lookup()).thenReturn(new SearchLookup(mapperService, mft -> null));
return context;
}

private void checkExpensiveQuery(BiConsumer<RuntimeKeywordMappedFieldType, QueryShardContext> queryBuilder) throws IOException {
RuntimeKeywordMappedFieldType ft = build("value('cat')");
Exception e = expectThrows(IllegalArgumentException.class, () -> queryBuilder.accept(ft, mockContext(false)));
assertThat(
e.getMessage(),
equalTo("queries cannot be executed against [script] fields while [search.allow_expensive_queries] is set to [false].")
);
}
}