Correct display of warning header by jasontedor · Pull Request #10470 · elastic/kibana

jasontedor · 2017-02-20T21:29:35Z

Elasticsearch produces warning headers for the use of deprecated features. These warning headers can contain commas which breaks the splitting of multiple values on commas. Upstream Elasticsearch has changed the warning headers to be specification compliant so that these headers can be safely split (the warning text and warning date must be quoted, so splits should only occur on commas that are not contained in quotes). Additionally, the upstream change includes additional details that are not needed for display in Console (a warning code, the Elasticsearch version that produced the warning, and the warning date). This commit corrects the splitting logic in Console to only split on commas not contained in quotes, and to extract the warning text from each warning header.

Relates elastic/elasticsearch#23275

Elasticsearch produces warning headers for the use of deprecated features. These warning headers can contain commas which breaks the splitting of multiple values on commas. Upstream Elasticsearch has changed the warning headers to be specification compliant so that these headers can be safely split (the warning text and warning date must be quoted, so splits should only occur on commas that are not contained in quotes). Additionally, the upstream change includes additional details that are not needed for display in Console (a warning code, the Elasticsearch version that produced the warning, and the warning date). This commit corrects the splitting logic in Console to only split on commas not contained in quotes, and to extract the warning text from each warning header.

jasontedor · 2017-02-20T21:33:06Z

@epixa I discussed this with @clintongormley. As it currently stands, this feature is buggy in case Elasticsearch sends back a warning header that contains a comma (for example, [groovy] scripts are deprecated, use [painless] scripts instead, relates elastic/elasticsearch#22986) as the header splitting mechanism splits on commas followed by a space which catches legitimate warning messages. Moreover, Elasticsearch is violating the specification for the format of warning headers. Therefore, we have targeted elastic/elasticsearch#23275 for v5.3.0 to correct the non-compliant headers, and then this enables the fix here so that the headers can be split correctly. Thus, this fix should go into v5.3.0 as well.

jbudz · 2017-02-22T19:50:29Z

src/core_plugins/console/public/src/input.js

-              warnings = _.map(warnings.split(", "), function (warning) {
-                  return "#! Deprecation: " + warning;
+              // pattern for valid warning header
+              var re = /\d{3} [^ ]+ \"([^"]*)\"( \"[^"]*\")/


should this support escaped quotes in the message?

That's a good question but I do not think it's necessary. The warning headers produced by Elasticsearch will not contained escaped quotes.

A regex to handle escaped quotes and escaped backslashes would look like this:

var re = /\d{3} \S+ \"((?:\\[\\"]|[^"])*)\"( \"(?:\\[\\"]|[^"])*\")/

Learning from the regex master: do we need the extra \\ in [\\"] ?

Ah you've just pointed out a flaw in my regex. Not such a master now! In fact it should be:

"((?:[^"\\]|\\.)*)"

Which means:

match "

capture:

zero or more times: match any character except \ or " or match \ followed by any character

match "

So it'd match this correctly:

"Here is an embedded \\ and a \" quote and \n"

That handles any backslashed character, not just quotes

Bah - ignore me. This would work (which is what I think you were suggesting):

