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: mark the ZodDate as string type in the OpenAPI schema #126

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Gamote
Copy link

@Gamote Gamote commented Dec 10, 2024

This PR aims to mark the ZodDate type as string in the OpenAPI schema.

Fixes

What was done

In the zod-to-openapi.ts I have adde a check that will detect if the source is ZodDate, if it is the object.type will be set to string.

Additionally a test was added in the zod-to-openapi.test.ts to reflect this case.

No breaking changes were added.

Copy link

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 suggestion.

@@ -51,6 +51,10 @@ export function zodToOpenAPI(
}
}

if (is(zodType, z.ZodDate)) {
object.type = 'string'
Copy link
Preview

Copilot AI Dec 10, 2024

Choose a reason for hiding this comment

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

The implementation should set the 'format' property to 'date-time' for the date type in the OpenAPI schema. Update the code to include 'object.format = "date-time"'.

Suggested change
object.type = 'string'
object.type = 'string'; object.format = 'date-time';

Copilot is powered by AI, so mistakes are possible. Review output carefully before use.

Positive Feedback
Negative Feedback

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Please select one or more of the options
Copy link
Author

Choose a reason for hiding this comment

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

@BenLorantfy I got this recommendation locally as well, but I wasn't sure that is necessary. Let me know.

Copy link
Owner

Choose a reason for hiding this comment

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

@Gamote I don't think it's a bad idea, but it should probably be date, not date-time:

image image

Copy link
Author

Choose a reason for hiding this comment

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

@BenLorantfy In that case it might be a bit more common that people are sharing ISO strings rather than just the date. What is your take on it?

Copy link
Owner

@BenLorantfy BenLorantfy Dec 10, 2024

Choose a reason for hiding this comment

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

We might need to support both format: "date" and format: "date-time". But we need a way to tell them apart (e.g. is this zod schema a z.string().date() schema or a z.string().datetime() schema). I can look into that later tonight or tmr if we're unsure how to do so.

Copy link
Author

@Gamote Gamote Dec 10, 2024

Choose a reason for hiding this comment

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

Yeah it might be the best approach, I can try to look into it tomorrow too, but I do agree that supporting both might be the way to go.

Copy link

@deweve deweve Dec 14, 2024

Choose a reason for hiding this comment

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

Hello, if it helps, I see you can check the format of a string with isDate or isDateTime.

You can do this when you know that the schema is a string

Edit: Sorry, the library already handles the string type with date and dateTime.

If it can helps my use case of z.date is to add a transform the date object to a specific format.

This is used in a serializer.

function serializeDate(date: Date): string {
    dayjs(date).format("YYYY-MM-DD");
}

z.date().transform(serializeDate)

In this case, you cannot guess the format

You will have a ZodEffect, the zodEffect schema contains a z.Date and not a z.String.

To conclude : The recommendation is good, z.Date will be transformed in string and it is always a datetime.

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

Successfully merging this pull request may close these issues.

3 participants