Repository navigation
[PROPOSAL] Replace Smithy with a native OpenAPI spec #189
Description
In the past discussions on Smithy vs OpenAPI [1] [2], we decided to have OpenAPI as the official spec of OpenSearch but use Smithy to describe the API as it's easier to organize:
- We won’t use Smithy features that are not translatable to OpenAPI.
- Any update to the Smithy spec must always be reflected in the OpenAPI spec.
- Smithy can be dropped in the future if necessary, allowing contributors to update to the OpenAPI spec directly rather than through the Smithy spec.
After a year of going with this strategy, as we've learned a lot more about the quirks of OpenSearch API that are not compatible with Smithy's many guardrails, writing the spec in Smithy is becoming more challenging:
1. Smithy does not support certain data types for path and query-string parameters
Many OpenSearch operations have URL parameters that accepts either a single value as a string, an array of values separated by commas. That is, these parameters have string | string[] as data type. This can be described in Smithy as a union. However, Smithy doesn't allow unions in its URL parameters (It also doesn't allow arrays/lists in path parameters specifically).
WORKAROUND: represent the data type as a string in Smithy but mark it with xDataType('string | string[]'). If the member of the list is not generic, we must also include xArrayItems('MemberName'), and xEnum([...]) when applied.
2. Smithy does not support untagged unions.
Only tagged unions are supported, which enforce a wrapping structure, while we have fields that are for example int | boolean. This would map to a oneOf in OpenAPI.
3. OpenSearch operations cause URI Conflict errors in Smithy
For example, since operations regarding an index or a list of indices have URI starting with /{index}/..., other operations, say GET _cat/indices, cause URL Conflict for Smithy as it sees _cat as an {index} parameter.
WORKAROUND: Mark every operations with @suppress(["HttpUriConflict"]) and every path parameter with @pattern("^(?!_|template|query|field|point|clear|usage|stats|hot|reload|painless).+$") and @xPattern("")
4. Unable to customise inlining and $ref naming strategy in OpenAPI conversion
If we are able to customise this strategy we can stop certain “simple” types from always being inlined in the OpenAPI conversion, and we can implement a custom naming scheme for the $refs such that we can name them to match the ElasticSearch spec we inherit. (e.g. #/components/schemas/_type:Duration)
5. Smithy mixins are always flattened/erased.
We’re unable to easily represent an inheritance hierarchy within Smithy that is translated to OpenAPI. Potential solution by introducing flag in translator that somehow retains mixins and puts them into allOf in OpenAPI.
In short, Smithy, with its many guardrails, is unfit to describe OpenSearch API:
- We can't correctly describe OpenSearch API with Smithy
- Translation from Smithy to OpenAPI is not lossless
- There are too many duck-tape workarounds to circumvent Smithy guardrails
The many gotchas, and conventions we have to establish (as seen in the DEVELOPER_GUIDE) also deter people from contributing to this project. People maintaining this repo, on top of understanding OpenAPI, must also learn Smithy and the workarounds to make it work with OpenSearch API.
Given the difficulties in mapping OpenSearch's API to Smithy, I propose we instead maintain our API definitions directly in OpenAPI. Smithy a great tool for creating new APIs thanks to its many guardrails, but OpenSearch API has many unconventional traits that are simply incompatible with it.
Activity
dblock commentedon Feb 8, 2024
nhtruong commentedon Feb 8, 2024
We will definitely lose all the features that are unique to Smithy. While its many guardrails are not compatible with existing OpenSearch API, they are actually great for new endpoints and new applications. Many of Smithy traits do have counterparts in OpenAPI and we can leverage OpenAPI extensions to cover the missing ones.
That might solve Bullet point number 2. But looks like it's still a draft for a design from Dec 2022? Though I see that restriction in Smithy as a necessary guardrail to prevent conflicting endpoints, so I didn't think of asking Smithy to "correct" it as it's not a bug to me. It's just not compatible with OpenSearch existing endpoints.
reta commentedon Feb 9, 2024
👍 to go OpenAPI full speed, Smithy gets its credits but the "recent" updates to OpenAPI spec (3.1) are buying us much more (specifically, JSON Schema support)
Tokesh commentedon Feb 10, 2024
Its sounds logical that we simply and write directly in OpenAPI.
I have encountered the two problems described above. It would be interesting to try writing directly into OpenAPI without the extra layer of Smithy.
VachaShah commentedon Feb 13, 2024
Just trying to understand that the issues we are facing currently with Smithy are entirely related to how Smithy works or are they because OpenSearch APIs are written in a certain way and we end up writing workarounds with OpenAPI as well?
nhtruong commentedon Feb 14, 2024
@VachaShah
Of course we will lose out on the biggest perk that comes out of the box with Smithy: Organization of files. Like mentioned in the first bullet, OpenAPI does support that but we will have to be smart about it.
I'm working on the Proof of Concept on how an editable pure OpenSearch API Spec would look like right now.
nhtruong commentedon Feb 20, 2024
Here's the POC Branch: https://github.com/opensearch-project/opensearch-api-specification/tree/feature/native_openapi
Check out the DEVELOPER_GUIDE to have a better understanding of the new layout.
Note that the POC spec are not the 1:1 translation from the current spec:
dblock commentedon Feb 27, 2024
Looks good. Lowercase
tools;) @Xtansia thoughts?Xtansia commentedon Feb 27, 2024
It looks good, I've had good success updating my Java codegen POC to operate off the split spec: opensearch-project/opensearch-java#366
Only issue to note is that at least in the case of the multi-file spec, Java/swagger-parser is unhappy with the
$refs that include extra#s like below, because it attempts to parse the$refas a URI (because it's a relative reference calculated from the current document base URI) and#is a reserved character and should be percent encoded unless it's the fragment delimiter:dblock commentedon Feb 28, 2024
Ideas.
OpenSearch.openapi.yamlbe broken up where all the API-related contents are in subfolders?nhtruong commentedon Feb 28, 2024
@Xtansia RE
$refin split spec. Java Swagger-Parser doesn't have the same complain in single-file spec? It matters because if we can limit programmatic consumption to single-file spec, this is no longer an issue. If it's still an issue, do you think replacing#with:would work?@dblock
We would still have a
buildprocess like you proposed of course.Xtansia commentedon Feb 29, 2024
@nhtruong for the
$refs it's not blocking and I wasn't relying on the libraries built in resolving anyway. However it is something to keep in mind even in the single file spec if we want to be fully correct according to the OpenAPI specification. Valid characters for a URI fragment are:A-Za-z,0-9,-._~!$&'()*+,;=:@/?or otherwise percent-encoded charactersnhtruong commentedon Feb 29, 2024
@Xtansia I see. I'll convert all
#to@in$refdblock commentedon Mar 1, 2024
I understand. I think the question is really whether we check in the combined result or not. My initial thought is not because it's a generated file, but I don't feel that strongly about it.
I'm thinking a release process is more important, something we don't have today in this repo. For example, creating a semver-versioned tag would trigger a proper release like here.
My issue is simpler, https://github.com/opensearch-project/opensearch-api-specification/blob/feature/native_openapi/OpenSearch.openapi.yaml is huge and difficult to edit. Maybe it can be broken up into a file per path.
nhtruong commentedon Mar 3, 2024
That file you linked is not meant to be edited. The spec is split into smaller files in the spec folder: https://github.com/opensearch-project/opensearch-api-specification/tree/feature/native_openapi/spec
The
OpenSearch.openapi.yamlin the root folder is generated from the files in the folder above. It's meant to be consumed programmatically and compare the diff of different versions of the spec.[-][PROPOSAL] Dropping Smithy in favor of Native OpenAPI[/-][+][PROPOSAL] Replace Smithy with a native OpenAPI spec[/+]dblock commentedon Mar 4, 2024
I see. That goes back to my other comment about checking in generated files. We should either remove those from the repo, or at least have a
buildfolder or something like that for them and add markers/comments into the file(s) to say they should not be edited.nhtruong commentedon Mar 26, 2024
Here's the PR for the switch from Smithy to OpenAPI: #208. It's easier to review this PR like reviewing a new repo by inspecting the feature branch.
We'll need to disable the
coverageworkflow to get this merged. I'm working on 2 follow-up PRs:coverageworkflow work for yaml specsspecfolder follow guidelines in the developer guide.nhtruong commentedon Apr 2, 2024
Closing this since #208 has already been merged.