Skip to content

Refuse to load fields from _source when using the fields option and support wildcards. #15017

Merged
jimczi merged 1 commit intoelastic:masterfrom
jimczi:fields_option
Nov 30, 2015
Merged

Refuse to load fields from _source when using the fields option and support wildcards. #15017
jimczi merged 1 commit intoelastic:masterfrom
jimczi:fields_option

Conversation

@jimczi
Copy link
Copy Markdown
Contributor

@jimczi jimczi commented Nov 25, 2015

Fixes #14489
Do not to load fields from _source when using the fields option.
Non stored (non existing) fields are ignored by the fields visitor when using the fields option.

Fixes #10783
Support * wildcard to retrieve stored fields when using the fields option.
Supported pattern styles are "xxx_", "_xxx", "xxx" and "xxx*yyy".

jpountz
jpountz reviewed Nov 25, 2015
View reviewed changes
core/src/main/java/org/elasticsearch/index/fieldvisitor/WildcardFieldsVisitor.java Outdated
Copy link
Copy Markdown
Contributor

@jpountz jpountz Nov 25, 2015

Choose a reason for hiding this comment

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

Should we fold the functionality directly into CustomFieldsVisitor?

@jpountz
Copy link
Copy Markdown
Contributor

jpountz commented Nov 25, 2015

Thanks Jim, I'm happily surprised that there were not too many tests using the fields option to get fields from the _source! Could you add a note to docs/reference/migration/migrate_3_0.asciidoc to let users know that in 3.0 the fields option won't be able to load fields from _source anymore?

@jpountz jpountz self-assigned this Nov 25, 2015
jpountz
jpountz reviewed Nov 26, 2015
View reviewed changes
rest-api-spec/src/main/resources/rest-api-spec/test/search/10_source_filtering.yaml
Copy link
Copy Markdown
Contributor

@jpountz jpountz Nov 26, 2015

Choose a reason for hiding this comment

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

The point of the two above tests seems to be to test bw compat, I think we can remove them?

@jpountz
Copy link
Copy Markdown
Contributor

jpountz commented Nov 26, 2015

Thanks @jimferenczi this looks good to me. I just left a comment about two REST tests that I think we should remove. @bleskes would you mind having a quick look at this PR before it gets merged?

@bleskes
Copy link
Copy Markdown
Contributor

bleskes commented Nov 26, 2015

This looks good to me. Thanks @jimferenczi . My only question is whether we should resolve wild cards in the field visitor. Feels more natural for me to resolve it based on the mappings and get a concrete list to the visitor. I think we have the support we need in the MapperService (or will be easy to build). I'm worried that a wild card on the lucene level may expose system/hidden fields users didn't mean.. I don't think it's a problem in practice though. @jpountz wdyt?

jpountz
jpountz reviewed Nov 27, 2015
View reviewed changes
core/src/main/java/org/elasticsearch/search/fetch/FetchPhase.java
Copy link
Copy Markdown
Contributor

@jpountz jpountz Nov 27, 2015

Choose a reason for hiding this comment

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

is AllFieldsVisitor still used with your change? If not can you remove it?

Copy link
Copy Markdown
Contributor Author

@jimczi jimczi Nov 27, 2015

Choose a reason for hiding this comment

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

no it's not. I'll remove it.

@jpountz
Copy link
Copy Markdown
Contributor

jpountz commented Nov 27, 2015

@bleskes Good question. We already support fields=* in master (see mentions of loadAllStoredFields in the code) and I'm not aware of problems, so I think we should be fine.

@clintongormley clintongormley added >enhancement :Search/Search Search-related issues that do not fall into other categories labels Nov 28, 2015
@jpountz
Copy link
Copy Markdown
Contributor

jpountz commented Nov 30, 2015

@jimferenczi Let's merge this one?

@jimczi
Fixes #14489
731833c 
  Do not to load fields from _source when using the `fields` option.
  Non stored (non existing) fields are ignored by the fields visitor when using the `fields` option.

Fixes #10783
  Support * wildcard to retrieve stored fields when using the `fields` option.
  Supported pattern styles are "xxx*", "*xxx", "*xxx*" and "xxx*yyy".
jimczi added a commit that referenced this pull request Nov 30, 2015
@jimczi
Merge pull request #15017 from jimferenczi/fields_option
e182072 
Refuse to load fields from _source when using the `fields` option and support wildcards.
@jimczi jimczi merged commit e182072 into elastic:master Nov 30, 2015
@jimczi jimczi deleted the fields_option branch November 30, 2015 10:01
@clintongormley
Copy link
Copy Markdown
Contributor

clintongormley commented Nov 30, 2015

@jimferenczi please add version labels for each branch that you merged this PR into

@clintongormley clintongormley mentioned this pull request Jan 17, 2016
Closed
@clintongormley clintongormley mentioned this pull request Jun 17, 2016
Closed
@jpountz jpountz mentioned this pull request Aug 25, 2016
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

@jpountz jpountz

Labels

>enhancement :Search/Search Search-related issues that do not fall into other categories v5.0.0-alpha1

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@jimczi @jpountz @bleskes @clintongormley