Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ML] Anomaly Explorer - ensure valid syntax after removing 2nd of 3 filters via icon #34187

Conversation

alvarezmelissa87
Copy link
Contributor

@alvarezmelissa87 alvarezmelissa87 commented Mar 29, 2019

Summary

Fix for #33861

When creating filters via filter icons in Anomaly Explorer - if middle filter is removed via icon duplicate and operators are left in the filter bar - which is invalid syntax.

This PR adds a check for those duplicate operators when removing a filter via icons and removes the duplication. Resulting syntax is valid.

Checklist

Use strikethroughs to remove checklist items you don't feel are applicable to this PR.

For maintainers

- [ ] This was checked for breaking API changes and was labeled appropriately
- [ ] This includes a feature addition or change that requires a release note and was labeled appropriately

@alvarezmelissa87 alvarezmelissa87 added non-issue Indicates to automation that a pull request should not appear in the release notes :ml Feature:ml-results legacy - do not use v7.2.0 labels Mar 29, 2019
@elasticmachine
Copy link
Contributor

Pinging @elastic/ml-ui

@alvarezmelissa87 alvarezmelissa87 requested a review from a team as a code owner March 29, 2019 18:46
@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

This works for three filters joined with ands, but the same sort of issue occurs when I have three filters linked with ors. Here I have removed the middle filter using the - icon, but an extra or is left in, so I see
image

rather than
image

Could this fix be extended to work with the or operator too?

@alvarezmelissa87
Copy link
Contributor Author

alvarezmelissa87 commented Apr 1, 2019

@peteharverson - added a fix for the OR operator as well 👍
@jgowdyelastic, @walterra - would you be up for taking a look as well? Thanks!

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@peteharverson peteharverson left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@walterra walterra left a comment

Choose a reason for hiding this comment

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

LGTM

// If string has a double 'and' operator (e.g. tag:thing and and tag:other) remove as it is illegal kuery syntax
const invalidAndPattern = /\s+(and)\s+(and)\s+/ig;
// If string has a double 'or' operator (e.g. tag:thing or or tag:other) remove as it is illegal kuery syntax
const invalidOrPattern = /\s+(or)\s+(or)\s+/ig;
Copy link
Member

Choose a reason for hiding this comment

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

this won't catch OR AND or AND OR.
the regex could be changed to something like /\s+(and|or)\s+(and|or)\s+/ig to catch all combinations of duplicate operators.

Copy link
Member

@jgowdyelastic jgowdyelastic left a comment

Choose a reason for hiding this comment

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

LGTM

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

@alvarezmelissa87 alvarezmelissa87 merged commit d3dd95d into elastic:master Apr 2, 2019
alvarezmelissa87 added a commit to alvarezmelissa87/kibana that referenced this pull request Apr 2, 2019
…ilters via icon (elastic#34187)

* Remove middle filter value correctly using icons

* remove middle filter value in OR query correctly with icons

* combine operator pattern check
@alvarezmelissa87 alvarezmelissa87 deleted the ml-anomaly-explorer-filter-syntax branch April 2, 2019 08:36
alvarezmelissa87 added a commit that referenced this pull request Apr 2, 2019
…ilters via icon (#34187) (#34340)

* Remove middle filter value correctly using icons

* remove middle filter value in OR query correctly with icons

* combine operator pattern check
@sophiec20 sophiec20 added the Feature:Anomaly Detection ML anomaly detection label Jun 19, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Feature:Anomaly Detection ML anomaly detection Feature:ml-results legacy - do not use :ml non-issue Indicates to automation that a pull request should not appear in the release notes v7.2.0
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants