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

fix(resolve schema reference): shortcut if definitions is undefined #830

Draft
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

jpb06
Copy link

@jpb06 jpb06 commented Oct 17, 2024

Hello. Firstof, thanks for maintaining this lib 🙇🏻

It appears this fix is causing an issue with our codebase. We have an endpoints that is is defined like this:

import type { FastifyInstance, FastifyRegisterOptions } from 'fastify';
import type { FastifyZodOpenApiTypeProvider } from 'fastify-zod-openapi';
import { z } from 'zod';

const myResponseSchema = z
  .object({
     // ...
  })
  .openapi({
    ref: 'MyResponse',
  });

fastifyInstance.withTypeProvider<FastifyZodOpenApiTypeProvider>().route({
    method: 'GET',
    url: '/',
    schema: {
      response: {
        [HTTPStatusCodeEnum.ok]: myResponseSchema
      },
    },

which resolves to a schema like this:

{
  "components": {
    "schemas": {
      "MyResponse": {
        "type": "object",
        "properties": {
           // ...
         }
      }
    }
  },
  "paths": {
    "/my-endpoint": {
      "get": {
        "responses": {
          "200": {
            "description": "Default Response",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/MyResponse",
                }
              }
            }
          }
        }
      }
    }
  }
}

The issue is there is no definitions for this ref, which makes the whole thing crash. Wasn't the case before.
One simple fix is to check if resolvedReference.definitions is undefined before trying to access it.

I tried to write a test to reproduce the issue but I'm not an openapi expert so I can't seem to get anywhere:

test('should not crash if response has no definitions', async (t) => {
  const fastify = Fastify();

  await fastify.register(fastifySwagger, {
    openapi: {
      components: {
        schemas: {
          MyResponse: {
            type: 'object',
            properties: {},
          },
        },
      },
      paths: {
        '/response-with-ref': {
          get: {
            responses: {
              200: {
                description: 'Default Response',
                content: {
                  'application/json': {
                    schema: {
                      $ref: '#/components/schemas/MyResponse',
                    },
                  },
                },
              },
            },
          },
        },
      },
    },
  });
  fastify.register(async (instance) => {
    instance.get(
      '/response-with-ref',
      {
        schema: {
          response: {
            200: { $ref: '#/components/schemas/MyResponse' },
          },
        },
      },
      () => {}
    );
  });

  await fastify.ready();

  const openapiObject = fastify.swagger();

  t.equal(typeof openapiObject, 'object');

  await Swagger.validate(openapiObject);
});

This fails with:

 ✖should not crash if response has no definitions > Failed building the serialization schema for GET: /response-with-ref, due to errornode_modules/fastify/lib/route.js:
   Cannot find reference "#/components/schemas/MyResponse"                                                                            400:21

Any help would be appreciated. Thank you!

Checklist

@mottet
Copy link

mottet commented Oct 24, 2024

Hi !

I missed this PR but I openned yesterday an issue on fastify-zop-openapi for the exact same problem samchungy/fastify-zod-openapi#172

I explained the problem with a repo that reproduce the bug https://github.com/mottet/bug-fastify-zod-openapi

I have the same conclusion about the fix in lib/util/resolve-schema-reference.js

@jpb06
Copy link
Author

jpb06 commented Oct 24, 2024

Thanks @mottet !

@steveatkoan
Copy link

Ah. Just spotted this. Submitted #836 yesterday which is related

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants