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

Merging multiple calls to openapi() #303

Closed
mrichards42 opened this issue Aug 6, 2024 · 6 comments · Fixed by #356
Closed

Merging multiple calls to openapi() #303

mrichards42 opened this issue Aug 6, 2024 · 6 comments · Fixed by #356

Comments

@mrichards42
Copy link

Hi, thanks for a great library!

I was surprised to see that if I make multiple calls to openapi() that the _def.openapi object is overwritten instead of being extended. My use case is that I have an existing module with some custom zod schemas (not z.custom(), I just mean functions returning schemas) that have partially filled out openapi info, where I then want to finish with a description and example at the point they are used.

That's a mouthful, so an example (this isn't my exact code, but close enough)

// I want effectType: "input" to sticky around
// this is just an example, in the real code I have several more openapi properties here
const funnyDate = z
  .string()
  .regex(funnyDateRegex)
  .transform(parseFunnyDate)
  .openapi({ effectType: "input" })

// when I use funnyDate, effectType gets clobbered by the second call to openapi
const SomeObject = z.object({
  date: funnyDate().openapi({ description: "hello world", example: "..." })
})

IMO augmenting is a little closer to how the rest of zod works (e.g. if i make a call to z.number().positive() and then add .description() I don't lose .positive()), but I get that changing this now could break existing code. Any thoughts?

@samchungy
Copy link
Owner

samchungy commented Aug 7, 2024

Hey @mrichards42!

Thanks for raising the issue. This one is admittedly tricky, as I think this is modeled similar to .describe() where if you were to .describe() twice it would override the first.

I do see your conundrum, but I'm not certain if I want to make a change to make them merge. I'd have to think about the implications

@mrichards42
Copy link
Author

Agreed, I don't think this is straightforward, just wanted to raise the issue for discussion, and especially see if you had any thoughts that wouldn't require an invasive change.

I'm not sure there's really a good example of this pattern in zod itself, since openapi() sets multiple values at once, and most (all?) of the other zod methods are only for setting a single value, so the idea of "merging" wouldn't come into play.

@Papooch
Copy link

Papooch commented Sep 25, 2024

I stumbled upon the same thing. A lot of the times, we want to add special examples or description to existing schemas used in different contexts, but I was bummed to see that multiple .openapi() calls are overwritten by the last one.

Example:

const userId = z
   .string()
   .format('x-user-id') // we use custom formats to generate type-safe client SDK
   .openapi({ example: "usr-123-456" })
   
// elsewhere
const message = z.object({
   senderId: userId.openapi({ description: "id of the sender" }) // this overwrites the example
})

If you don't want to break existing code, then the two alternatives are:

  • introduce a new major version
  • add a new method, e.g. mergeOpenapi, which explicitly merges any existing openapi metadata

However, my first intuition was that multiple .openapi call would be merged, so the existing behavior is unexpected for me.

@samchungy
Copy link
Owner

samchungy commented Sep 25, 2024

Hmmm I'm considering maybe adding an opts object here: https://github.com/samchungy/zod-openapi/blob/master/src/extendZod.ts#L4 to be able to configure it.

Another issue I foresee with introducing this would be the ref. We'd need to explicit omit it or else that's going to error at runtime but I also don't want to overcomplicate the logic in the extend

eg.

const foo = z.string().openapi({ ref: 'foo' });

const foo2 = foo.openapi( { description: "foo" });
// Reference foo is already registered

@Papooch
Copy link

Papooch commented Sep 26, 2024

That makes sense to me, something like

extendZodWithOpenApi(z, {
  chainingBehavior: 'merge' // 'override'
  // or perhaps
  mergeChained: true // default false
})

You'd need to define a new function that would perform the merging and a set of properties that are never merged.

I don't know if there are any edge with nested objects or arrays, but I would personally not bother with that and just overwrite all repeated top-level properties.

@samchungy
Copy link
Owner

Hey, just keeping you updated but I'll have a form of this in the library over the next couple weeks.

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.

3 participants