Check for runtime field loops in queries #61927

nik9000 · 2020-09-03T15:06:35Z

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

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 elastic#59332

elasticmachine · 2020-09-03T15:06:37Z

Pinging @elastic/es-search (:Search/Search)

nik9000 · 2020-09-03T15:07:36Z

...rc/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java

@@ -61,6 +71,26 @@ public final boolean isAggregatable() {
        return true;
    }

+    /**


I pulled this function into the superclass because it the "for query" version of it is tricky. We could probably avoid the type parameter if we really wanted to by being a bit more tricky, but I'm not sure it is worth it.

nik9000 · 2020-09-03T15:07:53Z

...rc/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java

    public Query regexpQuery(
        String value,
-        int flags,
+        int syntaxFlags,
+        int matchFlags,


Missing this was a bug that causes regexes not to work at all!

nik9000 · 2020-09-03T15:08:42Z

...elds/src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptIpMappedFieldType.java

@@ -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);


This was a bug!

should we push this separately?

I'm ok pushing it as part of this because this is ready.

nik9000 · 2020-09-03T15:09:02Z

...src/main/java/org/elasticsearch/xpack/runtimefields/mapper/ScriptKeywordMappedFieldType.java

        checkAllowExpensiveQueries(context);
-        return new StringScriptFieldRegexpQuery(script, leafFactory(context.lookup()), name(), value, flags, maxDeterminizedStates);
+        if (matchFlags != 0) {


I'd like to leave this for later.

I am no longer sure what is left to do here. Do we need an issue for it? Didn't you push the regex changes separately?

@markharwood is working on a change that'll take the matchFlags into account.

I'm not sure why this isn't a noop - I thought I pushed this exact change separately.

Ah, not it is a noop.

javanna · 2020-09-09T15:58:45Z

...rc/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java

    public Query phrasePrefixQuery(TokenStream stream, int slop, int maxExpansions) throws IOException {
        throw new IllegalArgumentException(unsupported("phrase prefix", "text"));
    }

+    @Override


I've seen this popup in this and some other PR, can we merge this fix separately?

I've pushed a fix for this to master and 7.x. I'll merge that into this branch now.

javanna · 2020-09-09T20:17:52Z

x-pack/plugin/src/test/resources/rest-api-spec/test/runtime_fields/90_loops.yml

@@ -183,6 +183,7 @@ setup:
            loop:
             terms:
               field: over_max_depth


do you mind adding one test here also for loops around queries? I know you wrote a lot of those above (thanks) but I would find this yaml suite more complete if we also covered loops for queries (I did not add specific tests in the first place because I knew they would fail)

I've added some in the last commit I pushed.

nik9000 · 2020-09-14T13:11:56Z

...rc/main/java/org/elasticsearch/xpack/runtimefields/mapper/AbstractScriptMappedFieldType.java

@@ -149,7 +179,7 @@ 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() + "]";
+        String thisField = "[" + name() + "] which is of type [runtime] with runtime_type [" + runtimeType() + "]";


This is something I bumped into while working on this. Just had the old name left over.

use the constant, then renaming is much easier.

nik9000 · 2020-09-14T13:12:49Z

...g/elasticsearch/xpack/runtimefields/mapper/AbstractNonTextScriptMappedFieldTypeTestCase.java

@@ -43,7 +43,7 @@ private void assertQueryOnlyOnTextAndKeyword(String queryName, ThrowingRunnable
            equalTo(
                "Can only use "
                    + queryName
-                    + " queries on keyword and text fields - not on [test] which is of type [script] with runtime_type ["


This is the other half of the "old name" bug I fixed way above.

nik9000 · 2020-09-14T13:14:02Z

x-pack/plugin/src/test/resources/rest-api-spec/test/runtime_fields/90_loops.yml

@@ -183,6 +183,7 @@ setup:
            loop:
             terms:
               field: over_max_depth


I've added some in the last commit I pushed.

javanna

I left a few nits, LGTM otherwise

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 elastic#59332

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

nik9000 added :Search/Search Search-related issues that do not fall into other categories v8.0.0 v7.10.0 labels Sep 3, 2020

nik9000 requested a review from javanna September 3, 2020 15:06

elasticmachine added the Team:Search Meta label for search team label Sep 3, 2020

nik9000 commented Sep 3, 2020

View reviewed changes

nik9000 added 2 commits September 3, 2020 11:39

Merge branch 'master' into runtime_fields_loop_in_query

0716b27

Merge branch 'master' into runtime_fields_loop_in_query

9ce7174

javanna reviewed Sep 9, 2020

View reviewed changes

nik9000 added 3 commits September 10, 2020 17:49

Merge branch 'master' into runtime_fields_loop_in_query

72c1b8b

Compile

f780a42

Prettier

4bc988b

nik9000 commented Sep 14, 2020

View reviewed changes

Merge branch 'master' into runtime_fields_loop_in_query

b66a16d

javanna approved these changes Sep 15, 2020

View reviewed changes

nik9000 added 2 commits September 15, 2020 13:01

iter

45d124b

Merge branch 'master' into runtime_fields_loop_in_query

bef47e2

nik9000 merged commit 12c3c15 into elastic:master Sep 15, 2020

nik9000 added the backport pending label Sep 15, 2020

nik9000 removed the backport pending label Sep 15, 2020

javanna added v7.11.0 and removed v7.10.0 labels Oct 7, 2020

pugnascotia added >bug >non-issue labels Dec 16, 2020

jakelandis added v8.0.0-alpha1 and removed v8.0.0 labels Jul 26, 2021

Check for runtime field loops in queries #61927

Check for runtime field loops in queries #61927

Uh oh!

Conversation

nik9000 commented Sep 3, 2020

Uh oh!

elasticmachine commented Sep 3, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

javanna left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!