Skip to content

Don't apply query:queryString:options to query_string filters#15640

Merged
Bargs merged 4 commits intoelastic:masterfrom
Bargs:queryStringFilter
Mar 29, 2018
Merged

Don't apply query:queryString:options to query_string filters#15640
Bargs merged 4 commits intoelastic:masterfrom
Bargs:queryStringFilter

Conversation

@Bargs
Copy link
Copy Markdown
Contributor

@Bargs Bargs commented Dec 15, 2017

Fixes #15527

Lukas and I chatted over Slack and we both agreed it didn't really make sense to apply query:queryString:options to query_string filters. If someone is using the advanced query DSL editor to create a query_string filter they probably want full control over it.

This is a breaking change so it will only go in 7.0. In 6.x users should use one of the workarounds.

@lukasolson
Copy link
Copy Markdown
Contributor

After thinking about this some more, I think it might actually make sense to add the query:queryString:options to query_string filters. This is because editing the query DSL of a filter is not the only way to get a query_string filter. Another way is to create a visualization and use the "Filters" aggregation. I think it does make sense for the filters that are created from clicking on of these to send the params. Thoughts?

@Bargs
Copy link
Copy Markdown
Contributor Author

Bargs commented Dec 19, 2017

@lukasolson I agree those should have query:queryString:options applied, but it already works that way. The createFilter helper for the filters agg calls toDSL on the aggConfig. This calls the write method on the filters aggType which uses decorateQuery.

@epixa
Copy link
Copy Markdown
Contributor

epixa commented Dec 22, 2017

Can you add breaking change docs for this in this PR? I think this would be the first one for 7.0, so you might need to do some basic setup for the 7.0 breaking change docs.

Copy link
Copy Markdown
Contributor

@lukasolson lukasolson left a comment

Choose a reason for hiding this comment

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

LGTM, just made a recommendation to add a test.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Could you just add a simple test here that verifies that the query:queryString:options aren't added in?

@lukasolson
Copy link
Copy Markdown
Contributor

@Bargs Did this PR fall through the cracks?

@Bargs
Copy link
Copy Markdown
Contributor Author

Bargs commented Feb 6, 2018

I need to add that test and update the docs. Just hasn't been a priority vs other stuff since it won't ship till 7.0.

@stacey-gammon stacey-gammon removed their request for review March 8, 2018 17:04
@stacey-gammon
Copy link
Copy Markdown

Since this isn't active right now I'm removing myself as a reviewer, just to clean up my "reviews requested of you" view. Just pop me back on whenever it's ready to go again.

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@Bargs Bargs force-pushed the queryStringFilter branch from 454d9b1 to 1e3042d Compare March 26, 2018 16:46
@Bargs Bargs requested a review from stacey-gammon March 26, 2018 17:15
@Bargs
Copy link
Copy Markdown
Contributor Author

Bargs commented Mar 26, 2018

Rebased and added the breaking change doc @epixa asked for. Also added the test @lukasolson asked for.

@stacey-gammon please take another look.

@elasticmachine
Copy link
Copy Markdown
Contributor

💔 Build Failed

@elasticmachine
Copy link
Copy Markdown
Contributor

💚 Build Succeeded

Copy link
Copy Markdown

@stacey-gammon stacey-gammon left a comment

Choose a reason for hiding this comment

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

LGTM!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants