Scripted fields must reference values via params when filtering #8642

stacey-gammon · 2016-10-12T19:36:51Z

Groovy and expression should continue to use 'value'.

This could have been better refactored but given the timing of the release and that this is a last minute change, that I am guessing will go into the next RC, I wanted the diffs to be as small as possible.

Closes #8404

stacey-gammon · 2016-10-12T19:40:28Z

@epixa @tbragin I tagged v5.0.0 and v5.0.1 as my guess is this will be considered critical and will be merged, since it breaks filtering on scripted fields using painless. Let me know if I should adjust. I tried to keep the change small.

epixa · 2016-10-12T19:49:30Z

@stacey-gammon If we can get this done, this should go into 5.0.0. Painless is a pretty big part of 5.0 for the whole stack, so it being broken in Kibana is a big deal.

kobelb · 2016-10-12T20:22:03Z

src/ui/public/filter_manager/__tests__/filter_manager.js

@@ -114,13 +114,13 @@ describe('Filter Manager', function () {
    checkAddFilters(0, null, 3);
    expect(appState.filters).to.have.length(2);

-    let scriptedField = {name: 'scriptedField', scripted: true, script: 1};
+    let scriptedField = {name: 'scriptedField', scripted: true, script: 1, lang: 'painless'};


Can we add a separate test for the painless scripted field, instead of modifying the existing test? I think we're still supporting Groovy, but recommending painless.

So, I don't think this is a very good test, at least for this particular issue. I investigated adding a test that would actually send the query to ES but abandoned the endeavor as too time consuming (for the moment) since this is supposed to get into 5.0. The original test didn't catch this bug so I find it somewhat useless (or at least geared towards verifying something else). What if the ES syntax changes in the future? We'd end up in the same situation and end up with a similar fix - updating both test and function. So adding an extra, similar test is just an extra spot we'd have to manually adjust.

I can still add the test if you feel strongly about it, because there isn't any harm in it. :)

I agree with you that it's a poor test. I don't feel very strongly regarding it.

Bargs · 2016-10-12T20:24:44Z

Range filters still don't work (to reproduce, create a vis with a range agg and click on one of the bars).

This might be a slightly different bug, but I also noticed an error is thrown when clicking on the bars of a date histogram built on a painless date field:

Bargs · 2016-10-12T20:30:02Z

I'm also getting errors creating filters via histogram aggs:

Bargs · 2016-10-12T20:50:56Z

I ran through the rest of the aggs in the vis editor. The only one where filtering via clicking the chart seems to work is the terms agg.

stacey-gammon · 2016-10-12T21:20:12Z

@Bargs I'm pretty sure that is a separate, existing issue. The error is around the 'gte' portion of the below request we are sending to ES:

"inline" : "(doc["bytes"].value * -1)>=gte && (doc["bytes"].value * -1)<lt"

That syntax looks invalid.

All these bugs do concern me though and make me think that spending some more time on tests that would catch these issues is worth some delay.

Do you feel I should address all painless, scripted, filtered field issues in this one PR or break them up? I think I would prefer to break them up. Then, if I can figure out how to add some round trip tests to verify ES can properly consume our syntax, I can add specific tests for each specific issue.

When you say "the rest of the aggs in the vis editor" do you mean that also non-scripted field filtering isn't working in that context? Regular fields seem to work for me, just painless scripted fields.

Bargs · 2016-10-12T22:44:25Z

The range filter works ok with a lucene expression scripted field, so I think it's new with painless.

I think it would be nice to address all of the painless filter issues in this PR if we can. If we find out one of these errors are really a different beast altogether we could spin it off though.

@LeeDr's functional tests have typically been great at finding this sort of bug. What's really hurting us here is that we don't have any functional tests for scripted fields. He just created an issue for that the other day #8594. I was going to try to tackle that as soon as I had time, but if you get to it first that would be awesome!

When you say "the rest of the aggs in the vis editor" do you mean that also non-scripted field filtering isn't working in that context?

Specifically painless scripted fields

Bargs · 2016-10-12T22:50:45Z

So I just tested the query with the script based range filter and it seems to be caused by the same params problem:

Fails

                {
                    "script": {
                        "script": {
                            "inline": "(doc['bytes'].value * 2)>=gte && (doc['bytes'].value * 2)<lt",
                            "params": {
                                "gte": 7500,
                                "lt": 9000,
                                "value": ">=7,500 <9,000"
                            },
                            "lang": "painless"
                        }
                    }
                },

Works

                {
                    "script": {
                        "script": {
                            "inline": "(doc['bytes'].value * 2)>=params.gte && (doc['bytes'].value * 2)<params.lt",
                            "params": {
                                "gte": 7500,
                                "lt": 9000,
                                "value": ">=7,500 <9,000"
                            },
                            "lang": "painless"
                        }
                    }
                },

Bargs · 2016-10-12T22:56:44Z

Updating

kibana/src/ui/public/filter_manager/lib/range.js

Line 36 in ebb9cd2

const script = _.map(params, function (val, key) {

to pass params. when the lang is painless fixes the errors with histogram and range aggregations. Still need to track down the issue with the date histogram though.

stacey-gammon · 2016-10-12T23:38:39Z

Ah, consider foot properly placed in mouth. Thanks for tracking that second error down. I've updated the pull request and will also investigate the date histogram stuff.

I'll update this PR with the fixes first, and then once everything is working, see if I can add functional tests. That way we'll at least be ready to go with the fix if the tests take longer than expected.

Bargs · 2016-10-12T23:56:51Z

I did some investigating into the date errors as well. The date filter is also a range filter under the hood, so the ES error stems from the fact that the params object for this date filter has a format key but the list of operators here has no such format key, resulting in a script that looks like this:

"inline": "(doc['@timestamp'].value)>=params.gte && (doc['@timestamp'].value)<params.lt && (doc['@timestamp'].value)undefinedparams.format"

I think we could just filter out the format key from params, I'm not sure if there's a more appropriate way to handle it or not.

That fixes the actual error, but the "Invalid date" label on the filter is a bit more complicated. There are a bunch of "filter mappers" in /Users/matt/Code/kibana/src/ui/public/filter_bar/lib/map_filter.js. The script mapper is attempting to format the filter value as a date, which fails because it's actually a range.

stacey-gammon · 2016-10-13T15:36:19Z

Alrighty, I fixed some of the label issues, the compile errors, and the test failures. There are still a couple issues with the filter bar labels (see #8660) but I didn't want to include them in a hotfix, esp since some of those issues exist with non-scripted fields as well.

For a hotfix PR, this ended up being a wider scope than I originally hoped, and I'm not super confident that this doesn't introduce any issues, but I don't have a better idea, unless we want to wait for more tests to be added around this (which I can work on today, but tbd when they would be ready).

Bargs · 2016-10-13T19:27:41Z

The value param in Boolean field filters created from a Viz are getting quoted which makes them fail. I noticed they're getting created correctly on Discover though.

Bargs · 2016-10-13T19:32:46Z

src/ui/public/filter_manager/lib/range.js

@@ -33,8 +33,11 @@ export default function buildRangeFilter(field, params, indexPattern, formattedV
      lt: '<',
    };

+    params = _.pick(params, (val, key) => { return key in operators; });


I would create a new variable instead of modifying params.

Bargs · 2016-10-13T19:39:27Z

src/ui/public/filter_manager/lib/__tests__/range.js

+      expect(filter.script.script.lang).to.eql('expression');
+      expect(filter.script.script.inline).to.eql(expectedInline);
+      expect(filter.script.script.params.value).to.eql('>=1 <=3');
+      expect(filter.meta.field).to.eql('script number');


I don't understand why the old test would fail, could you explain?

I'm reverting this change, indeed it's not needed. I arrived here because initially I updated the field to match non-scripted, so it was "[start date] to [end date]" rather than ">=[start date] <=[end date]". When the test failed the params were out of order, a side affect I thought of my updated code and then I thought, how silly that the order of params matters, so adjusted the check to be like this. On second thought though, it must have failed because of the string comparison, not because of the order of params (I'm guessing the comparator is smart enough to handle that). Since I reverted the new format I'm reverting this whole file.

Bargs · 2016-10-13T19:52:01Z

src/ui/public/filter_bar/lib/map_script.js

@@ -14,8 +14,7 @@ define(function () {
          if (filter.meta.formattedValue) {
            value = filter.meta.formattedValue;
          } else {
-            value = filter.script.script.params.value;
-            value = field.format.convert(value);
+            value = '' + filter.script.script.params.value;


Unfortunately removing the call to convert has the side effect of preventing field formatters from taking effect. For instance, in the screenshot below both bytes and Double Bytes are configured with a bytes field formatter, but only the non-scripted bytes field is display correctly.

I think we need to detect when the script filter is a range, and do what map_range.js is doing in those cases https://github.com/Bargs/kibana/blob/cd760781b202e74c0dc83edcd8e36341043a42f7/src/ui/public/filter_bar/lib/map_range.js#L22-L22

whew, good catch! Reverting the label changes - will tackle that in a separate PR.

Need to use params.value instead of value. Fixes elastic#8404 Add params prefix in another spot for painless scripted fields Fix date histogram with scripted fields Remove format: epoch_millis so the script compiles. I am not 100% confident of the side affect from this (it’s used for non-scripted fields, but I’m not sure where I would put it for scripted fields, or if it’s needed). At any rate, it appears that formatting settings for scripted fields is still being honored, even after removing it from params.

Bargs · 2016-10-13T21:42:18Z

We're going to handle #8642 (comment) and #8660 separate PRs, so this LGTM.

--------- **Commit 1:** Fix our request to ES for filtering on scripted fields Need to use params.value instead of value. Fixes #8404 Add params prefix in another spot for painless scripted fields Fix date histogram with scripted fields Remove format: epoch_millis so the script compiles. I am not 100% confident of the side affect from this (it’s used for non-scripted fields, but I’m not sure where I would put it for scripted fields, or if it’s needed). At any rate, it appears that formatting settings for scripted fields is still being honored, even after removing it from params. * Original sha: 7cdb74d * Authored by Stacey Gammon <[email protected]> on 2016-10-12T14:54:23Z

Bargs · 2016-10-13T21:47:28Z

Created a new ticket for the boolean filter issue #8677

[backport] PR #8642 to 5.x

[backport] PR #8642 to 5.0

--------- **Commit 1:** Fix our request to ES for filtering on scripted fields Need to use params.value instead of value. Fixes elastic#8404 Add params prefix in another spot for painless scripted fields Fix date histogram with scripted fields Remove format: epoch_millis so the script compiles. I am not 100% confident of the side affect from this (it’s used for non-scripted fields, but I’m not sure where I would put it for scripted fields, or if it’s needed). At any rate, it appears that formatting settings for scripted fields is still being honored, even after removing it from params. * Original sha: 24622fad83d0388a38d9557e4a1ff7b80ec27083 [formerly 7cdb74d] * Authored by Stacey Gammon <[email protected]> on 2016-10-12T14:54:23Z Former-commit-id: 4b54dc6

[backport] PR elastic#8642 to 5.x Former-commit-id: e080b85

stacey-gammon added review v5.0.0 v5.0.1 v5.1.0 v6.0.0 labels Oct 12, 2016

stacey-gammon assigned Bargs Oct 12, 2016

epixa removed the v5.0.1 label Oct 12, 2016

kobelb self-assigned this Oct 12, 2016

kobelb reviewed Oct 12, 2016

View reviewed changes

stacey-gammon force-pushed the fix-scripted-field-filtering branch from c909c45 to 8db3552 Compare October 13, 2016 13:40

stacey-gammon mentioned this pull request Oct 13, 2016

Should not show 'Invalid date' message when valid data format is used #8660

Closed

stacey-gammon force-pushed the fix-scripted-field-filtering branch from acaa91a to 3fc6b90 Compare October 13, 2016 15:31

Bargs suggested changes Oct 13, 2016

View reviewed changes

stacey-gammon force-pushed the fix-scripted-field-filtering branch from 3fc6b90 to 059c138 Compare October 13, 2016 20:22

stacey-gammon force-pushed the fix-scripted-field-filtering branch from 059c138 to 7cdb74d Compare October 13, 2016 20:29

Bargs approved these changes Oct 13, 2016

View reviewed changes

stacey-gammon merged commit a5aad48 into elastic:master Oct 13, 2016

This was referenced Oct 13, 2016

[backport] PR #8642 to 5.x - Scripted fields must reference values via params when filtering #8675

Merged

[backport] PR #8642 to 5.0 - Scripted fields must reference values via params when filtering #8676

Merged

stacey-gammon added a commit that referenced this pull request Oct 13, 2016

Merge pull request #8675 from elastic/jasper/backport/8642/5.x

e080b85

[backport] PR #8642 to 5.x

stacey-gammon added a commit that referenced this pull request Oct 13, 2016

Merge pull request #8676 from elastic/jasper/backport/8642/5.0

0b519aa

[backport] PR #8642 to 5.0

epixa added v5.1.1 and removed v5.1.0 labels Dec 8, 2016

airow pushed a commit to airow/kibana that referenced this pull request Feb 16, 2017

Merge pull request elastic#8675 from elastic/jasper/backport/8642/5.x

234125a

[backport] PR elastic#8642 to 5.x Former-commit-id: e080b85

snyk-bot mentioned this pull request Mar 10, 2020

[Snyk] Security upgrade webpack from 3.6.0 to 4.29.0 xhad/scc#28

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Scripted fields must reference values via params when filtering #8642

Scripted fields must reference values via params when filtering #8642

stacey-gammon commented Oct 12, 2016 •

edited

Loading

stacey-gammon commented Oct 12, 2016

epixa commented Oct 12, 2016

kobelb Oct 12, 2016

stacey-gammon Oct 12, 2016

kobelb Oct 12, 2016

Bargs commented Oct 12, 2016

Bargs commented Oct 12, 2016

Bargs commented Oct 12, 2016

stacey-gammon commented Oct 12, 2016

Bargs commented Oct 12, 2016

Bargs commented Oct 12, 2016

Bargs commented Oct 12, 2016

stacey-gammon commented Oct 12, 2016

Bargs commented Oct 12, 2016

stacey-gammon commented Oct 13, 2016

Bargs commented Oct 13, 2016

Bargs Oct 13, 2016

stacey-gammon Oct 13, 2016

Bargs Oct 13, 2016

stacey-gammon Oct 13, 2016

Bargs Oct 13, 2016

stacey-gammon Oct 13, 2016

Bargs commented Oct 13, 2016

Bargs commented Oct 13, 2016

Scripted fields must reference values via params when filtering #8642

Scripted fields must reference values via params when filtering #8642

Conversation

stacey-gammon commented Oct 12, 2016 • edited Loading

stacey-gammon commented Oct 12, 2016

epixa commented Oct 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bargs commented Oct 12, 2016

Bargs commented Oct 12, 2016

Bargs commented Oct 12, 2016

stacey-gammon commented Oct 12, 2016

Bargs commented Oct 12, 2016

Bargs commented Oct 12, 2016

Bargs commented Oct 12, 2016

stacey-gammon commented Oct 12, 2016

Bargs commented Oct 12, 2016

stacey-gammon commented Oct 13, 2016

Bargs commented Oct 13, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Bargs commented Oct 13, 2016

Bargs commented Oct 13, 2016

stacey-gammon commented Oct 12, 2016 •

edited

Loading