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

Inherit zod schema description by default #104

Closed
j-murata opened this issue Mar 15, 2023 · 19 comments · Fixed by #111 or #117
Closed

Inherit zod schema description by default #104

j-murata opened this issue Mar 15, 2023 · 19 comments · Fixed by #111 or #117

Comments

@j-murata
Copy link

j-murata commented Mar 15, 2023

schema.openapi({ description: 'Some string' });

I am aware that the extension method .openapi() can be used to set the description, as shown above.
However, currently, it appears that descriptions of schema other than the root schema that is the target of register(), i.e., child or grandchild schema, are not automatically inherited. (See #95. My apologies if I am wrong.)

I think it is great that the .openapi() method provides a way to override the description, but it would be even better if the original description could be inherited by default.

Currently, I am handling this by calling my own recursive functions such as the following in advance, but it would be very helpful if these could be done inside functions such as register().

import { ZodTuple, ZodType } from 'zod';

const ZOD_TYPE = 'ZodType';
const zodDefKeyTypes = Object.setPrototypeOf({
    type        : ZOD_TYPE,
    options     : 'Array|Object',
    left        : ZOD_TYPE,
    right       : ZOD_TYPE,
    items       : 'Array',
    rest        : ZOD_TYPE,
    keyType     : ZOD_TYPE,
    valueType   : ZOD_TYPE,
    args        : ZOD_TYPE,
    returns     : ZOD_TYPE,
    schema      : ZOD_TYPE,
    innerType   : ZOD_TYPE,
    in          : ZOD_TYPE,
    out         : ZOD_TYPE,
}, null);

function addOpenApiMetadata(target) {
    if (!(target instanceof ZodType)) {
        throw new TypeError('The passed value is not a `ZodType`.');
    }
    if (target instanceof ZodTuple) {
        target = zodTupleToArray(target);
    }
    const { _def, description, shape } = target;
    _def.openapi || Object.assign(_def, {
        openapi: {
            _internal: undefined,
            metadata: { description, param: undefined },
        },
    });
    if (typeof shape === 'object') {
        for (const [key, value] of Object.entries(shape)) {
            shape[key] = addOpenApiMetadata(value);
        }
    } else {
        for (const [key, value] of Object.entries(_def)) {
            if (!(key in zodDefKeyTypes)) {
                continue;
            }
            if (zodDefKeyTypes[key] === ZOD_TYPE) {
                _def[key] = addOpenApiMetadata(value);
                continue;
            }
            for (const [k, v] of Object.entries(value)) {
                value[k] = addOpenApiMetadata(v);
            }
        }
    }
    return target;
}

Note: See #103 for an implementation of zodTupleToArray.

@AGalabov
Copy link
Collaborator

Hey @j-murata would you be able to provide an example. I.e what zod schema you would want to use and what the returned OpenAPI should be? I think I got the overall idea but it would be great if I can see something that showcases the problem directly 🙏 .

@j-murata
Copy link
Author

For example, suppose the following schema is defined.

schema.mjs
import { z } from "zod";

export const errorResponse = z.object({
    type: z.string().default('about:blank').describe('A URI reference [RFC3986](https://datatracker.ietf.org/doc/html/rfc3986) that identifies the problem type'),
    title: z.string().describe('A short, human-readable summary of the problem type'),
    status: z.number().int().min(400).describe('The HTTP status code ([RFC9110, Section 15](https://datatracker.ietf.org/doc/html/rfc9110#section-15)) generated by the origin server for this occurrence of the problem'),
    instance: z.string().describe('A URI reference that identifies the specific occurrence of the problem'),
    detail: z.string().describe('A human-readable explanation specific to this occurrence of the problem'),
}).partial().describe('[RFC 7807 - Problem Details for HTTP APIs](https://datatracker.ietf.org/doc/html/rfc7807)');

For this schema, call register() and then generateDocument() as follows.

api.mjs
import { z } from 'zod';
import {
    extendZodWithOpenApi,
    OpenAPIGenerator,
    OpenAPIRegistry,
} from '@asteasolutions/zod-to-openapi';

import { errorResponse } from './schema.mjs';

extendZodWithOpenApi(z);

const apiRegistry = new OpenAPIRegistry();
apiRegistry.register('ErrorResponse', errorResponse);

const apiGenerator = new OpenAPIGenerator(apiRegistry.definitions, '3.1.0');
export const apiDocument = apiGenerator.generateDocument();

The generated apiDocument will output YAML like this. (notice the absence of the description field.)

spec.yml
openapi: 3.1.0
components:
  schemas:
    ErrorResponse:
      type: object
      properties:
        type:
          type: string
          default: about:blank
        title:
          type: string
        status:
          type: integer
          minimum: 400
        instance:
          type: string
        detail:
          type: string
  parameters: {}
paths: {}

On the other hand, if you call register() on the schema through addOpenApiMetadata() above, the output YAML will change as follows.

+import { addOpenApiMetadata } from './util.mjs';
 import { errorResponse } from './schema.mjs';

 extendZodWithOpenApi(z);

 const apiRegistry = new OpenAPIRegistry();

-apiRegistry.register('ErrorResponse', errorResponse);
+apiRegistry.register('ErrorResponse', addOpenApiMetadata(errorResponse));

spec-new.yml
openapi: 3.1.0
components:
  schemas:
    ErrorResponse:
      type: object
      properties:
        type:
          type: string
          default: about:blank
          description: A URI reference [RFC3986](https://datatracker.ietf.org/doc/html/rfc3986) that identifies the problem type
        title:
          type: string
          description: A short, human-readable summary of the problem type
        status:
          type: integer
          minimum: 400
          description: The HTTP status code ([RFC9110, Section 15](https://datatracker.ietf.org/doc/html/rfc9110#section-15)) generated by the origin server for this occurrence of the problem
        instance:
          type: string
          description: A URI reference that identifies the specific occurrence of the problem
        detail:
          type: string
          description: A human-readable explanation specific to this occurrence of the problem
      description: "[RFC 7807 - Problem Details for HTTP APIs](https://datatracker.ietf.org/doc/html/rfc7807)"
  parameters: {}
paths: {}

@AGalabov
Copy link
Collaborator

@j-murata I get it now. Great catch. I'll check where the issue is as this is definitely not the expected behavior.

@AGalabov
Copy link
Collaborator

@j-murata hmmm I just wrote some tests and they were all passing. That lead me to take a closer look at your code:

Your schema errorResponse is defined in a separate file. But within that file you've not used extendZodWithOpenApi (described here). Now that is a problem for you because we've overriden .describe to call .openapi.

So for example if you just inline your schema in the api.mjs file after the extendZodWithOpenApi it generates the correct schema.

Note: This is not only a problem for descriptions. Right now if you just add .openapi({ ...whatever }) in your errorResponse you'd actually get an error that .openapi is not defined.

Please check if this answers your issues and if you can resolve it with this information and get back to us 🙏

@j-murata
Copy link
Author

@AGalabov
I see. You mean I had to run extendZodWithOpenApi(z) before calling .describe()?
I will check it out. Thanks.

@j-murata
Copy link
Author

I made the following changes to schema.mjs and confirmed that the description fields are included in the output YAML, just like you said! Thanks.

schema.mjs
import { z } from "zod";
import { extendZodWithOpenApi } from '@asteasolutions/zod-to-openapi';

extendZodWithOpenApi(z);

export const errorResponse = z.object({
    ...
}).partial().describe('...');

// console.dir(errorResponse, { depth: null });

@j-murata
Copy link
Author

@AGalabov

Overriding zod's .describe() method inside extendZodWithOpenApi() may indeed be a reasonable solution.
However, from my use case, having to call this function when defining the schema seems a bit unusable.
This is because, in general, schema definitions using zod are not only for API specifications, but also for generating input validators, generating TypeScript type definitions, and for various other uses.
Of course it would be natural to call extendZodWithOpenApi() when creating an API specification, i.e. in api.mjs in the example above. But as far as schema.mjs is concerned, importing zod-to-openapi and calling this function seems unnatural to me.

flowchart LR
    S([Zod schema])
    A([API spec])
    I([Input validator])
    T([*.d.ts files])
    S --> A
    S --> I
    S --> T
Loading

Therefore, I would be happy if you could consider a different solution to this in the future, i.e. not overriding .describe(), but referring to and inheriting the value of description later.

@AGalabov
Copy link
Collaborator

@j-murata I get your point. And that is what we are doing in our project as well. Basically we have multiple schemas spread across files that we use for documentation (through the registry), validation and type coverage. And I do get your point about the description specifically, however with your current approach you wouldn't be able to use .openapi in that file at all - that means that you cannot "register" a function either.

I can share some insight on how we have setup our project:

  1. We have a utils/zod file that calls extendZodWithOpenApi and reexports z.
  2. We use that version everywhere in our project (we have an eslint rule not to import from 'zod')
  • this should also be achievable if you have an entrypoint where zod is extended and everything else is created afterwards
  1. We've exported a registry from a separate file that can be used where schemas are defined. That's just useful for us so that we can just use that exported registry where needed instead of importing all schemas that need to be registered.

All that being said if it is really the description that you want, then that shouldn't be a problem. It would be a slightly less elegant solution but it can work. Just let me know if the explanation above changes your perspective on the matter. If not I'll see how the implementation should change to account for it.

@j-murata
Copy link
Author

j-murata commented Mar 17, 2023

@AGalabov Thanks for your reply.

Just let me know if the explanation above changes your perspective on the matter.

I may not fully understand your explanation, but my perspective on this probably remains the same.
I feel that your project setup guidelines that you have described would not be possible without the assumption that everything is in the same repository or under our own control.

For example, suppose there is a repository for schema definitions that is shared by several teams.

  • This repository exports many zod schema definitions.
  • These definitions are referenced not only by the API server team, but also by other teams. (e.g. DB server team, client app team, etc.)
  • Of course zod is included in the "dependencies" in the package.json of this repository, but we cannot install zod-to-openapi just for the convenience of the API server team.
    • It is probably possible, but it means that we should not make changes just for the convenience of the API server team.

There is another repository, separate from the above, dedicated to the API server team.

  • This repository has zod-to-openapi installed (so it is possible to call .openapi()).
  • This repository imports only the schema definitions needed for the API server from the above shared repository, combines them appropriately, and builds the API specification through OpenAPIRegistry.
  • The description of each field may be overwritten if necessary, but basically we want to use the original definition as it is.
flowchart LR
    S[(repo for\nCommon Schema)]
    C([repo for\nClient App])
    A([repo for\nAPI Server])
    B([repo for\nDB Server])
    C -.-> S
    A -.-> S
    B -.-> S
    C -.-> A
Loading

@antonioalmeida
Copy link

antonioalmeida commented Mar 19, 2023

I can share some insight on how we have setup our project:

  1. We have a utils/zod file that calls extendZodWithOpenApi and reexports z.

  2. We use that version everywhere in our project (we have an eslint rule not to import from 'zod')

  • this should also be achievable if you have an entrypoint where zod is extended and everything else is created afterwards
  1. We've exported a registry from a separate file that can be used where schemas are defined. That's just useful for us so that we can just use that exported registry where needed instead of importing all schemas that need to be registered.

Hey @AGalabov, thanks for detailing how you approach this in your own projects. I feel like these patterns are valuable information to share as part of the README.md , perhaps with an example and even the eslint rule

@AGalabov
Copy link
Collaborator

AGalabov commented Mar 20, 2023

@j-murata I see your point now, thanks for the explanation. With that information provided I can agree that logic that depends on inner zod workings (length/description/regex, ...) that are already present per schema should in fact be retrieved from the schemas themselves as opposed to relying on the overrides and the call of .openapi.

I'll make the necessary changes asap.

@j-murata
Copy link
Author

@AGalabov Thank you for your understanding.

I'll make the necessary changes asap.

I myself am not in such an urgent need, so there is no need for you to respond so quickly.
I hope zod-to-openapi will become a more usable library!:wink:

@j-murata
Copy link
Author

@AGalabov Any updates on this?

I found a workaround that intentionally delays the loading of the schemas.
This is not very smart, but it works for now.

import { z } from 'zod';
import { extendZodWithOpenApi } from '@asteasolutions/zod-to-openapi';
// import { errorResponse } from './schema.mjs';
extendZodWithOpenApi(z);
const { errorResponse } = await import('./schema.mjs');

@AGalabov
Copy link
Collaborator

Hey @j-murata sorry for the delay. I am waiting on a Code review. We were kind of busy at work, but I'll ping around today.

AGalabov added a commit that referenced this issue Mar 31, 2023
…available-without-zod-overrides

use zod schema description when generating components instead of only .openapi
@AGalabov
Copy link
Collaborator

@j-murata you can give it a go using the latest v4.5.2 release.

@j-murata
Copy link
Author

j-murata commented Apr 1, 2023

@AGalabov
Unfortunately, it does not seem to output the descriptions of nested child and grandchild objects.
In the example above, the description of the errorResponse itself is output, but the description of each property under it is not.
Do you have any ideas on how to resolve this?

@AGalabov AGalabov reopened this Apr 13, 2023
@AGalabov
Copy link
Collaborator

@j-murata sorry, I kind of missed your message since the issue was preemptively closed by me :/ I saw it randomly in my mail. You are correct I've missed a place in the code where we were relying on the description from .openapi() and were not using the one from .describe(). I opened a new PR that should resolve it.

@AGalabov AGalabov linked a pull request Apr 15, 2023 that will close this issue
AGalabov added a commit that referenced this issue Apr 16, 2023
…b.com:asteasolutions/zod-to-openapi into enhancement/#99-automatic-registration
AGalabov added a commit that referenced this issue Apr 19, 2023
…ption-not-being-used

Bugfix/#104 nested description not being used
@AGalabov
Copy link
Collaborator

@j-murata this should hopefully be fully fixed in the v4.6.0 release. Please let me know - Github closed the issue on its own since the PR was linked

@j-murata
Copy link
Author

j-murata commented Apr 20, 2023

@AGalabov
I used v4.6.0 for my own codes and had no problems at all. Thank you for your long support!

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