Skip to content

Do not allow inner hits that fetch _source and have a non nested object field as parent #25749

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

Merged

Conversation

martijnvg
Copy link
Member

The nested source fetch logic can't properly select the part of the source that belongs to a specific nested document if a nested object field's parent object field is non nested.

PR for #25315

@jpountz
Copy link
Contributor

jpountz commented Jul 28, 2017

I think it could break quite a number of users? I'm wondering whether we should do the check at search time instead only in the case that the field is mapped as an object and is stored in the source as an array?

@martijnvg martijnvg force-pushed the nested_inner_hits_do_not_allow_object_field branch from f25bf42 to ece0e47 Compare July 31, 2017 12:47
@martijnvg
Copy link
Member Author

@jpountz I moved the check to fetch phase. This basically means that we throw
a better error message (instead of an AOBE) and not adding more restrictions.

@martijnvg martijnvg force-pushed the nested_inner_hits_do_not_allow_object_field branch from ece0e47 to 7cf26f4 Compare July 31, 2017 13:22
Copy link
Contributor

@jpountz jpountz left a comment

Choose a reason for hiding this comment

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

I left a question about the validation.

throw new IllegalArgumentException("Cannot execute inner hits. One or more parent object fields of nested field [" +
nestedObjectMapper.name() + "] are not nested. All parent fields need to be nested fields too");
}
sourceAsMap = (Map<String, Object>) nestedParsedSource.get(nested.getOffset());
Copy link
Contributor

Choose a reason for hiding this comment

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

You are only checking the first one, but I think we need to iterate over entries from 0 to nested.getOffset() ?

Copy link
Member Author

Choose a reason for hiding this comment

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

When the parent object is not nested then XContentMapValues.extractValue(...) extracts the values from two or more layers resulting in a list of list being returned, because there is one nested level, but in the _source there are two ore more levels. This is why I think only the first element of nestedParsedSource needs to be checked. Checking upto nested.getOffset() will likely cause an AOBE.

I'll add a comment explaining that.

@martijnvg martijnvg force-pushed the nested_inner_hits_do_not_allow_object_field branch from 7cf26f4 to 20ba983 Compare August 9, 2017 06:59
@colings86 colings86 added v5.5.4 and removed v5.5.3 labels Aug 25, 2017
@colings86 colings86 added v5.6.2 and removed v5.6.1 labels Sep 14, 2017
@martijnvg martijnvg force-pushed the nested_inner_hits_do_not_allow_object_field branch from 20ba983 to 6c46a67 Compare September 19, 2017 09:18
@martijnvg martijnvg merged commit 6c46a67 into elastic:master Sep 19, 2017
@martijnvg martijnvg removed the v5.5.4 label Sep 19, 2017
@lcawl lcawl removed the v6.1.0 label Dec 12, 2017
@clintongormley clintongormley added :Search/Search Search-related issues that do not fall into other categories and removed :Inner Hits labels Feb 14, 2018
@jimczi jimczi added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
>bug :Search/Search Search-related issues that do not fall into other categories v5.6.2 v6.0.0-rc1 v7.0.0-beta1
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants