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

Not able to parse allOf with an item has required only #260

Closed
C-ra-ZY opened this issue Dec 25, 2023 · 1 comment · Fixed by #261 or #262
Closed

Not able to parse allOf with an item has required only #260

C-ra-ZY opened this issue Dec 25, 2023 · 1 comment · Fixed by #261 or #262

Comments

@C-ra-ZY
Copy link
Contributor

C-ra-ZY commented Dec 25, 2023

Describe the bug
We are attempting to modify the behavior of openapi-zod-client in pull request 258, with the expectation of correctly handling schemas within the allOf array that only contain the required field.
The goal is to generate the correct type annotations for required fields and the corresponding Zod schema, like https://editor.swagger.io/ does(It can even automatically eliminate properties that cannot be resolved or located, show in the pic: abc).
image

However, after testing, it has been observed that the current code is not yet able to fully address this issue.
and more of that, pull request 258 didn't take types generating into account. which should be supplemented.
image

Minimal reproduction
A simplified schema definition to reproduce this is like:

openapi: 3.0.3
info:
  title: User
  version: 1.0.0
paths:
  /user:
    get:
      responses:
        200:
          description: return user
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/userResponse'

components:
  schemas:
    user:
      type: object
      properties:
        name:
          type: string
        email:
          type: string
    userResponse:
      type: object
      properties:
        user:
          allOf:
            - $ref: '#/components/schemas/user'
            - required:
              - name

The generated client code:

import { makeApi, Zodios, type ZodiosOptions } from "@zodios/core";
import { z } from "zod";

type userResponse = Partial<{
    user: user & unknown;
}>;
type user = Partial<{
    name: string;
    email: string;
}>;

const user: z.ZodType<user> = z.object({ name: z.string(), email: z.string() }).passthrough();
const userResponse: z.ZodType<userResponse> = z
    .object({
        user: user.and(
            z
                .object({})
                .and(z.object({ name: z.unknown() }))
                .passthrough()
        ),
    })
    .passthrough();

export const schemas = {
    user,
    userResponse,
};

const endpoints = makeApi([
    {
        method: "get",
        path: "/user",
        requestFormat: "json",
        response: userResponse,
    },
]);

export const api = new Zodios(endpoints);

export function createApiClient(baseUrl: string, options?: ZodiosOptions) {
    return new Zodios(baseUrl, endpoints, options);
}

but the code will trigger an error:
image
and the schema fail to require correctly property(even when delete the unnecessary .passthrough()):
image

Expected behavior
we can generate types and schemas correctly like https://editor.swagger.io/.

Additional context
guys can easily check issue reproduction at https://github.com/C-ra-ZY/openapi-zod-client/tree/test-on-first-fix
run

npx tsx test.ts

and see the generated test.client.ts

@marrowleaves
Copy link
Contributor

marrowleaves commented Dec 28, 2023

@C-ra-ZY Thanks for finding the flaw in handling the .passthrough() for empty objects, but I don't this the proposed fix is a valid solution as it should not remove any required fields from the user spec.

As it's specified in OpenAPI spec

allOf takes an array of object definitions that are validated independently but together compose a single object.

The visual doc in https://editor.swagger.io/ has a misleading feature that it tries to merge allOf objects as I mentioned in issue 257 and it actually fails to display required but unknown property(that seems just a bug)

A better fix might be to have an ad hoc case handling of required only objects by removing the z.object({}) part and generating z.object({ name: z.unknown() }) directly?

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