Strict json parsing improvements

[javanna]

We currently support the index.query.parse.strict setting that allows to turn on strict parsing, so that when deprecated keys are used elasticsearch returns an error. This was thought initialy for queries but got also expanded to anywhere we have json. Unfortunately the name of the setting doesn't reflect what it does anymore, as it can be used in the context of snapshots, aggregations, scripts etc. Let's discuss whether we should rename this setting of have separate settings for each feature. What do people think about this?

Also, I think it would be nice to improve this feature by collecting different errors and printing them all out at the same time rather than throwing exception at the first problem found. Or maybe add the possibility to enable logging deprecation usages rather than throwing exception (both can still be supported). Thoughts?

[colings86]

+1 to this.

Personally I think it should be one setting for the search request in general since I don't see a reason why you would want to be notified of deprecated keys in your queries but not in your aggregations, etc.

It would be great to collate the error and print them all at once as this would save users lots of time when debugging syntax problems

[clintongormley]

Actually, that setting was added for percolator queries and alias filters, where creating a query on an unmapped field led to it assuming the field was a string. i think the setting was added because a user complained about the change. Honestly, what they are doing is broken and I think we shouldn't support their particular use case. I do wonder if the need for this has been fixed by #11930? (at least for aliases if not for percolators?)

I think strict field lookups is distinct from warning/throwing exceptions for the use of deprecated functionality. This issue deals with fieldname lookups: #12016

But yes, I'm in favour of having a setting that controls whether the use of deprecated functionality throws an exception, warns to the deprecation log, or is ignored (see #8963 for the original suggestion)

[javanna]

Thanks for pointing this out @clintongormley I had a look at the codebase and this is what I found.

I do see in the history that index.query.parse.strict and index.query.parse.allow_unmapped_fields were introduced at the same time in the context of percolation and aliases. That said index.query.parse.strict was hooked into the existing ParseField infra as far as I can see, to enable/disable deprecation exceptions through it, which is the only thing that this setting does at the moment.

The error message that we are used to in the context of aliases and percolation "Strict field resolution and no field mapping can be found for the field with name..." depends on the value of the index.query.parse.allow_unmapped_fields setting though, nothing to do with the strict query parsing setting. Maybe @martijnvg can confirm on how we use these two settings.

I think it is worth at this point to introduce a new (more generic) setting to enable deprecation exceptions, which is not just a boolean but allows to either throw exception or log deprecation warning by using the deprecation logger that we now have.

[martijnvg]

The index.query.parse.allow_unmapped_fields setting can fail a search request if no mapping for a field can be found. The percolator relies on this setting, because it parse the percolator queries at creation time and a mapping misconfiguration can lead to unexpected and difficult experience during percolating. Also aliases still uses the index.query.parse.allow_unmapped_fields setting. I did not disable this as part of #11930, even though aliases are now parsed at search time. It could be disabled, but I didn't do that, because it felt to me it is good to know that an alias filter is incorrect at creation time rather then at search time, since the alias filter isn't defined in the search request it self and this makes things confusing. It felt to me that this behaviour should be changed in a different change (#11806, but this was later superseded by #12016)

The index.query.parse.strict was always there and deals with deprecated json format. The setting was there before strict parsing was added to the percolator and alias fields. This setting was just changed into a constant as part of the strict parsing change.

The index.percolator.map_unmapped_fields_as_string setting was added, because users in certain percolator scenarios (in cases when fields aren't known beforehand) did prefer flexibility over strictness and like the name suggests treats unmapped fields as string fields.

IMO all these settings are completely obsolete when #12016 gets added and the index.query.parse.allow_unmapped_fields and index.percolator.map_unmapped_fields_as_string should be removed as part of that change.

[clintongormley]

The index.query.parse.strict was always there and deals with deprecated json format. The setting was there before strict parsing was added to the percolator and alias fields. This setting was just changed into a constant as part of the strict parsing change.

Ah ok - i got the two mixed up.

IMO all these settings are completely obsolete when #12016 gets added and the index.query.parse.allow_unmapped_fields and index.percolator.map_unmapped_fields_as_string should be removed as part of that change.

I'm not sure I agree completely here (but maybe I'm missing something). As I understand it:

• alias filters are now parsed at execution time so they don't need the strict query parsing any more
• however percolators are still parsed only once, and so are susceptible to the same problems as before.
• the map_unmapped_fields_as_string setting was added for that one user's use case, which i still think is broken and i'm very tempted to remove it

[martijnvg]

alias filters are now parsed at execution time so they don't need the strict query parsing any more

Ok, lets disable the strict parsing for alias filters.

however percolators are still parsed only once, and so are susceptible to the same problems as before.

True, we still need that, I made my conclusions too quick.

the map_unmapped_fields_as_string setting was added for that one user's use case, which i still think is broken and i'm very tempted to remove it

is there a bug with map_unmapped_fields_as_string setting?

[martijnvg]

I opened #12150 for alias filters.

[clintongormley]

is there a bug with map_unmapped_fields_as_string setting?

Not that there is a bug, just that their use case "assume all unmapped fields are strings" seems pretty broken. Why would you create a percolator for an unknown field?

(While we're on it, a wildcarded field name in a percolator query suffers from a similar problem: it would just take into account fields that exist when the query is instantiated)

[javanna]

my proposals are now formalized as part of #8963 and we also have #12179 for percolator queries, I think we can close this issue then.