From 343d79f98fc0612958c44377c732c75d4ba80658 Mon Sep 17 00:00:00 2001 From: Elastic Jasper Date: Tue, 29 Nov 2016 10:51:43 -0500 Subject: [PATCH] Fix filters for complex painless scripted fields Backports PR #9171 **Commit 1:** Fix filters for complex painless scripted fields Painless scripted fields that consisted of more than a single expression would break when the user attempted to filter on them in Discover or Visualize because of the naive way we were building them. We now wrap the user's script in a lambda so that it can be as complex at they need it to be. The only special exception is that the user cannot define named functions since those cannot go inside a Painless lambda. Fixes https://github.com/elastic/kibana/issues/9024 Related https://github.com/elastic/elasticsearch/pull/21635 * Original sha: b2a86bb02e5f59d699bc8ef82d8d9993f2d34499 * Authored by Matthew Bargar on 2016-11-21T23:18:20Z **Commit 2:** DRY it up * Original sha: 927de50d3803d2c94d9599272fb073bb0a0ddb69 * Authored by Matthew Bargar on 2016-11-22T16:21:21Z **Commit 3:** Phrase tests * Original sha: 79e69bd730b95de739687f752594ab07747f8f2c * Authored by Matthew Bargar on 2016-11-22T16:48:12Z **Commit 4:** Only include necessary comparators and add tests * Original sha: 5b9137b59288fbef0d235ea2444bb7e4731d8707 * Authored by Matthew Bargar on 2016-11-22T17:38:59Z --- src/ui/public/field_editor/field_editor.html | 11 +------ .../__tests__/filter_manager.js | 3 +- .../public/filter_manager/filter_manager.js | 6 ++-- .../filter_manager/lib/__tests__/phrase.js | 30 ++++++++++++++++++- .../filter_manager/lib/__tests__/range.js | 10 +++++++ src/ui/public/filter_manager/lib/phrase.js | 27 ++++++++++++++--- src/ui/public/filter_manager/lib/range.js | 23 +++++++++++--- 7 files changed, 87 insertions(+), 23 deletions(-) diff --git a/src/ui/public/field_editor/field_editor.html b/src/ui/public/field_editor/field_editor.html index 7de9ded73ada5..11fb9144aa3c5 100644 --- a/src/ui/public/field_editor/field_editor.html +++ b/src/ui/public/field_editor/field_editor.html @@ -147,16 +147,7 @@

- Kibana currently imposes one special limitation on the scripts you write. Your scripts must consist of a single expression. Multi-statement scripts with an explicit return statement will not work. - In other words, the above example is a valid script, but something like the following is not. -

- -

- Invalid
- - def myField = doc['some_field'].value;
- return myField; - + Kibana currently imposes one special limitation on the painless scripts you write. They cannot contain named functions.

