body removal causes 400 - illegal_argument_exception

[afharo]

Regression report

In Kibana, we're migrating to stop using the deprecated body key. An example is elastic/kibana#204882.

However, some of our tests started failing with the following error:

illegal_argument_exception: request [/<data_stream>/_mapping] contains unrecognized parameter: [_data_stream_timestamp]

We managed to ship this because the code tries to update some existing mappings by GET the existing ones and PUT the full updated object. _data_stream_timestamp was automatically populated by ES and returned in GET request.

The old logic using the body param works because it forces the client to ship that parameter in the body of the request. However, after removing the body param, the client sent that property as a query parameter, leading to ES validation issues as this property is only accepted in the body.

Last working version

8.16.0

To reproduce

Try to update the mappings of an existing data stream by GET-ing the mappings first, adding the new field in the mappings, and PUT-ing the resulting object.

Expected behavior

The request is successful.

Node.js version

v20.18.2

TypeScript version

5.1.6

Elasticsearch client version

@elastic/[email protected] - @elastic/[email protected]

Elasticsearch server version

9.0.0-SNAPSHOT

Operating system

No response

Any other relevant environment information.

No response

[rudolf]

Just want to highlight the use case where this surfaced:

https://github.com/elastic/kibana/blob/main/x-pack/platform/plugins/shared/alerting/server/alerts_service/lib/create_concrete_write_index.ts#L73-L95

Here Kibana makes a simulate index template request to retrieve the mappings that an index template would apply if a data stream write index was rolled over. It then uses the mappings from the response to also apply these mappings to existing backing indices (presumably to synchronise the mappings?).

Since the simulate index template request responded with _data_stream_timestamp which is undocumented and not in the spec the update mappings requests started failing. Since Elasticsearch can add new response properties at any time to their API without it being considered a breaking change this is a significant risk, a minor upgrade of Elasticsearch can suddenly cause Eleasticsearch-js client calls to start failing.

While it would be a risky and breaking change to Elasticsearch-js, it seems like defaulting to passing unknown properties in the body would preferably over passing them as query params.

This would however break any consumers of Elasticsearch-js that are using query params not in the spec or belonging to a custom plugin (related elastic/elasticsearch-py#2181)

[JoshMock]

I do agree that passing unknowns to the body makes more sense than passing as query params. The main concern is, as you said, breaking any existing code that works on the assumption that unknown params will end up in the querystring.

My only other concern with using the body as the catch-all place for unknown params is that there are many requests that do not accept a JSON body. In those cases, query params probably are the better choice. It might be possible to update the code generator to be able to choose the ideal catch-all for both of these cases, but I'm not confident I'd have something stable in time for 9.0. I can look into it next week but it will be a tight squeeze.

@rudolf @afharo would making this change introduce even more last-minute pain for Kibana, or do you think it will be a smooth transition? If it's painful, maybe holding off is the better choice.

[afharo]

would making this change introduce even more last-minute pain for Kibana, or do you think it will be a smooth transition? If it's painful, maybe holding off is the better choice.

elastic/kibana#208776 proves that not doing this change is blocking Kibana from removing all body usages. Especially in the putMappings API. While the indices.putSettings wraps all the settings under a property settings, putMappings expects al the params in the root level, and, apparently, there are many root-level mappings fields that are not in the spec. Maybe a safer approach for this API is to wrap the properties inside a top-level mappings field.

WDYT?

[JoshMock]

Maybe a safer approach for this API is to wrap the properties inside a top-level mappings field.

Making a spec change like this is a discussion that would need to happen in the spec repo. Because all request types are derived from the spec, for all language clients, that change has to be something that will be compatible with all dynamic and static clients work.

elastic/kibana#208776 proves that not doing this change is blocking Kibana from removing all body usages

Which change? Adding body and querystring parameters to each request? Or changing the catch-all to body for most requests? The former was released in alpha.3 last week; the latter I have not yet implemented because I've been working on #2605, which was deemed a high priority.

[JoshMock]

I'm going to work on changing the catch-all from querystring to body (except for APIs that don't accept a body) soon and get it published in the next 9.0.0 alpha.

[JoshMock]

As of #2636 the param logic changes to:

1. if recognized as a body param from the spec, put it in the JSON body
2. if recognized as a path param, put it in the URL path
3. if recognized as a query param or a "common" query param (e.g. pretty, error_trace), put it in the querystring
4. if not recognized and this API accepts a JSON body, put it in the JSON body
5. if not recognized and this API does not accept a JSON body, put it in the querystring

The first two steps are identical to 8.x. The last three steps replace the logic that put all unrecognized params in the querystring.

The change also updates each API function to only instantiate its arrays of accepted body/path/query param names once per client instance rather than during every function call, by moving the array values up to the constructor or module, depending on which is available. Hopefully that introduces a small performance/memory improvement.

I'm going to merge that PR today and publish 9.0.0-alpha.4 to npm shortly after.

[JoshMock]

9.0.0-alpha.4 is now available on npm @afharo @rudolf