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

Add exclusion support to response filtering #19865

Merged
merged 1 commit into from Aug 30, 2016

Conversation

tlrx
Copy link
Member

@tlrx tlrx commented Aug 8, 2016

This PR adds support for excluding filters to the response filtering feature.

It adds the parameters filter_path_include and filter_path_exclude that can be used to combine inclusive and/or exclusive filters when filtering the response from a REST API request execution:

GET /_cluster/state?filter_path_include=metadata.indices.*.state&filter_path_exclude=metadata.indices.logs-*

{
  "metadata" : {
    "indices" : {
      "index-1" : {
        "state" : "open"
      },
      "index-3" : {
        "state" : "open"
      },
      "index-2" : {
        "state" : "open"
      }
    }
  }
}

When both filter_path_include and filter_path_exclude are set, it works by chaining two FilteringGeneratorDelegate: one for content exclusion and one for content inclusion (it always excludes first). If the whole content is filtered out, the output is an empty object.

"source",
"filter_path",
"filter_path_include",
"filter_path_exclude");
Copy link
Member

Choose a reason for hiding this comment

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

you should sync up with @elastic/es-clients to make sure that they make the same change in their clients/runners if needed. Otherwise their runners may barf as some parameters are provided although they are not listed in the api spec.

Copy link
Member Author

Choose a reason for hiding this comment

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

I agree, but I finally removed the filter_path_excludes and filter_path_includes in favor of a +/- support using the current filter_path parameter, as Clinton suggested.

@clintongormley
Copy link

@tlrx what about ant syntax, eg

?filter_path=metadata.indices.*.state,-metadata.indices.logs-*

@tlrx
Copy link
Member Author

tlrx commented Aug 11, 2016

@clintongormley Sure, I also prefer this notation.

@tlrx
Copy link
Member Author

tlrx commented Aug 17, 2016

This has been updated according to your comments @clintongormley @javanna

--------------------------------------------------

And for more control, both inclusive and exclusive filters can be combined in the same expression:

Choose a reason for hiding this comment

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

Worth specifying which takes precedence: include or exclude

Copy link
Member Author

Choose a reason for hiding this comment

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

I updated the doc.

@s1monw
Copy link
Contributor

s1monw commented Aug 29, 2016

left some minors LGTM otherwise

@tlrx tlrx force-pushed the add-filter-path-exclusions branch 4 times, most recently from 06a531a to 48c42a5 Compare August 29, 2016 19:12
This commit adds the support for exclusion filter to the response filtering (filter_path) feature. It changes the XContentBuilder APIs so that it now accepts two types of filters: inclusive and exclusive. Filters are no more String arrays but sets of String instead.
@tlrx tlrx force-pushed the add-filter-path-exclusions branch from 48c42a5 to b4245c7 Compare August 30, 2016 07:12
@tlrx tlrx merged commit b4245c7 into elastic:master Aug 30, 2016
@tlrx
Copy link
Member Author

tlrx commented Aug 30, 2016

Thanks @s1monw - merged with your suggestions.

@s1monw
Copy link
Contributor

s1monw commented Aug 30, 2016

thanks @tlrx

@tlrx tlrx deleted the add-filter-path-exclusions branch August 30, 2016 07:33
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants