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

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

Merged
merged 4 commits into from
Mar 29, 2018

Conversation

Bargs
Copy link
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
Member

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
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
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
Member

@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.

@@ -1,18 +1,12 @@
import { buildQueryFromFilters } from '../from_filters';
Copy link
Member

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
Member

@Bargs Did this PR fall through the cracks?

@Bargs
Copy link
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
Contributor

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
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
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
Contributor

💔 Build Failed

@elasticmachine
Copy link
Contributor

💚 Build Succeeded

Copy link
Contributor

@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