Enforce _parent field resolution to be strict

[martijnvg]

A type defined in a has_child query must have a _parent field.
A type defined in a has_parent query must have child type with a _parent field pointing to it.

PR fixes #9461 too. The query parsing failed sometimes with a unexpected NPE, because the _parent field is pointing to a non existing type. The consistent strict _parent field resolution makes sure the NPE can't occur any more.

This PR also includes a commit that removes the old and used _cache and _cache_key options. (master only)

[jpountz]

Should it reuse index.query.parse.strict instead of introducing a new setting?

[martijnvg]

Yes, reusing is better than introducing a new setting. The strict setting deals with a missing or incorrect configured _parent field (_parent field pointing to a different type than was intended). While index.query.parse.strict deals with old invalid json keys, that is why I don't find it the best option. Maybe strict isn't good a name...

Perhaps index.query.parse.allow_unmapped_fields should be reused? This feels closer to what I'm trying to do here.

[jpountz]

Perhaps index.query.parse.allow_unmapped_fields should be reused? This feels closer to what I'm trying to do here.

+1 to reuse, the use-case looks very similar to me. We could think about renaming it to something more generic in the future.

[martijnvg]

We could think about renaming it to something more generic in the future.

+1 I'll do that in a different issue.

Also I think we should have the same behaviour for the nested query too.

[clintongormley]

I'm not sure about this change. Why can't we just say: if there is no defined _parent, return no docs for that index/type?

It'd be the equivalent of ignoring documents that don't have a mapping for the field we're searching on.

[martijnvg]

@clintongormley Whether we return no documents depends on how the has_child/has_parent get used in the query dsl (e.g. boolean should vs. boolean must clause), but I see what you mean...

Actually the behaviour I'm after is similar to the indices query, but instead based whether an index exists it is based on whether the _parent field exists. Maybe instead there should be a no_parent_field_query option (similar to no_match_query option on indices query) on the has_child/has_parent? So if there is no _parent field and this option isn't specified then we fail with an error, but if it is specified we return docs based on the no_parent_field_query option.

[jpountz]

It'd be the equivalent of ignoring documents that don't have a mapping for the field we're searching on.

I think there are use-cases for both. When you have a well-defined schema, it can be desirable that anything that targets a field that is not in the schema is rejected. So somehow I wish we had a single parameter that would help fail in such cases as well as when a referenced _parent does not exists like Martijn proposes?

[clintongormley]

@jpountz isn't this the strict query parsing that was added for filtered aliases/percolators? This could be made configurable on a per-query basis (defaulting to off).

[martijnvg]

@jpountz @clintongormley What about adding _unmapped_fields_query option to queries where it makes sense? (similar to how the no_match_query on the indices query)

Not specifying the _unmapped_fields_query will result in parse errors if fields don't exist, but when the _unmapped_fields_query is specified it will substitute the failing query.

This will work a bit different than how the index.query.parse.allow_unmapped_fields setting works, but I feel just this option isn't flexible enough?

[jpountz]

@martijnvg The idea sounds reasonable, but we already have lots of parameters that apply to queries (names, boosts, etc.) and I'd like to only add new options if they have strong use-cases. I think it would already be great progress to have a single per-request flag that allows to control whether to be lenient in terms of unmapped fields (including missing parents).

[martijnvg]

@jpountz That is a valid concern, so lets start by adding a top level field to the search api called _unmapped_fields that support the following values: allow and disallow (default)? This new option should default to index.query.parse.allow_unmapped_fields. Maybe as part of the work we should change the index.query.parse.allow_unmapped_fields option into index.query.parse.unmapped_fields with the same values as this new request flag?

[rjernst]

Maybe as part of the work we should change the index.query.parse.allow_unmapped_fields option into index.query.parse.unmapped_fields with the same values as this new request flag?

+1. I would also rather use the values ignore, warn and error then (and preferably default to error although I know that is probably contentious).

[martijnvg]

@rjernst I like those values, but just wondering about the warn value. Should that just be like ignore, but then also log a warning message?

[rjernst]

@martijnvg Yes, that was my intention with having it there.

[martijnvg]

@rjernst Cool, that makes sense to me too.

So what I plan to do is remove the strict option from this PR and make has_child/has_parent fail if no _parent field can be found with a parser error (not silently fail). This will also fix the build failure reported via #9461

Then in a followup issue/PR I'll this top level _unmapped_fields option to the search api and refactor index.query.parse.allow_unmapped_fields option into index.query.parse.unmapped_fields option with the discussed values.

[martijnvg]

@jpountz @rjernst I updated this PR. The strict option has been removed and force type resolution for has_child and has_parent to be strict.

[martijnvg]

Closing this PR as it is no longer relevant. (#12016)