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

Generate missing models from legacy spec #82

Closed
nhtruong opened this issue Mar 16, 2023 · 4 comments · Fixed by #83 or #91
Closed

Generate missing models from legacy spec #82

nhtruong opened this issue Mar 16, 2023 · 4 comments · Fixed by #83 or #91
Assignees

Comments

@nhtruong
Copy link
Collaborator

nhtruong commented Mar 16, 2023

This spawns from #80

Basically use legacy spec to generate smithy models so we don't have to fill out 308 operations by hand.

The translator code can be found here: https://github.com/nhtruong/opensearch-smithy-spec-translator

@nhtruong
Copy link
Collaborator Author

nhtruong commented Mar 28, 2023

Goals

We chose maintaining OpenSearch API Spec via Smithy, even though the end-result is standard OpenAPI spec, mainly for its maintainability. So, the generated Smithy Spec should take advantage of Smithy unique features while assure that the models can still be properly translated to OpenAPI:

  • Each OpenSearch action, each comprised of one or many API operations, are grouped into folders. For example, the smithy files for indicies.create action will be stored in ./model/indices/create folder. (This grouping is impossible with OpenAPI 3.1, one of many reasons we chose Smithy)
  • Each action has 2 smithy files:
    • operations.smithy: Describes the operations of said action
    • structures.smithy: Describes the structures of the Inputs and Outputs of the operations.
  • Most parameters are not only shared within the operations of the same action, but they are also reused in other actions. The parameter definitions, therefore, are stored in common locations for resuse. For example, common_booleans.smithy and common_string_lists.smithy

Problems and Solutions

I came across many problems of various sizes while converting the legacy spec to Smithy. They are listed here for discussion.

Parameter naming collisions

The vast majority of parameters appear in more than 1 action. However, a large portion of parameters, though have the same name, have different shapes, which cause name collisions when translated to shared Smithy models:

  • Parameters sharing the same name with inconsequential differences in their descriptions have their descriptions uniformly updated to 1 identical description (Usually the differences are typos or in punctuation).
  • If a parameter has slightly different descriptions in different actions, their smithy models will have the most common description, or no description at all if they are all unique. Actions with unique descriptions for said parameter will also include the parameter description when it's listed in its structures.smithy file, which will override the description listed in the parameter's model. This strategy is also applied to parameters with different defaults in different actions.
  • Completely different parameters that share the same names, like id, name, metric, are given unique Smithy names. For example, AliasName, TemplateName, PipelineName are Smithy models for name parameters in different actions.

The grouping of Operations into Actions

The fact that one action can have multiple operations causes a few issues:

  • Operation Naming: Each operation in Smithy must have a unique name. The operation name can be generated from the action name. However, since there are multiple operations, the operation named in in the <action_name>_<number> format. @Xtansia suggested to use <http_verb><action_name>With<path_param> format. I'm fine with either since this is inconsequential (The latter is harder to implement since you have to extract the param name, and deal with multiple path params).
  • Shared Query and Body Parameters: All operations of the same action share an identical set of query parameters and in the case of PUT and POST operations, the body parameters as well. To avoid repeating these parameters, mixins are used extensively in operation input definitions.

Default values hidden in descriptions

Many parameters do not explicitly state their default values in the legacy spec but mention the defaults in the descriptions. These values are extracted programmatically using regex when the pattern is clear, and manually added when not.

Extra meta data

Some meta data was added to the smithy models via vendorExtensions trait:

  • operation-group: To group operations into action. This info is needed for client generation.
  • version-added: default to 1.0 for all operations described in the legacy spec except remoteStore.restore operations. This is to support incremental client generation.
  • deprecation information: Some deprecated parameters also come with info on version-deprecated and deprecation-description

@reta
Copy link
Contributor

reta commented Mar 28, 2023

@nhtruong the choice of Smithy is still not obvious, specifically taking into account plugins and extensions, I believe there are compelling arguments to use OpenAPI 3.0+ directly [1], without Smithy shim.

[1] opensearch-project/opensearch-clients#55

@nhtruong
Copy link
Collaborator Author

nhtruong commented Mar 28, 2023

@reta I'm still drafting this comment and accidentally hit Comment. Still editing it now. It will be a bit clearer. This issue simply explains what I did to get to generate the files in this PR: #83 to help people understand what's going on and review it.

As discussed in #80, the end result will be OpenAPI 3.1. We're only using smithy, for now at least, as a device to backfill missing info (like Output schemas, and Body schemas), because it's easier to do so via Smithy than OpenAPI.

@VachaShah
Copy link
Collaborator

Reopening this issue, since there is a part 2 for the PR.

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