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

Schemas from subpackages break without calls to .openapi() #158

Closed
mikestopcontinues opened this issue Jul 14, 2023 · 9 comments · Fixed by #159
Closed

Schemas from subpackages break without calls to .openapi() #158

mikestopcontinues opened this issue Jul 14, 2023 · 9 comments · Fixed by #159

Comments

@mikestopcontinues
Copy link

mikestopcontinues commented Jul 14, 2023

At the moment, I maintain one package with all of my application types, which I then import and wrap with .openapi() calls for use with zod-to-openapi. However, this behavior completely stopped working recently. I thought it was caused by updating to 5.3.0, but testing ruled that out.

Here's an example of code that no longer works:

import {Key} from '@my/package';

const KeyPost = Key.pick({name: true}).openapi('KeyPost');
const KeyPatch = Key.pick({name: true}).openapi('KeyPut');

registry.registerPath('/key', {...{schema: KeyPost}...});
registry.registerPath('/key/{keyId}', {...{schema: KeyPut}...});

And the error:

node:internal/process/esm_loader:97
    internalBinding('errors').triggerUncaughtException(
                              ^
UnknownZodTypeError {
  message: 'Unknown zod object type, please specify `type` and other OpenAPI props using `ZodSchema.openapi`.',
  data: {
    currentSchema: {
      unknownKeys: 'strip',
      catchall: $e {...},
    schemaName: 'Key'
  }
}

I can swallow some of the errors with a merge:

const KeyPost = z.object({}).merge(Key.pick({name: true})).openapi('KeyPost');

However, then I get the error for deeper properties that are reused:

node:internal/process/esm_loader:97
    internalBinding('errors').triggerUncaughtException(
                              ^
UnknownZodTypeError {
  message: 'Unknown zod object type, please specify `type` and other OpenAPI props using `ZodSchema.openapi`.',
  data: {
    currentSchema: {
      checks: [ { kind: 'min', value: 0 }, { kind: 'max', value: 255 } ],
      typeName: 'ZodString',
      coerce: false
    },
    schemaName: undefined
  }
}

I wish I had a better sense of how things stopped working, but I tried both rolling back my package updates as well as rolling back to previously working commits and clearing all caches and temp data. Neither brings back the prior behavior. I know it was working, because I was actively working with the code yesterday.

On the other hand, I experienced similar errors when I first started working with zod-to-openapi, so I suspect that there's likely something deeper going on.

@mikestopcontinues mikestopcontinues changed the title Nested types without .openapi() broken in v5.3.0 Nested, reused types without .openapi() broken in v5.3.0 Jul 14, 2023
@mikestopcontinues mikestopcontinues changed the title Nested, reused types without .openapi() broken in v5.3.0 Nested, reused types without .openapi() Jul 14, 2023
@mikestopcontinues mikestopcontinues changed the title Nested, reused types without .openapi() Schemas from subpackages break without calls to .openapi() Jul 14, 2023
@AGalabov
Copy link
Collaborator

@mikestopcontinues thank you for reporting.

Would you be able to provide a full example of something that breaks it. Since .registerPath does not follow the signature you've provided (path first and an object as a second parameter). And furthermore I don't understand how you are picking fileds that are not apparent in the Key schema.

I tried to create this script:

export const registry = new OpenAPIRegistry();

const Key = z.object({ name: z.string() });

const KeyPost = Key.pick({ name: true }).openapi('KeyPost');
const KeyPatch = Key.pick({ name: true }).openapi('KeyPatch');

registry.registerPath({
  path: '/key',
  method: 'post',
  responses: {
    200: {
      description: 'Sample response',
      content: {
        'application/json': {
          schema: KeyPost,
        },
      },
    },
  },
});

registry.registerPath({
  path: '/key/{keyId}',
  method: 'patch',
  responses: {
    200: {
      description: 'Sample response',
      content: {
        'application/json': {
          schema: KeyPatch,
        },
      },
    },
  },
});

and it did generate a correct result:

"paths": {
        "/key": {
            "post": {
                "responses": {
                    "200": {
                        "description": "Sample response",
                        "content": {
                            "application/json": {
                                "schema": {
                                    "$ref": "#/components/schemas/KeyPost"
                                }
                            }
                        }
                    }
                }
            }
        },
        "/key/{keyId}": {
            "patch": {
                "responses": {
                    "200": {
                        "description": "Sample response",
                        "content": {
                            "application/json": {
                                "schema": {
                                    "$ref": "#/components/schemas/KeyPatch"
                                }
                            }
                        }
                    }
                }
            }
        }
    }

The above seems correct. So please elaborate on your issue with a reproducible example

@mikestopcontinues
Copy link
Author

Hi @AGalabov, I updated the original post just now. I was not correct in my initial diagnosis. The issue is with pulling the schemas from other packages in a monorepo. I'm actively trying to narrow down the issue now.

@AGalabov
Copy link
Collaborator

@mikestopcontinues would you be able to share how the actual Key schema looks like? Looking at the error data something there should be lacking a type. Maybe I would be able to help if I see the schema. If it is not that schema I am not quite sure what else it could be.

@mikestopcontinues
Copy link
Author

@AGalabov Here's a reproduction. The bug occurs as I find in my actual monorepo.

A key source of confusion for me is that this exact code was working perfectly yesterday, under heavy development. I watched my types, my docs, and my client SDK update with changes, without problem. Some combination of updating dependencies and clearly temp data broke things. But the fact that this repro also errors suggests there's a real bug here.

Anything else I can provide?

@AGalabov
Copy link
Collaborator

@mikestopcontinues thank you!

I did pull your code and added a few console.logs of the broken KeyPublic schema.
Some inner zod-to-openapi workings here - in order to check the instance of a specific zod type we are using schema.constructor.name (in order to account for different zod instances- a previous bug we've found).

In general this particular schema should have ZodObject as a constructor name. However in your case it is just Z (and it's internal catchall section has its constructor.name=ve). So the problem is that something in this repositories setup is causing the constructor name obfuscation/mimification.

There are two things that I can say:

  1. If you can take a look at any configuration changes to the way the repo is built - you might be able to fo find the root cause for your particular case.
  2. For zod-to-openapi there is another approach that we could possibly take in order to check the "instance type" - I'd need to play with it a bit to confirm if that is possible

@mikestopcontinues
Copy link
Author

@AGalabov Thanks a million. I'd been working in watch mode yesterday (no minification), and today I was trying to do a production build. As far as I'm concerned, my issue is resolved.

I do see the value of better type discovery. Does Type._def.typeName give you the right thing? instanceof would be preferable, but just a thought for if it didn't work.

Thanks again!

@AGalabov
Copy link
Collaborator

@mikestopcontinues

As far as I'm concerned, my issue is resolved.
That's great to hear! Glad I could help!

As for:

I do see the value of better type discovery. Does Type._def.typeName give you the right thing? instanceof would be preferable, but just a thought for if it didn't work.

Yes the instanceof check was the cleanest and most obvious solution that we had initially. However this had its problems (basically having two zod instances). I am not sure if the _def.typeName was always there and we just missed it or if it is a new addition to zod itself. From my first look at it it seems like it will work just as want it to. I'll probably test some more tomorrow morning and if all works I'll open up a PR.

So considering you're "resolved" I'll leave this issue open until I get to check the alternative approach. If it doesn't work for whatever reason I'll have to close the issue with a comment.

Thank you for the collaboration as well!

AGalabov added a commit that referenced this issue Jul 17, 2023
…-breaks-type-checks

use _def.typeName for type checking zod schemas
@AGalabov
Copy link
Collaborator

@mikestopcontinues I think this should be working now for you with v5.3.1

@mikestopcontinues
Copy link
Author

Awesome, thanks!

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 a pull request may close this issue.

2 participants