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

Add support for $ref in header schemas #432

Closed

Conversation

cq-fpietron
Copy link

@cq-fpietron cq-fpietron commented Jun 29, 2021

Greetings!

First of all, thanks for all the work on the fastify ecosystem. It's a blast to utilise it in projects.

This PR includes a small adjustment to the way headers are treated in plainJsonObjectToOpenapi3.

The motivation is a scenario from my setup, where I attach some headers validation to most of my routes. They contain either enums or examples that I'd like to be properly passed down to the resulting OpenAPI schema. One of the benefits of this is, in tandem with properly initialising refs, to have its presentation in swagger-ui improved: select fields for enum etc.

I first experimented with injecting example and examples fields directly into the value returned from toOpenapiProp, however I think the approach with $ref is both more elegant and functional.

Please let me know whether: my suggestion is too narrow or not, and is the added test sufficient enough.

I'm also wondering

  • if it shouldn't search for $ref within an additional schema property,
  • whether the type field next to possible $ref should now be optional or not - best case scenario I'd like to define the $ref only; same goes for description?

PS: isn't the PR template outdated? I see no benchmark script in package.json.

Cheers!

Checklist

@@ -167,6 +167,9 @@ function plainJsonObjectToOpenapi3 (container, jsonSchema, externalSchemas) {
type: jsonSchemaElement.type
}
}

if (jsonSchemaElement.$ref) result.schema.$ref = jsonSchemaElement.$ref
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering whether this should be

if (jsonSchemaElement.$ref) {
   result.schema.$ref = jsonSchemaElement.$ref
   delete result.schema.type
   delete result.description
 }

To avoid undefined fields in fastify.swagger()?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As you pass a $ref the property type has no purpose to exist, on the other hand description is still legit to be here. So technically you should only delete result.schema.type

@zekth zekth requested a review from climba03003 June 29, 2021 09:41
Copy link
Member

@mcollina mcollina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@coveralls
Copy link

Pull Request Test Coverage Report for Build 981880591

  • 3 of 3 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage remained the same at 100.0%

Totals Coverage Status
Change from base Build 981089321: 0.0%
Covered Lines: 537
Relevant Lines: 537

💛 - Coveralls

@climba03003
Copy link
Member

climba03003 commented Jun 29, 2021

After checking the generated json, I think this cannot be supported unless we add the support of object property $ref resolution.
The output and the test seems to be fine, but you can see that components.schemas is empty object. swagger-ui will throw error because #/components/headers/someHeader cannot be find.

{
  "openapi": "3.0.3",
  "info": {
    "title": "Test swagger",
    "description": "testing the fastify swagger api",
    "version": "0.1.0"
  },
  "components": {
    "securitySchemes": {
      "apiKey": {
        "type": "apiKey",
        "name": "apiKey",
        "in": "header"
      }
    },
    "schemas": {}
  },
  "paths": {
    "/": {
      "get": {
        "requestBody": {
          "content": {
            "application/json": {
              "schema": {
                "type": "object",
                "required": [
                  "hello"
                ],
                "properties": {
                  "hello": {
                    "type": "string"
                  }
                }
              }
            }
          }
        },
        "parameters": [
          {
            "in": "header",
            "name": "referencedHeader",
            "required": true,
            "schema": {
              "$ref": "#/components/headers/someHeader"
            }
          }
        ],
        "responses": {
          "200": {
            "description": "Default Response"
          }
        }
      }
    }
  },
  "servers": [
    {
      "url": "http://localhost"
    }
  ],
  "security": [
    {
      "apiKey": []
    }
  ],
  "tags": [
    {
      "name": "tag"
    }
  ],
  "externalDocs": {
    "description": "Find more info here",
    "url": "https://swagger.io"
  }
}

@cq-fpietron
Copy link
Author

@climba03003 thanks for your checks. I have stumbled into this too. I was able to get it to work in a slightly hacky way, though.

The solution was to use $refs to components/schemas instead of parameters or headers(As per OpenAPI 3.0 components section, components/headers are for response headers, not request headers). I guess it is not as crucial in this case as I have only seen references to components/schemas in fastify-swagger's codebase.

I also had those definitions added to fastify via app.addSchema with $id: '#/components/schemas/someHeader.

It would appear that parsing components/parameters is nearly valid, as I can either get fully valid OA3 schema or one that displays in swagger-ui nicely. I have created a sample repository available here to depict this behaviour: cq-fpietron/fastify-swagger-refs.

Starting up the server you can toggle between registering fastifySwagger with either option.

@climba03003 , do you have any idea how we could estimate work on getting this feature right?

@climba03003
Copy link
Member

@climba03003 thanks for your checks. I have stumbled into this too. I was able to get it to work in a slightly hacky way, though.

The solution was to use $refs to components/schemas instead of parameters or headers(As per OpenAPI 3.0 components section, components/headers are for response headers, not request headers). I guess it is not as crucial in this case as I have only seen references to components/schemas in fastify-swagger's codebase.

I also had those definitions added to fastify via app.addSchema with $id: '#/components/schemas/someHeader.

It would appear that parsing components/parameters is nearly valid, as I can either get fully valid OA3 schema or one that displays in swagger-ui nicely. I have created a sample repository available here to depict this behaviour: cq-fpietron/fastify-swagger-refs.

Starting up the server you can toggle between registering fastifySwagger with either option.

@climba03003 , do you have any idea how we could estimate work on getting this feature right?

The fastest way is update the code of resolver in https://github.com/Eomm/json-schema-resolver which fit the needs.
The best way is rewrite a brand new resolver which can solve #337

@davidschmitt
Copy link

davidschmitt commented Aug 18, 2021

FYI, I have created a drop-in replacement for json-schema-resolver which solves many of the $ref related issues in external schemas for fastify-swagger.

I've published the code here: https://github.com/davidschmitt/fastify-schema-resolver.git

I haven't published the package on npmjs.com (I've never done that before), but if the fastify-swagger maintainers are interested in switching I can do so.

EDIT: I just realized that while my new resolver fixes fastify-swagger $ref issues for body and response schemas, it does not correct the top level $ref issue for headers, params or querystring schemas. My apologies for posting on this thread

@stale
Copy link

stale bot commented Apr 16, 2022

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

@stale stale bot added the stale label Apr 16, 2022
@stale stale bot closed this Apr 27, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants