Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Please bring back body parameter to API calls #2181

Closed
honzakral opened this issue Mar 21, 2023 · 6 comments · Fixed by #2383
Closed

Please bring back body parameter to API calls #2181

honzakral opened this issue Mar 21, 2023 · 6 comments · Fixed by #2383

Comments

@honzakral
Copy link
Contributor

Describe the feature: Currently, when the documentation or implementation is out of sync (see #2179 , #2180 for examples) the API becomes unusable and users have to resort to using the underlying perform_request method eliminating some of the benefits of having a dedicated API (parameter checking, URL composition, type hinting, ...). It would be useful to still be able to pass in the entire body in those cases.

It would also be great for the common use case of getting an object from Elasticsearch, modifying it, and reapplying it to the cluster, for example index templates, ILM policies, and other configurations. This cannot be now easily done with **body since there are some manually introduced changes like _meta in the JSON body as opposed to meta in the python API. This makes the API extremely brittle for such cases.

@honzakral
Copy link
Contributor Author

Another case is point is inconsistencies like put_settings which doesn't follow the patttern of accepting a kwarg for every key in body but instead accepts a body parameter that is undocumented and renamed to settings. This is incredibly confusing

@sethmlarson
Copy link
Contributor

(I'm aware you know most of the history here, want to give context to other readers)

In the past our clients had the body key because we had no specification for HTTP request and response bodies so we necessarily had to be generic with the request body parameter from v1.x to v7.x of the client. We had information about path and query parameters so we were able to give helpful parameters for those parts of the API, but not the request body.

This is unfortunate because this meant parameters that were from the Elasticsearch application layer (_source, track_total_hits) were mixed in with a parameter that is from the HTTP/transport layer (body).

Starting in late 7.x timeframe we have an API spec for request bodies and are able to distinguish between a request body which is a set of parameters (ie indices.create has mappings, settings, etc) and where the entire request body is a parameter (the "create document" API has document={...}). All 8.x Elasticsearch clients use this API spec and give semantic names to the parts of a request body.

Currently, when the documentation or implementation is out of sync the API becomes unusable and users have to resort to using the underlying perform_request method

I think we should aim to fix the API specification in these cases since the fixes there bubble down to all other language clients, docs, and other consumers.

It would also be great for the common use case of getting an object from Elasticsearch, modifying it, and reapplying it to the cluster, for example index templates, ILM policies, and other configurations. This cannot be now easily done with **body since there are some manually introduced changes like _meta in the JSON body as opposed to meta in the python API. This makes the API extremely brittle for such cases.

The Python client automatically handles both _meta= and meta= in order to support this use-case since leading underscores have a special meaning in Python.

I know there's another common case of having a request body in a JSON file, I've recommended using ** for these cases as well. Semantically then the entire API request is being encoded as JSON rather than a single piece of the API request. You're giving us all the parameters and values and we figure out where best to serialize them.

Another case is point is inconsistencies like put_settings which doesn't follow the pattern of accepting a kwarg for every key in body but instead accepts a body parameter that is undocumented and renamed to settings.

We can't hard-code all parameters in this case because index settings can be user-controlled/custom due to plugins. On the undocumented front (I'm assuming here you're talking about RTD) I'm confused why the settings parameter is not appearing in the autodocs since it's clearly a part of the function signature. Will look at why that's not getting populated.

Happy to discuss more about any of the above.

@honzakral
Copy link
Contributor Author

On the undocumented front (I'm assuming here you're talking about RTD)...

I am talking anywhere - currently there are well over 300 undocumented parameters. The change from body to various other keys (be it just a simple alias like put_settings, index, or decomposition like with put_index_template) has not been documented and realistically the only way to find out how to translate the documentation examples in the Elasticsearch documentation to a functioning Python code is to read the code of the client and understanding the mechanism of elasticsearch's API.

I understand the arguments you are making (though I do not share the same view) but to make those changes without documentation, without supplying Python examples in the API documentation (despite having the UI suggesting there would be examples in Python) renders the client de facto undocumented. Keeping the body argument would at least preserve the option of using the official elasticsearch documentation to create a functioning Python code.

btw another reason to have the body is performance - in the past I passed in already json-encoded string to body which provided significant performance benefits in niche cases

Ultimately this is of course your decision but please consider adding the body parameter back as an addition to your approach. What I would suggest is adding it and then raising an exception if it is used together with any of the decomposed arguments - that way you can provide your more granular API as well as an easy escape hatch to be used for backwards compatibility and/or any above mentioned use cases. It would not increase the complexity of the code, or the documentation, in any meaningful way and would allow people to use the client more effectively.

@sethmlarson
Copy link
Contributor

Discussed this with the team and only the Python client doesn't have some way to load JSON into a request. We also discussed the common use-case of people iterating on and refining their query in Kibana Dev Tools before wanting to put that request into use with a client library (and potentially further refining their query). If folks have to translate between the two the Python client not supporting body would be a point of confusion.

For these reasons we've elected to un-deprecate body support in the Python client.

@pquentin
Copy link
Member

elasticsearch-py 8.10.1 removed the deprecation warning! https://github.com/elastic/elasticsearch-py/releases/tag/v8.10.1.

What is left to do is allowing any body, not just the ones we think are valid according to https://github.com/elastic/elasticsearch-specification/ (which is not always fully up-to-date and does not account for custom plugins). The current workaround is to call client.perform_request.

@pquentin
Copy link
Member

This now fixed in the main branch and will be available in the next elasticsearch-py minor release, 8.12.0, likely early next year.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants