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

#115 added support for ZodPipeline #118

Merged
merged 3 commits into from
Apr 19, 2023

Conversation

AGalabov
Copy link
Collaborator

Based on the documentation it seems that we can generate a documentation for ZodPipeline.

However we do have two options:

  1. Use the in property saved within the zod schema (done in the PR). My thought process was that this is what an API would receive when this schema is used for validation.
  2. Use the out property as it describes the target state (might not always be the case).

Overall I feel like in is the only correct option we can use. But saying that it does bring up the question why cannot we just use the same logic when handling the result of .transform(). @georgyangelov any thoughts would be appreciated.

@AGalabov AGalabov requested a review from georgyangelov April 15, 2023 20:10
@AGalabov AGalabov linked an issue Apr 15, 2023 that may be closed by this pull request
Copy link
Collaborator

@georgyangelov georgyangelov left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. As far as transform goes - I agree, we can use the input schema. I was under the impression that we couldn't extract the in schema in that case and that's why we weren't supporting it.

describe('pipe', () => {
z.string()
.transform(val => val.length)
.pipe(z.number().min(5));
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the problem?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I got what the problem is 😄

@@ -974,6 +974,10 @@ export class OpenAPIGenerator {
};
}

if (isZodType(zodSchema, 'ZodPipeline')) {
return this.toOpenAPISchema(zodSchema._def.in, isNullable, defaultValue);
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well this was easy 😄

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are you sure that in is the right thing to use here?

I would expect something like this:

  • Use in for parameters as this is the possible input, from which an instance can be created
  • Use out for schemas as this is the resulting type that will come out

@AGalabov
Copy link
Collaborator Author

@georgyangelov well I am not sure what the problem was to be honest. It might have been "fixed"/resolved by zod from then. From what I see and was even mentioned in another issue we can just use _def.schema to unwrap it -> essentially going to the underlying schema. So I think we can definitely give it a go.

@AGalabov AGalabov merged commit 8f9285f into master Apr 19, 2023
@AGalabov AGalabov deleted the feature/#115-add-support-for-zod-pipelines branch April 19, 2023 08:29
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.

Support for ZodPipeline
3 participants