fastify / fastify-swagger

Swagger documentation generator for Fastify
MIT License
889 stars 199 forks source link

Wildcard paths fails Swagger validation #773

Closed andreaspalsson closed 6 months ago

andreaspalsson commented 7 months ago

Prerequisites

Fastify version

4.25.0

Plugin version

8.12.1

Node.js version

18.16.0

Operating system

macOS

Operating system version (i.e. 20.04, 11.3, 10)

14.1.1

Description

We are running into a issue when creating a schema and Swagger documentation for a wildcard route.

When creating a route with a wildcard in the path, the swagger schema produced does not pass the validation when used on https://editor.swagger.io/. The problem is that URL does not match with the path parameters in the schema because the * has been renamed to wildcard.

We notice this when we started to validate our Swagger schema with https://editor.swagger.io/ and the following errors occurred

Semantic error at paths./id/{id}/star/{wildcard}
Declared path parameter "wildcard" needs to be defined as a path parameter at either the path or operation level
Jump to line 20

Semantic error at paths./id/{id}/star/{wildcard}.get.parameters.1.name
Path parameter "*" must have the corresponding {*} segment in the "/id/{id}/star/{wildcard}" path
Jump to line 31

We have also created a small repo that shows the issue https://github.com/Milkywire/fastify-swagger-wildcard

Steps to Reproduce

Setup a route with a wildcard like this

 fastify.get(
   "/id/:id/star/*",
   {
     schema: {
       params: {
         id: { type: "number" },
         "*": { type: "string" },
       },
     },
   },
   (request, reply) => {
     reply.send(request.params);
   }
 );

The documentation produced by @fastify/swagger will replace the * with {wildcard} in the path of the endpoint. So the documentation will look like this

"/id/{id}/star/{wildcard}": {
  "get": {
        "parameters": [
        {
            "schema": { "type": "number" },
            "in": "path",
            "name": "id",
            "required": true
        },
        {
            "schema": { "type": "string" },
            "in": "path",
            "name": "*",
            "required": true
        }
        ],
        "responses": { "200": { "description": "Default Response" } }
    }
}

Copy the JSON into https://editor.swagger.io/ and you will get the following errors

Semantic error at paths./id/{id}/star/{wildcard}
Declared path parameter "wildcard" needs to be defined as a path parameter at either the path or operation level
Jump to line 20

Semantic error at paths./id/{id}/star/{wildcard}.get.parameters.1.name
Path parameter "*" must have the corresponding {*} segment in the "/id/{id}/star/{wildcard}" path
Jump to line 31

Expected Behavior

The path in the documentation matches the path specified in fastify.

Potential Workaround

A potential workaround that we looked at was to replace the star in the endpoint schema with “wildcard” to align with the documentation. This works okey when using JavaScript we just have to remember that we still need to use the “*” when getting the value from request.params and not wildcard.

However in our setup using TypeScript and TypeBox for our schema setup this is not an option since the request.params object is limited to the fields you define in the schema.

casantosmu commented 7 months ago

Seems like a bug in fastify-swagger. When converting the URL, the plugin replaces the "*" param with {wildcard} here, but it doesn't do the same for the parameters schema. Two potential solutions:

  1. Do not convert the "*" param to {wildcard}.
  2. Convert the "*" param to "wildcard" in the parameters schema.

Since the conversion of the URL seems to be the expected behavior and changing it could break current behavior, the second option seems best. However, it might break code for those aware of this behavior and defining the param schema for "/id/:id/star/*" like:

params: {
  id: { type: "number" },
  wildcard: { type: "string" },
},

I'm willing to work on this if the maintainers check this issue.

mcollina commented 6 months ago

Thanks for reporting! Would you like to send a Pull Request to address this issue? Remember to add unit tests.

casantosmu commented 6 months ago

@mcollina, what do you think about these two possible solutions? I'm interested in getting to work on this

mcollina commented 6 months ago

I don't think we can change the actual schema of the route, if that's what you are asking. Anything that happens only in openapi land... it's ok.