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

[PROPOSAL] Refactor query parameter definitions for maintainability #783

Open
kolchfa-aws opened this issue Jan 10, 2025 · 7 comments
Open

Comments

@kolchfa-aws
Copy link
Contributor

What/Why

  1. There are many query parameters that are shared by several APIs. For these parameters, I think it's best to keep the description of the parameter in the common shared schema. For example, expand_wildcards is shared by several APIs, including several CAT APIs. Some of the CAT APIs define their own descriptions for this parameter, while the description should be part of the common schema.
  2. For parameters that take enums (for example, the same expand_wildcards), the description should not contain the valid parameter values. Rather, these can be generated from the schema.

For reference, here's the common schema for expand_wildcards:

ExpandWildcards:
      oneOf: 
        - $ref: '#/components/schemas/ExpandWildcard'
        - type: array
          items:
            $ref: '#/components/schemas/ExpandWildcard'
    ExpandWildcard:
      oneOf:
        - type: string
          const: all
          description: Match any index, including hidden ones.
        - type: string
          const: closed
          ...

This proposal is to make it as follows:

ExpandWildcards:
      description: Specifies the type of index that wildcard expressions can match. Supports comma-separated values. 
      oneOf: 
        - $ref: '#/components/schemas/ExpandWildcard'
        - type: array
          items:
            $ref: '#/components/schemas/ExpandWildcard'
    ExpandWildcard:
      oneOf:
        - type: string
          const: all
          description: Match any index, including hidden ones.
        - type: string
          const: closed
          ...

And remove descriptions from individual APIs like CAT indices:

cat.indices::query.expand_wildcards:
      in: query
      name: expand_wildcards
      description: The type of index that wildcard patterns can match.  Supported values are `all`, `open`, `closed`, `hidden`, and `none`.
      schema:
        $ref: '../schemas/_common.yaml#/components/schemas/ExpandWildcards'
      style: form

What problems are you trying to solve?

Improve maintainability of the spec

@Naarcha-AWS
Copy link
Contributor

@kolchfa-aws: To clarify, you're saying keep the schema reference within individual namespaces the same, but remove the descriptions so it uses the common description?

@nhtruong
Copy link
Collaborator

Note that even though most are identical, There are subtle differences in what set of values is allowed in expand_wildcards in certain endpoints.

@kolchfa-aws
Copy link
Contributor Author

Yes, have one common description for all of the same parameters and generate the valid values for documentation based on the enum (I am assuming that the subtle differences are reflected in the spec).

@Naarcha-AWS
Copy link
Contributor

@kolchfa-aws and @nhtruong: I'll try this out locally this afternoon. @nhtruong, would this affect the spec-insert tool logic in any way?

@kolchfa-aws
Copy link
Contributor Author

@nhtruong I think we'd need to add logic for populating valid enum values so it spits them out in the following format:

| `expand_wildcards` | String | Specifies the types of indexes to which wildcard expressions can expand. Supports comma-separated values. Valid values are: <br> - `all`: Expand to all open and closed indexes, including hidden indexes. <br> - `open`: Expand to open indexes. <br> - `closed`: Expand to closed indexes. <br> - `hidden`: Include hidden indexes when expanding. Must be combined with `open`, `closed`, or both. <br> - `none`: Do not accept wildcard expressions. <br> Default is `open`. |

@Naarcha-AWS
Copy link
Contributor

Performed a small local test according to @kolchfa-aws example. It passes. I would be curious to see how this affects the output of the spec-insert tool as it currently stands.

Furthermore, although it would be tempting to identify as many common parameters and replace as many as we can in a single PR, I would suggest a slower approach per namespace file. Doing so would help us account for the following:

  1. Identifying which endpoints are currently missing in documentation.
  2. Allowing reviewers to call out cases where the common definition needs to account for unique endpoint behaviors, as @nhtruong mentioned.

@nhtruong
Copy link
Collaborator

nhtruong commented Jan 14, 2025

I think we'd need to add logic for populating valid enum values so it spits them out in the following format:

Yeah that can be done. I'll take a crack at it later this week.

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

No branches or pull requests

3 participants