Update the schema for the REST API specification #42346
Conversation
|
I had a look and I think the changes are a good improvement of our spec. For instance, for years it was hard to associate paths with their corresponding parts. I think though that we should keep, at least for now, multiple methods. Take bulk for instance (but this applies to many API), it supports POST and PUT for all of its paths. I would not want to see only POST in the spec unless the corresponding code supports only POST. Or rather, if we do want to make such change, I would like to make it separately from this refactoring and give it more visibility, have a separate discussions about it and so on. If you adapt that I can take a stab at updating the java code in the yaml test runner so we get a green build and we can move this forward. |
|
I've re-introduced the multiple methods for endpoint in c6193e1. |
cbuescher
added
:Core/Infra/REST API
|
Pinging @elastic/es-core-infra |
|
This is a move in a good direction for the clients team. |
| "parts":{ | ||
| "id":{ | ||
| "type":"string", | ||
| "required":true, |
There was a problem hiding this comment.
I think that we can get rid of the required flag here, now that parts are listed under their own path. All parts are then required in the path that they are part of.
There was a problem hiding this comment.
Yes, good good catch! The required property is indeed redunant in the new format, since effectively all the dynamic parts in a specific path are required.
|
heya I have adapted the yaml test runner and merged master in. Also left a comment on removing the |
|
Tests seem to be red because there are more spec files that need to be converted to the new format. I think that besides |
|
I've re-generated the JSON files for Core APIs from current master (picking up the new |
|
Jenkins, retest this please. |
javanna
force-pushed
the
rest_spec_update
branch
from
93ff5fe to
e9bd883
Compare
|
I fixed a couple of test issues I found:
and rebased the branch on current master, which had a lot of conflicts due to #44487 . |
|
retest this please |
javanna
force-pushed
the
rest_spec_update
branch
from
e9bd883 to
7eca6ce
Compare
|
run elasticsearch-ci/bwc |
javanna
force-pushed
the
rest_spec_update
branch
2 times, most recently
from
619df14 to
c282def
Compare
This patch updates the REST API spefication in JSON files to better encode deprecated entities, to improve specification of URL paths, and to open up the schema for future extensions. Notably, it changes the `paths` from a list of strings to a list of objects, where each particular object encodes all the information for this particular path: the `parts` and the `methods`. Among the benefits of this approach is eg. encoding the difference between using the `PUT` and `POST` methods in the Index API, to either use a specific document ID, or let Elasticsearch generate one. Also `documentation` becomes an object that supports an `url` and also a `description` which is a new field.
javanna
changed the title
The logic for choosing the path to use when running tests has been simplified, as a consequence of the path parts being listed under each path in the spec. The special case for create and index has been removed. Also the parsing code has been hardened so that errors are thrown earlier when the structure of the spec differs from what expected, and their error messages should be more helpful.
javanna
force-pushed
the
rest_spec_update
branch
from
c282def to
3c259e5
Compare
* Update the REST API specification This patch updates the REST API spefication in JSON files to better encode deprecated entities, to improve specification of URL paths, and to open up the schema for future extensions. Notably, it changes the `paths` from a list of strings to a list of objects, where each particular object encodes all the information for this particular path: the `parts` and the `methods`. Among the benefits of this approach is eg. encoding the difference between using the `PUT` and `POST` methods in the Index API, to either use a specific document ID, or let Elasticsearch generate one. Also `documentation` becomes an object that supports an `url` and also a `description` which is a new field. * Adapt YAML runner to new REST API specification format The logic for choosing the path to use when running tests has been simplified, as a consequence of the path parts being listed under each path in the spec. The special case for create and index has been removed. Also the parsing code has been hardened so that errors are thrown earlier when the structure of the spec differs from what expected, and their error messages should be more helpful.
* Update the REST API specification This patch updates the REST API spefication in JSON files to better encode deprecated entities, to improve specification of URL paths, and to open up the schema for future extensions. Notably, it changes the `paths` from a list of strings to a list of objects, where each particular object encodes all the information for this particular path: the `parts` and the `methods`. Among the benefits of this approach is eg. encoding the difference between using the `PUT` and `POST` methods in the Index API, to either use a specific document ID, or let Elasticsearch generate one. Also `documentation` becomes an object that supports an `url` and also a `description` which is a new field. * Adapt YAML runner to new REST API specification format The logic for choosing the path to use when running tests has been simplified, as a consequence of the path parts being listed under each path in the spec. The special case for create and index has been removed. Also the parsing code has been hardened so that errors are thrown earlier when the structure of the spec differs from what expected, and their error messages should be more helpful.
|
Thank you for the clear PR description @karmi and for the broadcast email @Mpdreamz! They were both immensely helpful for the ES UI team. I see a number of breaking changes here which impact the ES UI team's maintenance workflow for Kibana Console (addressed by elastic/kibana#43427). This isn't terribly disruptive for us, but could we please add the |
Follow up from #42346. Since the `methods` array is in order of preference when calling the index API with an `{id}` we prefer to use the `PUT` http method.
Follow up from #42346. Since the `methods` array is in order of preference when calling the index API with an `{id}` we prefer to use the `PUT` http method.
Related: elastic/elasticsearch#42346 (cherry picked from commit cbca3aa)
The purpose of this patch is to improve the schema of the Elasticsearch REST API specification.
It tries to solve a number of outstanding issues with the current specification, and to make the schema more open for future extension. It requires the consumers — eg. the client code generators — to update their code, but the long-term benefits should be worth it.
A brief overview of the changes:
methodandpathcombination.A snippet of the new schema, copied from the "Create" API, looks like this:
{ "url":{ "paths":[ { "path":"/{index}/_create/{id}", "method":"PUT", "parts":{ "id":{ "type":"string", "required":true, "description":"Document ID" }, "index":{ "type":"string", "required":true, "description":"The name of the index" } } }, { "path":"/{index}/{type}/{id}/_create", "method":"PUT", "parts":{ "id":{ "type":"string", "required":true, "description":"Document ID" }, "index":{ "type":"string", "required":true, "description":"The name of the index" }, "type":{ "type":"string", "description":"The type of the document", "deprecated":true } }, "deprecated":{ "version":"7.0.0", "description":"Specifying types in urls has been deprecated" } } ] }}The URL paths are no longer defined as a list of strings, with disconnected
partsandmethodelements outside the definition. Instead, every path now lists the HTTP method to use, and the dynamic parts it contains. It also defines whether this path has been deprecated, instead of using an extra key,deprecated_paths, as before. For the cost of a little duplication, each dynamic part can now be marked as deprecated, directly in the path definition.The dubious top-level
pathelement has been removed: the original idea was to consider it as the "canonical" path, but in practice, the specification consumers never had a good use for it, and the ambiguity betweenpathandpathscreated a lot of confusion.The
paramsandbodyelements have been left intact, as well as the recently introduceddeprecatedelement.Another breaking change is that the
documentationelement has been transformed to an object. Apart from the existingurlkey, it adds adescriptionkey, which contains a concise description of the API endpoint. These descriptions are used in the client code generators, and they have to be written carefully — they are displayed prominently on the documentation websites for various languages, for example ongodoc.org:To allow the code generators work with a freeform text like this, the description has to be a full sentence, and has to start with a verb. Multiple lines are allowed.
One outstanding question is whether we should drop support for multiple HTTP methods in the endpoint definition, as this patch does. After some thinking and conversations, I think it's the right thing to do: a mere list of methods doesn't contain any information if there's any difference (for example in case of the "Create" API), and in some cases (the "Search" API), the listed methods are included as "optional", trying to encode certain leniency in the Elasticsearch API. This is probably confusing to specification readers, and doesn't have much value in the end.
To make the updates, I've created a Ruby script to load the old JSON files (json_spec_update.rb.txt), and generate the files with a new schema. I'm using another Ruby script (json_spec_retouch.rb.txt) to "retouch" the new files, when needed.
Related:
/cc @elastic/es-clients