Should we rework strict parsing?

[nik9000]

I was looking into failing the build when we accidentally use deprecated syntax in the docs (a fairly annoying problem to users) and I noticed (again) that we have index.query.parse.strict. It is supposed to fail any request that uses deprecated syntax. But we don't really test it so far as I can tell. We test the ParseField supports strict matching but not the setting that enables it from the outside.

So I played with the setting and I'm pretty sure it doesn't work at all now. At least, not in any way I expect it to work. You can't set the setting in elasticsearch.yaml. You can't set it in index settings. You can't set it on a url. This seems bad.

I'm not really sure what we should do about it too. Personally, I'd love to be able to control the setting on a request level and the cluster level. That'd be super useful in the docs because you could turn it on in elasticsearch.yaml and then fail all the deprecated requests unless they were marked as "deprecated" somehow. No more deprecated syntax sneaking into the docs.

But there is also the index level - we use it for things like "is it ok to use off instead of false?" Should the setting live at the index level too? If it does does it only affect mappings and settings on that level or does it affect all interactions with the index? What happens if you do a search across multiple indexes that have different values for the setting? This all feels like overkill for this setting.

To top it all off the way this setting flows around the code is kind of odd. We have ParseFieldMatcher which holds the setting and extracts it from a Settings instance. We aren't particularly careful with which Settings instance we build the ParseFieldMatcher using. Usually it is the global one but because we build ParseFieldMatcher a ton of times in the code it is difficult to tell for sure.

[nik9000]

Relates to #17512 and #8963.

[javanna]

I am afraid that we forgot to migrate index.query.parse.strict to the new settings infra. I think we should make it a blocker to fix that so that the setting can be provided. Renaming it to make it more generic should be considered too I think.

[clintongormley]

@javanna should we even have this setting anymore? what does it do?

it looks like it was first added here bc5a9ca#diff-bb1674dc350eba2cee9dbef9525e22c5R120

[clintongormley]

It seems to have a number of different uses, some of which should just be defaults (eg queries must end with }) but we could use it as a flat to upgrade deprecation warnings to exceptions.

[javanna]

I thought that is the only chance we have to fail requests with deprecated syntax, or at least what ParseField uses. Unfortunately what it does is very different from what it should do at the moment (it got lost during the settings migration I believe).

[javanna]

and the point in favour of renaming it is that we use ParseField in a lot more places than just query parsing. We should then discuss whether we want to be able to enable failing requests with deprecated syntax per request, like Nik proposed in this issue. But a setting seems the way to go to control it at the cluster level?

[nik9000]

I think we should have some setting that people can set to true in development that fails requests that have deprecated syntax. They can override it on the request level. They can set it to false in production so it never fails anything and they just catch these problems in dev (hopefully). I'd certainly have used it.

If this setting, or some version of it is the way to go then great. I'm a bit skeptical because I see us doing stuff like calling DEPRECATION_LOGGER.deprecated directly in places and this settings wouldn't catch it. I'd love to be able to set something in the thread context that just fails the request if you call DEPRECATION_LOGGER.deprecated. Or add something to the respond processing to turn the status code to 400 if you have any Warning headers. Or something. These are my first instincts though.

I don't know that it is that simple though as part of the problem with deprecated syntax is deprecated mappings and settings. The whole "override on the request" is much harder to do there.

[nik9000]

Somewhat related discussion: #19504 (comment)

[javanna]

Taking here some of the discussion we are having in #19504 , now that we always return deprecation warnings as response headers I tend to agree that we should consider removing the index.query.parse.strict setting and either remove the notion of failing a request if it contains deprecated syntax, or find another way to achieve that. At the moment the strict parsing and deprecation warnings are too decoupled, one can easily use one without the other. Strict parsing requires to use the ParseFieldMatcher which is a construct that I introduced but I never really liked, it is confusing and I would be more than happy to get rid of it.

I am really tempted to just clean stuff up first, then we can see whether we still need strict parsing and how we can implement it differently with the infra that we have now. Maybe we can simply piggy back on the deprecation logger as the central place for doing anything around deprecated syntax.

Good news is that it is not such a breaking change given that we never documented the setting, plus users cannot even set the setting I think at the moment :)

[javanna]

We discussed this in FixItFriday. Everybody agreed that we should remove the index.query.parse.strict setting. This setting is undocumented and not settable at the moment within a node; the only place where it has some effect is in our unit tests (e.g. AbstractQueryTestCase) to make sure that we don't use any deprecated syntax in our tests and that we test deprecations separately. This part needs some replacement in our test infra but we don't need a setting for that.

Removing the setting would remove the need for ParseFieldMatcher (as well as ParseFieldMatcherSupplier) in our codebase and simplify our parsing code.

A separate discussion will need to happen later on whether we want to introduce a new setting to make a request fail in case deprecated syntax is used. Clients could already implement that based on warning response headers. We have to double check if there are deprecation warnings that don't get returned as response headers but only logged, and what should happen for those if any.

[nik9000]

Hurray! Thanks for all the work @javanna!