diff --git a/src/ui/public/filter_manager/__tests__/filter_manager.js b/src/ui/public/filter_manager/__tests__/filter_manager.js index a27b56ff6926c..33725381f36de 100644 --- a/src/ui/public/filter_manager/__tests__/filter_manager.js +++ b/src/ui/public/filter_manager/__tests__/filter_manager.js @@ -5,6 +5,7 @@ import expect from 'expect.js'; import ngMock from 'ng_mock'; import FilterManagerProvider from 'ui/filter_manager'; import FilterBarQueryFilterProvider from 'ui/filter_bar/query_filter'; +import { buildInlineScriptForPhraseFilter } from '../lib/phrase'; let $rootScope; let queryFilter; let filterManager; @@ -120,7 +121,7 @@ describe('Filter Manager', function () { meta: {index: 'myIndex', negate: false, field: 'scriptedField'}, script: { script: { - inline: '(' + scriptedField.script + ') == params.value', + inline: buildInlineScriptForPhraseFilter(scriptedField), lang: scriptedField.lang, params: {value: 1} } diff --git a/src/ui/public/filter_manager/filter_manager.js b/src/ui/public/filter_manager/filter_manager.js index 6d8486f39dbcd..3bf8a22051264 100644 --- a/src/ui/public/filter_manager/filter_manager.js +++ b/src/ui/public/filter_manager/filter_manager.js @@ -1,5 +1,7 @@ import _ from 'lodash'; import FilterBarQueryFilterProvider from 'ui/filter_bar/query_filter'; +import { buildInlineScriptForPhraseFilter } from './lib/phrase'; + // Adds a filter to a passed state export default function (Private) { let queryFilter = Private(FilterBarQueryFilterProvider); @@ -54,13 +56,11 @@ export default function (Private) { break; default: if (field.scripted) { - // painless expects params.value while groovy and expression languages expect value. - const valueClause = field.lang === 'painless' ? 'params.value' : 'value'; filter = { meta: { negate: negate, index: index, field: fieldName }, script: { script: { - inline: '(' + field.script + ') == ' + valueClause, + inline: buildInlineScriptForPhraseFilter(field), lang: field.lang, params: { value: value diff --git a/src/ui/public/filter_manager/lib/__tests__/phrase.js b/src/ui/public/filter_manager/lib/__tests__/phrase.js index c21b4768715a0..53bc01f34c6d9 100644 --- a/src/ui/public/filter_manager/lib/__tests__/phrase.js +++ b/src/ui/public/filter_manager/lib/__tests__/phrase.js @@ -1,11 +1,13 @@ -import fn from 'ui/filter_manager/lib/phrase'; +import { buildInlineScriptForPhraseFilter, default as fn } from 'ui/filter_manager/lib/phrase'; import expect from 'expect.js'; import _ from 'lodash'; import ngMock from 'ng_mock'; import FixturesStubbedLogstashIndexPatternProvider from 'fixtures/stubbed_logstash_index_pattern'; + let indexPattern; let expected; + describe('Filter Manager', function () { describe('Phrase filter builder', function () { beforeEach(ngMock.module('kibana')); @@ -42,4 +44,30 @@ describe('Filter Manager', function () { expect(fn(indexPattern.fields.byName['script number'], 5, indexPattern)).to.eql(expected); }); }); + + describe('buildInlineScriptForPhraseFilter', function () { + + it('should wrap painless scripts in a lambda', function () { + const field = { + lang: 'painless', + script: 'return foo;', + }; + + const expected = `boolean compare(Supplier s, def v) {return s.get() == v;}` + + `compare(() -> { return foo; }, params.value);`; + + expect(buildInlineScriptForPhraseFilter(field)).to.be(expected); + }); + + it('should create a simple comparison for other langs', function () { + const field = { + lang: 'expression', + script: 'doc[bytes].value', + }; + + const expected = `(doc[bytes].value) == value`; + + expect(buildInlineScriptForPhraseFilter(field)).to.be(expected); + }); + }); }); diff --git a/src/ui/public/filter_manager/lib/__tests__/range.js b/src/ui/public/filter_manager/lib/__tests__/range.js index 94bee0426f4b6..dc5edd687293b 100644 --- a/src/ui/public/filter_manager/lib/__tests__/range.js +++ b/src/ui/public/filter_manager/lib/__tests__/range.js @@ -43,6 +43,16 @@ describe('Filter Manager', function () { expect(fn(indexPattern.fields.byName['script number'], {gte: 1, lte: 3}, indexPattern)).to.eql(expected); }); + it('should wrap painless scripts in comparator lambdas', function () { + const expected = `boolean gte(Supplier s, def v) {return s.get() >= v} ` + + `boolean lte(Supplier s, def v) {return s.get() <= v}` + + `gte(() -> { ${indexPattern.fields.byName['script date'].script} }, params.gte) && ` + + `lte(() -> { ${indexPattern.fields.byName['script date'].script} }, params.lte)`; + + const inlineScript = fn(indexPattern.fields.byName['script date'], {gte: 1, lte: 3}, indexPattern).script.script.inline; + expect(inlineScript).to.be(expected); + }); + it('should throw an error when gte and gt, or lte and lt are both passed', function () { expect(function () { fn(indexPattern.fields.byName['script number'], {gte: 1, gt: 3}, indexPattern); diff --git a/src/ui/public/filter_manager/lib/phrase.js b/src/ui/public/filter_manager/lib/phrase.js index b9e9cc92fa573..37d64ff4d02a7 100644 --- a/src/ui/public/filter_manager/lib/phrase.js +++ b/src/ui/public/filter_manager/lib/phrase.js @@ -3,9 +3,6 @@ export default function buildPhraseFilter(field, value, indexPattern) { let filter = { meta: { index: indexPattern.id} }; if (field.scripted) { - // painless expects params.value while groovy and expression languages expect value. - const valueClause = field.lang === 'painless' ? 'params.value' : 'value'; - // See https://github.com/elastic/elasticsearch/issues/20941 and https://github.com/elastic/kibana/issues/8677 // for the reason behind this change. ES doesn't handle boolean types very well, so they come // back as strings. @@ -17,8 +14,10 @@ export default function buildPhraseFilter(field, value, indexPattern) { convertedValue = value === 'true' ? true : false; } + const script = buildInlineScriptForPhraseFilter(field); + _.set(filter, 'script.script', { - inline: '(' + field.script + ') == ' + valueClause, + inline: script, lang: field.lang, params: { value: convertedValue @@ -34,3 +33,23 @@ export default function buildPhraseFilter(field, value, indexPattern) { } return filter; }; + + +/** + * Takes a scripted field and returns an inline script appropriate for use in a script query. + * Handles lucene expression and Painless scripts. Other langs aren't guaranteed to generate valid + * scripts. + * + * @param {object} scriptedField A Field object representing a scripted field + * @returns {string} The inline script string + */ +export function buildInlineScriptForPhraseFilter(scriptedField) { + // We must wrap painless scripts in a lambda in case they're more than a simple expression + if (scriptedField.lang === 'painless') { + return `boolean compare(Supplier s, def v) {return s.get() == v;}` + + `compare(() -> { ${scriptedField.script} }, params.value);`; + } + else { + return `(${scriptedField.script}) == value`; + } +} diff --git a/src/ui/public/filter_manager/lib/range.js b/src/ui/public/filter_manager/lib/range.js index deec58b703199..a5463b86a03d7 100644 --- a/src/ui/public/filter_manager/lib/range.js +++ b/src/ui/public/filter_manager/lib/range.js @@ -32,14 +32,29 @@ export default function buildRangeFilter(field, params, indexPattern, formattedV lte: '<=', lt: '<', }; + const comparators = { + gt: 'boolean gt(Supplier s, def v) {return s.get() > v}', + gte: 'boolean gte(Supplier s, def v) {return s.get() >= v}', + lte: 'boolean lte(Supplier s, def v) {return s.get() <= v}', + lt: 'boolean lt(Supplier s, def v) {return s.get() < v}', + }; const knownParams = _.pick(params, (val, key) => { return key in operators; }); - const script = _.map(knownParams, function (val, key) { - // painless expects params.[key] while groovy and expression languages expect [key] only. - const valuePrefix = field.lang === 'painless' ? 'params.' : ''; - return '(' + field.script + ')' + operators[key] + valuePrefix + key; + let script = _.map(knownParams, function (val, key) { + return '(' + field.script + ')' + operators[key] + key; }).join(' && '); + // We must wrap painless scripts in a lambda in case they're more than a simple expression + if (field.lang === 'painless') { + const currentComparators = _.reduce(knownParams, (acc, val, key) => acc.concat(comparators[key]), []).join(' '); + + const comparisons = _.map(knownParams, function (val, key) { + return `${key}(() -> { ${field.script} }, params.${key})`; + }).join(' && '); + + script = `${currentComparators}${comparisons}`; + } + const value = _.map(knownParams, function (val, key) { return operators[key] + field.format.convert(val); }).join(' ');