Stricter `UpdateSettingsRequest` parsing on the REST layer

[olcbean]

In UpdateSettingsRequest every property is interpreted as a setting name. For example :

{
  "field3" : "value3"
}

will be parsed as the property field3 with value value3 ( similar to the ClusterUpdateSettingsRequest ).

But if there is a top-level key "settings" for a map

{
  "field1" : "value1",
  "field2" : "value2",
  "settings" : 
  {
    "field3" : "value3"
  }
}

then only "field3" : "value3" is parsed as a setting in the UpdateSettingsRequest#fromXContent and the other properties are silently ignored. ( I could not find a reference to this in the documentation )

Do we want such leniency in the UpdateSettingsRequst ?
Do we want to keep on supporting the possibility to wrap the settings as a "settings" map ?
Or should this be deprecated in 6.x and removed in 7.0 ?

[elasticmachine]

Pinging @elastic/es-core-infra

[jasontedor]

I think we should make the same transition that was made in #23869: hard break in 7.0.0. It is a bug that we accept and handle requests as you describe.

[olcbean]

In #23869 the top-level keys were checked and "enforced".

Here I think that it would be better to remove the top-level key ( as only settings are expected by the api so there is no need for grouping ) . The only other similar api is the cluster update settings api where we have "transient" and "persistent" as top-level elements but no "settings".

In other words

{
  "field1" : "value1",
  "field2" : "value2",
  "settings" : 
  {
    "field3" : "value3"
  }
}

• in 6.x should result in a warning and accept only "field3" : "value3" as a setting
• in 7.0 there are two options :
  • throw a parsing exception ( as "settings" is detected at the top-level )
  • parse all three settings with no special handling for "settings" : field1, field2 and settings.field3 ( this should throw during validation )

@jasontedor what do you think ?

[olcbean]

@jasontedor I would prefer to go with the second option in 7.0 : no special handling for "settings" at the top-level. What do you think ?

[gwbrown]

I can confirm just looking at the parsing code that this is still an issue as of 7.10.0 (and master at that time as well).

This is an easy fix code-wise whatever solution we decide to use, but I'm going to tag this team-discuss to attempt to come to a conclusion as to what should be done (at which point this becomes a good candidate for good first issue).

As whatever fix for this is a breaking change, we should also detect this condition in and emit a deprecation warning for [next minor], with the change itself in [next major].

[rjernst]

We discussed this last week and agreed it is still something we want to do. I'm marking this for anyone to pick up.