fastify / fastify-swagger

Swagger documentation generator for Fastify
MIT License
915 stars 201 forks source link

feat: Add hideSchemas swagger option (#686) #687

Closed imjuni closed 1 year ago

imjuni commented 1 year ago

Checklist

Uzlopak commented 1 year ago

I am now too lazy to read the code. But the only way to avoid those schemas to be rendered in the final document should be by resolving the references with the schemas.

mcollina commented 1 year ago

I am now too lazy to read the code. But the only way to avoid those schemas to be rendered in the final document should be by resolving the references with the schemas.

Exactly. That's what I'm not 100% sure about here.

imjuni commented 1 year ago

@Uzlopak , @mcollina Here is example,

const fastify = require('fastify');
const server = fastify();

// case 01. error raise when server start
server.addSchema({
  $id: 'IReqPokeDetailQuerystring',
  type: 'object',
  properties: {
    tid: {
      $ref: 'missing reference!',
    },
  },
  required: ['tid'],
});

// case 02. error raise when request call
server.get(
  '/pokemon/:name',
  {
    schema: {
      querystring: {
        type: 'object',
        properties: {
          tid: {
            $ref: 'missing reference!',
          },
        },
        required: ['tid'],
      },
    },
  },
  () => {
    /* ... */
  },
);

I prefer what case01 then case02. Because more than quickly aware error from validation schema. But case01 generate so many DTO in swagger document. My company project already over 100 DTOs. Since Querystirng, Headers, Params, and Body each generate a DTO, the DTO can be increased very much. So I hope hide Querystring, Headers, Params and Body DTO. Because Querystring, Headers, Params and Body already describe in API spec.

create-ts-json-schema

hideSchemas option is great helpful for create-ts-json-schema.

Uzlopak commented 1 year ago

I understand that, but you need to resolve the references. If the DTO is not used anyway, it should not be added to the final swagger document.

But you are just manually suppress the propagation to the final document.

So e.g. i would actually expect that if you specifiy IReqPokeDetailQuerystring as a reference that if it is used somewhere to be resolved so that the specification is still valid and remove it from the final swagger documentation. If it is not referenced anywhere, it would be anyway removed.

So instead of hideSchemas i would expect a resolveReferences option, where you pass an Array or maybe a function where the name of a reference is passed and then decided if it should be resolved or not

resolveReference: (name: string) => boolean

Then you could do something like

function resolveReference(name) {
   return name.endsWith("Querystring") === false
}
imjuni commented 1 year ago

I understand resolve reference. But some difference of this case. First I try to remove reference like that,

ref = Ref()
const prevDefinitions = prepareSwaggerDefinitions({
  ...swaggerObject.definitions,
  ...(ref.definitions().definitions)
}, ref)

// remove reference
const definitions = {}
for (const [$id, schema] of Object.entries(prevDefinitions)) {
  if (defOpts.resolveReference(schema.$id)) {
    definitions[$id] = schema
  }
}

But this approach fail to build document in case of OpenAPI v2. Because OpenAPI v2 use reference for document building. This work raise error from prepareSwaggerMethod. But OpenAPI v3 fine work this approach. I thought it would be better for v2 and v3 to behave the same. Also I can chage option type string[] to Function. Like that,

const definitions = {}
for (const [$id, schema] of Object.entries(swaggerObject.definitions)) {
  if (defOpts.isHideReference(schema.title)) {
    definitions[$id] = schema
  }
}
Uzlopak commented 1 year ago

Imho you are just removing the DTOs from the final document. I mean your unit tests actually show it beatifully. So despite you added schema-0, schema-1 and schema-2 you manually remove schema-1, and because you dont use it in your schema building the schema succeeds . If we would remove schema-2, we should get a FST_ERR_SCH_SERIALIZATION_BUILD-Error.

We should actually fist add a flag which removes unused definitions from the final swagger-documentation.

Example unit test:

test('remove unused schemas', async t => {
  t.plan(2)
  const fastify = Fastify()
  await fastify.register(fastifySwagger, { ...openapiOption, removeUnusedSchemas: true })

  fastify.addSchema({
    $id: 'schema-01',
    type: 'object',
    properties: {
      id: { type: 'number', description: 'data id' }
    },
    required: ['id']
  })

  fastify.addSchema({
    $id: 'schema-02',
    type: 'object',
    properties: {
      id: { type: 'number', description: 'data id' }
    },
    required: ['id']
  })

  fastify.addSchema({
    $id: 'schema-03',
    type: 'object',
    properties: {
      id: { type: 'number', description: 'data id' }
    },
    required: ['id']
  })

  fastify.get('/', {
    schema: {
      querystring: { $ref: 'schema-02' },
      response: {
        200: {
          type: 'object'
        }
      }
    }
  }, () => {})

  await fastify.ready()

  const openapiObject = fastify.swagger()
  await Swagger.validate(openapiObject)

  const definitions = openapiObject.components.schemas
  t.ok(definitions)
  t.same(definitions, {
    'def-2': {
      type: 'object',
      properties: {
        id: { type: 'number', description: 'data id' }
      },
      required: ['id'],
      title: 'schema-03'
    }
  })
})

In a second step, we should implement a flag, which forces to resolve specified DTOs. And then remove them from being exposed in the final swagger document.

imjuni commented 1 year ago

I want exactly "just removing the DTOs from the final document".

Because swagger definitions and fastify schemas cannot be exactly the same, that unattainable goal. For example, @fastify/multipart can add validation schema. However, this schema is not what people want and may need to be hidden. Or people want add schema for onRequest and preValidation. How can hide that schema? I think need that view(swagger document) and model(definitions) decoupling. even if that is not hideSchemas.

I realize problem. hideSchemas work fine parameters (Querystring, Headers, Params, Body). But raise error on Response section. And Swagger.validate don't detect this problem, swagger document building success but display error message on browser top. So, I think it is better not to do find already used reference work because it does the simular work as json-schema-resolver and it is a heavy work.