Skip to content

[OpenAPI] Merge multiple paths in a single operation #4415

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

Draft
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

swallez
Copy link
Member

@swallez swallez commented May 26, 2025

Fixes #4277

Adds an option in the OpenAPI export to group the many operations resulting from endpoints with multiple paths in a single operation. This works around the current issue where there are many duplicated operations in the nav bar and in the documentation.

A "main path" is chosen to become the path of the single OpenAPI operation that is produced. This is the endpoint's longest path, which will show all path parameters, be them required or optional. All paths are then listed at the top of the description (including the main one), see rendering below.

WIP: do not merge this PR now. This option must be configurable, as it is destructive from an OpenAPI schema perspective since it removes some operations.


Search endpoint rendered by Bump with this PR:

image

@lcawl
Copy link
Contributor

lcawl commented May 26, 2025

Looking at the current output, for example:

  "description": "**All methods and paths for this operation:**\n\n<div><operation-summary>\n 
<span class=\"operation-verb get\">GET</span>\n 
<span class=\"operation-path\">/_analyze</span>\n
</operation-summary></div>\n
...

IMO if it make it more usable in general to just accomplish this list via simple markdown, that's okay too. For example:

"Valid methods and paths for this operation include:\n
- `GET /_analyze`\n
- `POST /_analyze`\n
- `GET /{index}/_analyze`\n
- `POST /{index}/_analyze`\n
...

@lcawl
Copy link
Contributor

lcawl commented May 26, 2025

WIP: do not merge this PR now. This option must be configurable, as it is destructive from an OpenAPI schema perspective since it removes some operations.

I see 179 operations where this new text appears in the OpenAPI document, so it's a pretty impactful change. It's not something we ideally do manually, but I think it will be necessary to confirm that:

  1. none of the operations that have been removed are ones that are affected by overlays (or if they are, that the overlay is moved to the appropriate consolidated operation), and
  2. none of the operations that have been removed are target URLs (e.g. from https://github.com/elastic/elasticsearch-specification/blob/main/specification/_doc_ids/table.csv, https://github.com/elastic/kibana/blob/main/src/platform/packages/shared/kbn-doc-links/src/get_doc_links.ts, or I guess even from docs in general... which would imply that in addition to cleaning up their usage we'll likely need to create redirects just in case we don't catch all usages).

For example, if we need to create redirects, it will require information like this:

Old URL New URL
https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-async-search-submit-1 https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-async-search-submit
https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-bulk-1 https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-bulk
https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-bulk-3 https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-bulk
https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-bulk-2 https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-bulk
https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-watcher-execute-watch-1 https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-watcher-execute-watch
https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-watcher-execute-watch-2 https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-watcher-execute-watch
https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-watcher-execute-watch-3 https://www.elastic.co/docs/api/doc/elasticsearch/operation/operation-watcher-execute-watch

@swallez
Copy link
Member Author

swallez commented May 28, 2025

IMO if it make it more usable in general to just accomplish this list via simple markdown

The generated markup uses the CSS classes used by Bump in the main path description to provide a consistent rendering.

I've added a commit that removes the <operation-summary> that doesn't have any benefit in this context. The remaining <div><span> still make it valid Markdown and will be rendered correctly even outside of Bump. Now can also produce plain Markdown if you prefer.

I see 179 operations where this new text appears in the OpenAPI document, so it's a pretty impactful change

Yes. As we already discussed, it cannot replace the current OpenAPI file. It has to be a new one, dedicated to the docs pipeline.

none of the operations that have been removed are target URLs

So I understand we're good on that front?

Some background: when we generate one operation for each method+path, the first one (as found in schema.json) has a "plain" (no suffix) identifier, and the others have a counter suffix (-1, -2, etc). When merging multiple path, all these suffixed operations are removed, and we only have the unsuffixed operation.

@flobernd
Copy link
Member

Good to know there will be a dedicated OpenAPI file for the docs. This should also make it trivial to convert GHF markdown admonitions to the bump.sh syntax.

@lcawl
Copy link
Contributor

lcawl commented May 29, 2025

Some background: when we generate one operation for each method+path, the first one (as found in schema.json) has a "plain" (no suffix) identifier, and the others have a counter suffix (-1, -2, etc). When merging multiple path, all these suffixed operations are removed, and we only have the unsuffixed operation.

That's great and should make it easier to check we don't have any of those suffixed URLs used elsewhere.
Looping in @georgewallace and @szabosteve so they can weigh in on the outstanding questions around whether it's preferrable to use plain markdown for the list of alternatives and when/if we want to do redirects and check for impact to overlays, link services, etc per #4415 (comment)

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

Successfully merging this pull request may close these issues.

Unique summaries per path
3 participants