"((?:\\"|[^"])*)"

Which says:

match *

capture:

zero or more times: match \" or any character except "

match "

After additional discussion with @clintongormley and @bleskes, it has been agreed that we should allow escaped quotes in warning messages, so I will work on a change on both sides to support them. Thanks for raising the question @jbudz. 😄

jbudz · 2017-02-22T19:56:37Z

src/core_plugins/console/public/src/input.js

+              var re = /\d{3} [^ ]+ \"([^"]*)\"( \"[^"]*\")/
+              // split on any comma that is followed by an even number of quotes
+              warnings = _.map(warnings.split(/, (?=(?:[^\"]*\"[^\"]*\")*[^\"]*$)/), function (warning) {
+                var match = re.exec(warning)


if there's no match, accessing match[1] will cause a full screen fatal error. thoughts on a match !== null check?

This is a good point. The warning headers produced by Elasticsearch should always match this pattern but I think your suggestion to err on the side of safety is a good one.

I pushed 7f98c48.

If the warning header from Elasticsearch comes back in the wrong format, this could lead to the regex not matching which would lead to a blow-up. While Elasticsearch should not do this, let us be defensive here.

epixa · 2017-02-22T21:33:33Z

I kicked off a new build on this because the old one aborted on jenkins: https://kibana-ci.elastic.co/job/elastic+kibana+pull-request+multijob-selenium/2722/console

tylersmalley

LGTM - tested against #10470

bleskes

Thanks @jasontedor for picking this side of the equation as well. Can we add a unit test for the regexes (I can help with where to put them and how to run). Also I think we should deal with escaping properly so we never have to worry about this again.

bleskes · 2017-02-26T20:44:39Z

src/core_plugins/console/public/src/input.js

+              warnings = _.map(utils.splitOnUnquotedCommaSpace(warnings), function (warning) {
                var match = re.exec(warning)
                // extract the actual warning if there was a match
                return "#! Deprecation: " + (match !== null ? match[1] : warning)


we should unescape a match... Also, I wonder if it's got have all of this as a util method (i.e., extractDeprecationMessages ) and test it.

I pushed 8a9f432.

bleskes · 2017-02-26T20:44:47Z

src/core_plugins/console/public/src/utils.js

  });
 }

+utils.splitOnUnquotedCommaSpace = function (s) {


fancy pants

bleskes

Sweet. Thanks Jason!

epixa · 2017-02-27T16:25:30Z

Is this backwards compatible with the old warning headers? In other words, is it safe to merge even if the Elasticsearch PR isn't in?

re-reviewing

epixa · 2017-02-27T17:17:29Z

The corresponding Elasticsearch PR went in, so my question is no longer valid. @jbudz can you merge/backport this to 5.3?

Backports PR #10470 **Commit 1:** Correct display of warning header Elasticsearch produces warning headers for the use of deprecated features. These warning headers can contain commas which breaks the splitting of multiple values on commas. Upstream Elasticsearch has changed the warning headers to be specification compliant so that these headers can be safely split (the warning text and warning date must be quoted, so splits should only occur on commas that are not contained in quotes). Additionally, the upstream change includes additional details that are not needed for display in Console (a warning code, the Elasticsearch version that produced the warning, and the warning date). This commit corrects the splitting logic in Console to only split on commas not contained in quotes, and to extract the warning text from each warning header. * Original sha: 02896ea * Authored by Jason Tedor <jason@tedor.me> on 2017-02-20T20:44:14Z **Commit 2:** Safer regex handling If the warning header from Elasticsearch comes back in the wrong format, this could lead to the regex not matching which would lead to a blow-up. While Elasticsearch should not do this, let us be defensive here. * Original sha: 7f98c48 * Authored by Jason Tedor <jason@tedor.me> on 2017-02-22T20:23:35Z **Commit 3:** Stricter handling of warning headers * Original sha: f14055c * Authored by Jason Tedor <jason@tedor.me> on 2017-02-24T19:08:28Z **Commit 4:** Add tests for deprecation messages * Original sha: 8a9f432 * Authored by Jason Tedor <jason@tedor.me> on 2017-02-27T15:42:56Z

jbudz · 2017-02-27T17:52:49Z

5.3: 2a52a9d
5.x/5.4: 4b954d4

jasontedor · 2017-03-01T02:16:57Z

Thanks all for the reviews.

Relates to elastic/kibana#10470

jasontedor added v5.3.0 v5.4.0 v6.0.0 labels Feb 20, 2017

jasontedor requested review from bleskes and tylersmalley February 20, 2017 21:29

epixa added review and removed review labels Feb 21, 2017

jasontedor added the blocker label Feb 22, 2017

jbudz self-assigned this Feb 22, 2017

jbudz reviewed Feb 22, 2017

View reviewed changes

Safer regex handling

7f98c48

If the warning header from Elasticsearch comes back in the wrong format, this could lead to the regex not matching which would lead to a blow-up. While Elasticsearch should not do this, let us be defensive here.

jbudz previously approved these changes Feb 22, 2017

View reviewed changes

tylersmalley self-assigned this Feb 22, 2017

tylersmalley approved these changes Feb 23, 2017

View reviewed changes

bleskes suggested changes Feb 23, 2017

View reviewed changes

Stricter handling of warning headers

f14055c

bleskes reviewed Feb 26, 2017

View reviewed changes

src/core_plugins/console/public/src/utils.js

});

}

utils.splitOnUnquotedCommaSpace = function (s) {

Copy link
Copy Markdown

Contributor

bleskes Feb 26, 2017

Choose a reason for hiding this comment

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

fancy pants

Add tests for deprecation messages

8a9f432

bleskes approved these changes Feb 27, 2017

View reviewed changes

jbudz approved these changes Feb 27, 2017

View reviewed changes

epixa added the review label Feb 27, 2017

jbudz merged commit 38ca174 into elastic:master Feb 27, 2017

This was referenced Feb 27, 2017

[5.3] Correct display of warning header #10594

Merged

[5.x] Correct display of warning header #10595

Merged

jasontedor deleted the warning-header branch March 1, 2017 02:16

dakrone added a commit to dakrone/es-mode that referenced this pull request Mar 8, 2017

Display ES' header warnings in output of executed commands

527a2f7

Relates to elastic/kibana#10470

jasontedor mentioned this pull request Jan 18, 2019

Add some deprecation optimizations elastic/elasticsearch#37597

Merged

Conversation

jasontedor commented Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jasontedor commented Feb 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

clintongormley Feb 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

epixa commented Feb 22, 2017

Uh oh!

tylersmalley left a comment

Choose a reason for hiding this comment

Uh oh!

bleskes left a comment

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!

bleskes left a comment

Choose a reason for hiding this comment

Uh oh!

epixa commented Feb 27, 2017

Uh oh!

epixa commented Feb 27, 2017

Uh oh!

jbudz commented Feb 27, 2017

Uh oh!

jasontedor commented Mar 1, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

jasontedor commented Feb 20, 2017 •

edited

Loading

jasontedor commented Feb 20, 2017 •

edited

Loading

clintongormley Feb 23, 2017 •

edited

Loading