fastify-swagger

fastify-swagger copied to clipboard

Prerequisites

• [X] I have written a descriptive issue title
• [X] I have searched existing issues to ensure the bug has not already been reported

Fastify version

3.x

Plugin version

5.1.1

Node.js version

16.14.0

Operating system

macOS

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

Chrome: 100

Description

Optional parameters are specified as required

Steps to Reproduce

1. Create simple route

fastify.get(
  '/',
  {
    schema: {
      params: {
        type: 'object',
        properties: {
          id: {
            type: 'number'
          },
          name: {
            type: 'string'
          }
        },
        required: ['name']
      },
      response: {
        200: {
          type: 'object',
          properties: {
            hello: { type: 'string' }
          }
        }
      }
    }
  },
  (request, reply) => {
    reply.send({ hello: `world` })
  }
)

2. Start fastify server

3. Open swagger page

Expected Behavior

Optional parameters required if set

I see this behavior is hardcoded

Openapi https://github.com/fastify/fastify-swagger/blob/19899f09122814651b8e79b26dfc562fc2fbe52b/lib/spec/openapi/utils.js#L171

Swagger https://github.com/fastify/fastify-swagger/blob/19899f09122814651b8e79b26dfc562fc2fbe52b/lib/spec/swagger/utils.js#L134

It is expected behavior. Since params is the variables of path, it must be required.

@climba03003 Just encountered this.

I understand your point, but Fastify supports making last parameter optional, see here.

It would be nice for "create/update" endpoints.

I even mirrored this in my schema

@StepanMynarik

Are you willing to provide a PR for this?

@Uzlopak

Thank you for the lighting quick response!

Let me see what I can do. This will be my first.

@Uzlopak

So I tried it only to later realize I have no idea what I'm doing 😆

Not well-versed within tap, swagger/openapi nor this repository specifics. At first glance seemed like a simple change, well...

Also few unit tests break as soon as "required" is not always true and I don't know how to fix these since I don't fully understand all of them.

You can provide the code for guidance. No worry about not being familiar with the tools we are using.

For your endpoint, I recommend to split it using POST for creation and PUT or PATCH for update.

For the PR I would accept, only the last parameter is allowed to be optional. Otherwise, it seems like a design fault of API since it allow something like endpoint///// to call.

The code was nothing to speak of. Few lines and it was definitely wrong. Wasn't able to even commit to my own fork due to weird errors (something about bash, probably related to the fact unit tests weren't passing?).

At least I can point to new relevant code locations:

• OpeanAPI: https://github.com/fastify/fastify-swagger/blob/master/lib/spec/openapi/utils.js#L209
• Swagger: https://github.com/fastify/fastify-swagger/blob/master/lib/spec/swagger/utils.js#L141

For easy "isLast" detection I changed this:

.map((propKey) => {
  const jsonSchema = toOpenapiProp(propKey, obj[propKey])

to this:

.map((propKey, index, array) => {
  const isLast = index === array.length - 1
  const jsonSchema = toOpenapiProp(propKey, obj[propKey], isLast)

As per the OAS 3.0 spec, it’s not allowed to have a path parameter to be optional.

Determines whether this parameter is mandatory. If the parameter location is "path", this property is REQUIRED and its value MUST be true. Otherwise, the property MAY be included and its default value is false.

So looks like this is correct behaviour.

This makes sense except for the last parameter at the end of the path.

Consider the following endpoints:

• books/3 (GET to read or PUT/POST to update)
• books (POST to create)

But yeah, can be fairly easily implemented by making three different endpoints in Fastify right now (GET, PUT and POST).

It's just convenient to use the same POST endpoint for both create and update with optional ID param in path.

But your example is wrong, as they are definetly three different endpoints?!

Sorry, tired after a long day 😄

The example can be implemented using two endpoints, GET for read by ID and POST for create/update based on whether ID parameter is provided or not. For this to work, you need the ID parameter to be optional.

It's just convenient to have create/update operations in a single POST endpoint when putting something together quick and you repeat that for multiple resources.

This is just one possible use of this, not very clean, I admit.

For me the main problem is that Fastify allows you to specify the last param as optional explicitly by appending questionmark like so ":param_name?". When this is a documented intentional feature of Fastify, I would expect this package to support that behavior as well.

I think that specific entry should be duplicated in the OAS to support the optional parameter.

can be fixed using this transform method in @fastify/swagger's plugin config:

transformObject: ({ openapiObject }) => {
  // fix for optional path parameters
  for (const [url, path] of Object.entries(openapiObject.paths)) {
    for (const method of Object.values(path)) {
      if (!method.parameters) continue;
      for (const param of method.parameters) {
        if (param.in === 'path') param.required = !url.includes(`{${param.name}}?`);
      }
    }
  }

  return openapiObject;
},

looks good at least in Swagger UI