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

[BUG] OpenAPI spec validation failed on missing descriptions and name pattern #633

Closed
zelinh opened this issue Oct 21, 2024 · 7 comments · Fixed by #646
Closed

[BUG] OpenAPI spec validation failed on missing descriptions and name pattern #633

zelinh opened this issue Oct 21, 2024 · 7 comments · Fixed by #646
Labels
bug Something isn't working

Comments

@zelinh
Copy link
Member

zelinh commented Oct 21, 2024

What is the bug?

I tried to load the opensearch API spec YAML into a python client openapi-spec-validator.
However, when running the spec validation with python client, it fails on different aspects.

How can one reproduce the bug?

Download the opensearch-api-spec YAML. https://github.com/opensearch-project/opensearch-api-specification/releases/download/main-latest/opensearch-openapi.yaml

pip install openapi-spec-validator

Run this python script

from openapi_spec_validator import validate
from openapi_spec_validator.readers import read_from_filename

spec_file = "/.../Downloads/opensearch-openapi.yaml"
# Load your OpenAPI spec (YAML format)
spec_dict, base_uri = read_from_filename(spec_file)
# If no exception is raised by validate(), the spec is valid.
validate(spec_dict)

Will see error

openapi_spec_validator.validation.exceptions.OpenAPIValidationError: 'description' is a required property

Failed validating 'required' in schema['properties']['components']['properties']['responses']['additionalProperties']['else']:
    {'$comment': 'https://spec.openapis.org/oas/v3.1.0#response-object',
     'type': 'object',
     'properties': {'description': {'type': 'string'},
                    'headers': {'type': 'object',
                                'additionalProperties': {'$ref': '#/$defs/header-or-reference'}},
                    'content': {'$ref': '#/$defs/content'},
                    'links': {'type': 'object',
                              'additionalProperties': {'$ref': '#/$defs/link-or-reference'}}},
     'required': ['description'],
     '$ref': '#/$defs/specification-extensions',
     'unevaluatedProperties': False}

On instance['components']['responses']['indices.create_data_stream@200']:
    {'content': {'application/json': {'schema': {'$ref': '#/components/schemas/_common:AcknowledgedResponseBase'}}}}

After manually updating the component description, we would see another error regarding of the component schema name issue such as info@200/_common:Name don't match the pattern.

What is the expected behavior?

The API spec yaml should be validated by spec validator for OpenAPI 3.1.

What is your host/environment?

MacOS, python 3.9.13

@zelinh zelinh added bug Something isn't working untriaged labels Oct 21, 2024
@dblock dblock removed the untriaged label Oct 22, 2024
@dblock
Copy link
Member

dblock commented Oct 22, 2024

We have a linter/validator for various parts of the spec, and should add thirdparty validators like this one to https://github.com/opensearch-project/opensearch-api-specification/blob/main/.github/workflows/validate-spec.yml.

@dblock
Copy link
Member

dblock commented Oct 25, 2024

I added a python and ruby workflow that uses various libraries to validate the spec in #646 and got ... thousands of errors from the Ruby one ;) Before I go and attempt to fix some of them would love to hear from @nhtruong about what we should be doing here.

@zelinh
Copy link
Member Author

zelinh commented Oct 25, 2024

Yeah. I was seeing the great amount of errors when using this kind of spec validator. Those all look legit according to the documentations from OpenAPI 3.1.0. I would like to hear what we plan as well. :)

@nhtruong
Copy link
Collaborator

nhtruong commented Oct 28, 2024

The files in /spec folders are not typical OpenAPI spec files due to the way the files and components are divided (the $ref objects are not all in a root file like you will see in other OpenAPI specs), and many OpenAPI parsers can't handle this. The merged spec file, however, should be able to pass the OpenAPI validation.

  • The response component doesn't have description as required: The old spec had these as ''. We can make the merger insert these blank descriptions or simply add OK response for 2XX responses and Not found for 404.
  • The component responses name doesn't match pattern ^[a-zA-Z0-9._-]+$. This is a harder problem to resolve because the naming pattern we use are to identify responses of operation groups. Since the operation group itself is already ^[a-zA-Z0-9._-]+$, we use @ as in indices.create@200 to differentiate different response codes of the same operation group. (The client code gens also rely on this to to tell the responses apart). We can change it to indices.create.200 at the cost of slightly less visually distinguishable between operation-group and response code.

@zelinh
Copy link
Member Author

zelinh commented Nov 5, 2024

One new issue i found is:
For example /_bulk, we specify the requestBodies for bulk as an array which conflicts with the application/x-ndjson format, which uses newline-delimited JSON rather than a JSON array. We should specify whichever schema for application/x-ndjson as a String.

Ref:

content:
application/x-ndjson:
schema:
type: array

@dblock
Copy link
Member

dblock commented Nov 6, 2024

@zelinh I think this is a separate problem, want to extract this to a new issue.

I have validation passing in #646.

@zelinh
Copy link
Member Author

zelinh commented Nov 6, 2024

Just created a new issue for this found. #656 Thanks for the fix on validation.

@zelinh I think this is a separate problem, want to extract this to a new issue.

I have validation passing in #646.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants