Recovery API supports details param not detailed

[javanna]

As part of #28878 we discovered that the recovery API accepts a detailed parameter, also documented, that is unused. The parameter should rather be called details (see https://github.com/elastic/elasticsearch/blob/master/server/src/main/java/org/elasticsearch/indices/recovery/RecoveryState.java#L918), although it's no longer accepted by the recovery API since we introduced parameters validation. Also the detailed flag is transported to the response, without being serialized nor printed out, where it should be removed.

We should make this work, by either accepting the details param instead and updating docs and spec or modifying RecoveryState to look for detailed instead of details. The detailed flag though doesn't need to be part of the response, and does not need to be part of the request either, given that RestRequest is provided as Params argument to the toXContent method.

[olcbean]

In a few rest apis detailed flag is used :

elasticsearch/server/src/main/java/org/elasticsearch/rest/action/admin/cluster/RestListTasksAction.java

Line 72 in e7d1e12

boolean detailed = request.paramAsBoolean("detailed", false);

And details is used as a filedname in few responses :

elasticsearch/server/src/main/java/org/elasticsearch/rest/action/search/RestExplainAction.java

Line 141 in e7d1e12

static final String DETAILS = "details";

elasticsearch/server/src/main/java/org/elasticsearch/rest/action/search/RestExplainAction.java

Line 121 in e7d1e12

builder.startArray(Fields.DETAILS);

Maybe it would be more consistent to change the details to detailed ?

[elasticmachine]

Pinging @elastic/es-core-infra

[elasticmachine]

Pinging @elastic/es-distributed

[olcbean]

We should make this work, by either accepting the details param instead and updating docs and spec or modifying RecoveryState to look for detailed instead of `

I think that it would be better to go the second way : use detailed as a url parameter as done in other requests ( basically go with the documentation - similar to what was done for field_data vs fielddata for clear cache api )

@PnPie @javanna what do you think ?

[javanna]

@olcbean I am not sure, you would go that way because the param was accepted before as detailed although it had no effect? On the other hand it will have some effect now, I wonder if any user ever use that parameter.

[PnPie]

Yes, and even if there were users using detailed flag before it wouldn't work right ? because in RecoveryState we parsed it as details.

[javanna]

@PnPie one could say there is a subtle difference between not doing anything and throwing error. Probably the latter is better than the former at this point, at least you know that something does not work upfront.

[olcbean]

Sorry, I think that I was not really clear. Let me try to re-elaborate.

What I meant is that at this case it could be better to go with the documentation and stick with the detailed ( as controversial as it may be, sometimes it could be better to go with the documentation over the concrete implementation ;) )

Without digging too deep, a quick search through the resp-api-spec shows that detailed parameter is used in several apis and details in none.

Maybe making detailed work as expected would be more consistent. Am I missing something ?

[javanna]

thanks for the explanation @olcbean ! I am fine either way, @bleskes you reviewed the PR associated to this issue, do you have an opinion on this?

[PnPie]

@olcbean Yes, I agree with that (keep it the same as others), and according to all we discussed, I'm more in favor of keeping it as detailed.

[javanna]

agreed @PnPie, I have updated the description of this issue accordingly.