Skip to content

Commit

Permalink
Check for runtime field loops in queries (backport of #61927) (#62421)
Browse files Browse the repository at this point in the history 
We were checking for loops in queries before, but we had an "off by one"
error where we wouldn't notice the "top level" runtime field when
detecting a loop. So the error message would be wrong.

I also caught a few bugs with query generation caused by missing
`@Override` annotations and fixed a few of them. There is a bug with
`regexp` queries with match options that I'm not fixing in this PR but
will get to later.

Relates to #59332
  • Loading branch information
@nik9000
nik9000 authored Sep 15, 2020
1 parent 0a7f335 commit e5ad3a4
Show file tree
Hide file tree
Showing 18 changed files with 417 additions and 291 deletions.
44 changes: 40 additions & 4 deletions ...main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,15 @@
import org.apache.lucene.search.spans.SpanMultiTermQueryWrapper;
import org.apache.lucene.search.spans.SpanQuery;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.common.TriFunction;
import org.elasticsearch.common.geo.ShapeRelation;
import org.elasticsearch.common.time.DateMathParser;
import org.elasticsearch.common.unit.Fuzziness;
import org.elasticsearch.index.mapper.MappedFieldType;
import org.elasticsearch.index.mapper.TextSearchInfo;
import org.elasticsearch.index.query.QueryShardContext;
import org.elasticsearch.script.Script;
import org.elasticsearch.search.lookup.SearchLookup;

import java.io.IOException;
import java.time.ZoneId;
Expand All @@ -31,12 +33,19 @@
/**
* Abstract base {@linkplain MappedFieldType} for scripted fields.
*/
abstract class AbstractScriptMappedFieldType extends MappedFieldType {
abstract class AbstractScriptMappedFieldType<LeafFactory> extends MappedFieldType {
protected final Script script;
private final TriFunction<String, Map<String, Object>, SearchLookup, LeafFactory> factory;

AbstractScriptMappedFieldType(String name, Script script, Map<String, String> meta) {
AbstractScriptMappedFieldType(
String name,
Script script,
TriFunction<String, Map<String, Object>, SearchLookup, LeafFactory> factory,
Map<String, String> meta
) {
super(name, false, false, TextSearchInfo.SIMPLE_MATCH_ONLY, meta);
this.script = script;
this.factory = factory;
}

protected abstract String runtimeType();
Expand All @@ -61,6 +70,26 @@ public final boolean isAggregatable() {
return true;
}

/**
* Create a script leaf factory.
*/
protected final LeafFactory leafFactory(SearchLookup searchLookup) {
return factory.apply(name(), script.getParams(), searchLookup);
}

/**
* Create a script leaf factory for queries.
*/
protected final LeafFactory leafFactory(QueryShardContext context) {
/*
* Forking here causes us to count this field in the field data loop
* detection code as though we were resolving field data for this field.
* We're not, but running the query is close enough.
*/
return leafFactory(context.lookup().forkAndTrackFieldReferences(name()));
}

@Override
public abstract Query termsQuery(List<?> values, QueryShardContext context);

@Override
Expand Down Expand Up @@ -149,8 +178,15 @@ public SpanQuery spanPrefixQuery(String value, SpanMultiTermQueryWrapper.SpanRew
}

private String unsupported(String query, String supported) {
String thisField = "[" + name() + "] which is of type [script] with runtime_type [" + runtimeType() + "]";
return "Can only use " + query + " queries on " + supported + " fields - not on " + thisField;
return String.format(
Locale.ROOT,
"Can only use %s queries on %s fields - not on [%s] which is of type [%s] with runtime_type [%s]",
query,
supported,
name(),
RuntimeFieldMapper.CONTENT_TYPE,
runtimeType()
);
}

protected final void checkAllowExpensiveQueries(QueryShardContext context) {
Expand Down
10 changes: 5 additions & 5 deletions ...fields/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/RuntimeFieldMapper.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -51,7 +51,7 @@ public <FactoryType> FactoryType compile(Script script, ScriptContext<FactoryTyp

protected RuntimeFieldMapper(
String simpleName,
AbstractScriptMappedFieldType mappedFieldType,
AbstractScriptMappedFieldType<?> mappedFieldType,
MultiFields multiFields,
CopyTo copyTo,
String runtimeType,
Expand Down Expand Up @@ -86,7 +86,7 @@ protected String contentType() {

public static class Builder extends ParametrizedFieldMapper.Builder {

static final Map<String, BiFunction<Builder, BuilderContext, AbstractScriptMappedFieldType>> FIELD_TYPE_RESOLVER =
static final Map<String, BiFunction<Builder, BuilderContext, AbstractScriptMappedFieldType<?>>> FIELD_TYPE_RESOLVER =
org.elasticsearch.common.collect.Map.of(BooleanFieldMapper.CONTENT_TYPE, (builder, context) -> {
builder.formatAndLocaleNotSupported();
BooleanScriptFieldScript.Factory factory = builder.scriptCompiler.compile(
Expand Down Expand Up @@ -199,7 +199,7 @@ private static RuntimeFieldMapper toType(FieldMapper in) {
private final Parameter<String> format = Parameter.stringParam(
"format",
true,
mapper -> ((AbstractScriptMappedFieldType) mapper.fieldType()).format(),
mapper -> ((AbstractScriptMappedFieldType<?>) mapper.fieldType()).format(),
null
).setSerializer((b, n, v) -> {
if (v != null && false == v.equals(DateFieldMapper.DEFAULT_DATE_TIME_FORMATTER.pattern())) {
Expand All @@ -211,7 +211,7 @@ private static RuntimeFieldMapper toType(FieldMapper in) {
true,
() -> null,
(n, c, o) -> o == null ? null : LocaleUtils.parse(o.toString()),
mapper -> ((AbstractScriptMappedFieldType) mapper.fieldType()).formatLocale()
mapper -> ((AbstractScriptMappedFieldType<?>) mapper.fieldType()).formatLocale()
).setSerializer((b, n, v) -> {
if (v != null && false == v.equals(Locale.ROOT)) {
b.field(n, v.toString());
Expand All @@ -232,7 +232,7 @@ protected List<Parameter<?>> getParameters() {

@Override
public RuntimeFieldMapper build(BuilderContext context) {
BiFunction<Builder, BuilderContext, AbstractScriptMappedFieldType> fieldTypeResolver = Builder.FIELD_TYPE_RESOLVER.get(
BiFunction<Builder, BuilderContext, AbstractScriptMappedFieldType<?>> fieldTypeResolver = Builder.FIELD_TYPE_RESOLVER.get(
runtimeType.getValue()
);
if (fieldTypeResolver == null) {
Expand Down
19 changes: 6 additions & 13 deletions .../main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptBooleanMappedFieldType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -27,12 +27,9 @@
import java.util.Map;
import java.util.function.Supplier;

public class ScriptBooleanMappedFieldType extends AbstractScriptMappedFieldType {
private final BooleanScriptFieldScript.Factory scriptFactory;

public class ScriptBooleanMappedFieldType extends AbstractScriptMappedFieldType<BooleanScriptFieldScript.LeafFactory> {
ScriptBooleanMappedFieldType(String name, Script script, BooleanScriptFieldScript.Factory scriptFactory, Map<String, String> meta) {
super(name, script, meta);
this.scriptFactory = scriptFactory;
super(name, script, scriptFactory::newFactory, meta);
}

@Override
Expand Down Expand Up @@ -71,14 +68,10 @@ public ScriptBooleanFieldData.Builder fielddataBuilder(String fullyQualifiedInde
return new ScriptBooleanFieldData.Builder(name(), leafFactory(searchLookup.get()));
}

private BooleanScriptFieldScript.LeafFactory leafFactory(SearchLookup searchLookup) {
return scriptFactory.newFactory(name(), script.getParams(), searchLookup);
}

@Override
public Query existsQuery(QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new BooleanScriptFieldExistsQuery(script, leafFactory(context.lookup()), name());
return new BooleanScriptFieldExistsQuery(script, leafFactory(context), name());
}

@Override
Expand Down Expand Up @@ -149,7 +142,7 @@ public Query rangeQuery(
@Override
public Query termQuery(Object value, QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new BooleanScriptFieldTermQuery(script, leafFactory(context.lookup()), name(), toBoolean(value));
return new BooleanScriptFieldTermQuery(script, leafFactory(context), name(), toBoolean(value));
}

@Override
Expand All @@ -176,11 +169,11 @@ private Query termsQuery(boolean trueAllowed, boolean falseAllowed, QueryShardCo
return existsQuery(context);
}
checkAllowExpensiveQueries(context);
return new BooleanScriptFieldTermQuery(script, leafFactory(context.lookup()), name(), true);
return new BooleanScriptFieldTermQuery(script, leafFactory(context), name(), true);
}
if (falseAllowed) {
checkAllowExpensiveQueries(context);
return new BooleanScriptFieldTermQuery(script, leafFactory(context.lookup()), name(), false);
return new BooleanScriptFieldTermQuery(script, leafFactory(context), name(), false);
}
return new MatchNoDocsQuery("neither true nor false allowed");
}
Expand Down
20 changes: 7 additions & 13 deletions ...src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptDateMappedFieldType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,8 +37,7 @@
import java.util.Map;
import java.util.function.Supplier;

public class ScriptDateMappedFieldType extends AbstractScriptMappedFieldType {
private final DateScriptFieldScript.Factory scriptFactory;
public class ScriptDateMappedFieldType extends AbstractScriptMappedFieldType<DateScriptFieldScript.LeafFactory> {
private final DateFormatter dateTimeFormatter;

ScriptDateMappedFieldType(
Expand All @@ -48,8 +47,7 @@ public class ScriptDateMappedFieldType extends AbstractScriptMappedFieldType {
DateFormatter dateTimeFormatter,
Map<String, String> meta
) {
super(name, script, meta);
this.scriptFactory = scriptFactory;
super(name, script, (n, params, ctx) -> scriptFactory.newFactory(n, params, ctx, dateTimeFormatter), meta);
this.dateTimeFormatter = dateTimeFormatter;
}

Expand Down Expand Up @@ -84,10 +82,6 @@ public ScriptDateFieldData.Builder fielddataBuilder(String fullyQualifiedIndexNa
return new ScriptDateFieldData.Builder(name(), leafFactory(lookup.get()));
}

private DateScriptFieldScript.LeafFactory leafFactory(SearchLookup lookup) {
return scriptFactory.newFactory(name(), script.getParams(), lookup, dateTimeFormatter);
}

@Override
public Query distanceFeatureQuery(Object origin, String pivot, float boost, QueryShardContext context) {
checkAllowExpensiveQueries(context);
Expand All @@ -103,7 +97,7 @@ public Query distanceFeatureQuery(Object origin, String pivot, float boost, Quer
TimeValue pivotTime = TimeValue.parseTimeValue(pivot, "distance_feature.pivot");
return new LongScriptFieldDistanceFeatureQuery(
script,
leafFactory(context.lookup())::newInstance,
leafFactory(context)::newInstance,
name(),
originLong,
pivotTime.getMillis(),
Expand All @@ -115,7 +109,7 @@ public Query distanceFeatureQuery(Object origin, String pivot, float boost, Quer
@Override
public Query existsQuery(QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new LongScriptFieldExistsQuery(script, leafFactory(context.lookup())::newInstance, name());
return new LongScriptFieldExistsQuery(script, leafFactory(context)::newInstance, name());
}

@Override
Expand All @@ -139,7 +133,7 @@ public Query rangeQuery(
parser,
context,
DateFieldMapper.Resolution.MILLISECONDS,
(l, u) -> new LongScriptFieldRangeQuery(script, leafFactory(context.lookup())::newInstance, name(), l, u)
(l, u) -> new LongScriptFieldRangeQuery(script, leafFactory(context)::newInstance, name(), l, u)
);
}

Expand All @@ -155,7 +149,7 @@ public Query termQuery(Object value, QueryShardContext context) {
DateFieldMapper.Resolution.MILLISECONDS
);
checkAllowExpensiveQueries(context);
return new LongScriptFieldTermQuery(script, leafFactory(context.lookup())::newInstance, name(), l);
return new LongScriptFieldTermQuery(script, leafFactory(context)::newInstance, name(), l);
});
}

Expand All @@ -179,7 +173,7 @@ public Query termsQuery(List<?> values, QueryShardContext context) {
);
}
checkAllowExpensiveQueries(context);
return new LongScriptFieldTermsQuery(script, leafFactory(context.lookup())::newInstance, name(), terms);
return new LongScriptFieldTermsQuery(script, leafFactory(context)::newInstance, name(), terms);
});
}

Expand Down
19 changes: 6 additions & 13 deletions ...c/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptDoubleMappedFieldType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -29,12 +29,9 @@
import java.util.Map;
import java.util.function.Supplier;

public class ScriptDoubleMappedFieldType extends AbstractScriptMappedFieldType {
private final DoubleScriptFieldScript.Factory scriptFactory;

public class ScriptDoubleMappedFieldType extends AbstractScriptMappedFieldType<DoubleScriptFieldScript.LeafFactory> {
ScriptDoubleMappedFieldType(String name, Script script, DoubleScriptFieldScript.Factory scriptFactory, Map<String, String> meta) {
super(name, script, meta);
this.scriptFactory = scriptFactory;
super(name, script, scriptFactory::newFactory, meta);
}

@Override
Expand Down Expand Up @@ -63,14 +60,10 @@ public ScriptDoubleFieldData.Builder fielddataBuilder(String fullyQualifiedIndex
return new ScriptDoubleFieldData.Builder(name(), leafFactory(searchLookup.get()));
}

private DoubleScriptFieldScript.LeafFactory leafFactory(SearchLookup searchLookup) {
return scriptFactory.newFactory(name(), script.getParams(), searchLookup);
}

@Override
public Query existsQuery(QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new DoubleScriptFieldExistsQuery(script, leafFactory(context.lookup()), name());
return new DoubleScriptFieldExistsQuery(script, leafFactory(context), name());
}

@Override
Expand All @@ -89,14 +82,14 @@ public Query rangeQuery(
upperTerm,
includeLower,
includeUpper,
(l, u) -> new DoubleScriptFieldRangeQuery(script, leafFactory(context.lookup()), name(), l, u)
(l, u) -> new DoubleScriptFieldRangeQuery(script, leafFactory(context), name(), l, u)
);
}

@Override
public Query termQuery(Object value, QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new DoubleScriptFieldTermQuery(script, leafFactory(context.lookup()), name(), NumberType.objectToDouble(value));
return new DoubleScriptFieldTermQuery(script, leafFactory(context), name(), NumberType.objectToDouble(value));
}

@Override
Expand All @@ -109,6 +102,6 @@ public Query termsQuery(List<?> values, QueryShardContext context) {
terms.add(Double.doubleToLongBits(NumberType.objectToDouble(value)));
}
checkAllowExpensiveQueries(context);
return new DoubleScriptFieldTermsQuery(script, leafFactory(context.lookup()), name(), terms);
return new DoubleScriptFieldTermsQuery(script, leafFactory(context), name(), terms);
}
}
30 changes: 12 additions & 18 deletions ...s/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptIpMappedFieldType.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,15 +37,9 @@
import java.util.Map;
import java.util.function.Supplier;

public final class ScriptIpMappedFieldType extends AbstractScriptMappedFieldType {

private final Script script;
private final IpScriptFieldScript.Factory scriptFactory;

public final class ScriptIpMappedFieldType extends AbstractScriptMappedFieldType<IpScriptFieldScript.LeafFactory> {
ScriptIpMappedFieldType(String name, Script script, IpScriptFieldScript.Factory scriptFactory, Map<String, String> meta) {
super(name, script, meta);
this.script = script;
this.scriptFactory = scriptFactory;
super(name, script, scriptFactory::newFactory, meta);
}

@Override
Expand Down Expand Up @@ -79,14 +73,10 @@ public ScriptIpFieldData.Builder fielddataBuilder(String fullyQualifiedIndexName
return new ScriptIpFieldData.Builder(name(), leafFactory(searchLookup.get()));
}

private IpScriptFieldScript.LeafFactory leafFactory(SearchLookup searchLookup) {
return scriptFactory.newFactory(name(), script.getParams(), searchLookup);
}

@Override
public Query existsQuery(QueryShardContext context) {
checkAllowExpensiveQueries(context);
return new IpScriptFieldExistsQuery(script, leafFactory(context.lookup()), name());
return new IpScriptFieldExistsQuery(script, leafFactory(context), name());
}

@Override
Expand All @@ -107,7 +97,7 @@ public Query rangeQuery(
includeUpper,
(lower, upper) -> new IpScriptFieldRangeQuery(
script,
leafFactory(context.lookup()),
leafFactory(context),
name(),
new BytesRef(InetAddressPoint.encode(lower)),
new BytesRef(InetAddressPoint.encode(upper))
Expand All @@ -119,14 +109,18 @@ public Query rangeQuery(
public Query termQuery(Object value, QueryShardContext context) {
checkAllowExpensiveQueries(context);
if (value instanceof InetAddress) {
return InetAddressPoint.newExactQuery(name(), (InetAddress) value);
return inetAddressQuery((InetAddress) value, context);
}
String term = BytesRefs.toString(value);
if (term.contains("/")) {
return cidrQuery(term, context);
}
InetAddress address = InetAddresses.forString(term);
return new IpScriptFieldTermQuery(script, leafFactory(context.lookup()), name(), new BytesRef(InetAddressPoint.encode(address)));
return inetAddressQuery(address, context);
}

private Query inetAddressQuery(InetAddress address, QueryShardContext context) {
return new IpScriptFieldTermQuery(script, leafFactory(context), name(), new BytesRef(InetAddressPoint.encode(address)));
}

@Override
Expand All @@ -149,7 +143,7 @@ public Query termsQuery(List<?> values, QueryShardContext context) {
}
cidrQueries.add(cidrQuery(term, context));
}
Query termsQuery = new IpScriptFieldTermsQuery(script, leafFactory(context.lookup()), name(), terms);
Query termsQuery = new IpScriptFieldTermsQuery(script, leafFactory(context), name(), terms);
if (cidrQueries == null) {
return termsQuery;
}
Expand All @@ -176,6 +170,6 @@ private Query cidrQuery(String term, QueryShardContext context) {
// Force the terms into IPv6
BytesRef lowerBytes = new BytesRef(InetAddressPoint.encode(InetAddressPoint.decode(lower)));
BytesRef upperBytes = new BytesRef(InetAddressPoint.encode(InetAddressPoint.decode(upper)));
return new IpScriptFieldRangeQuery(script, leafFactory(context.lookup()), name(), lowerBytes, upperBytes);
return new IpScriptFieldRangeQuery(script, leafFactory(context), name(), lowerBytes, upperBytes);
}
}
Loading

0 comments on commit e5ad3a4

Please sign in to comment.