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

Issues with generating OpenAI client in zod #12

Closed
mhaligowski opened this issue Nov 14, 2023 · 7 comments
Closed

Issues with generating OpenAI client in zod #12

mhaligowski opened this issue Nov 14, 2023 · 7 comments

Comments

@mhaligowski
Copy link

Great tool, incredibly helpful!

I am having issues when generating zod types for the OpenAI schema definition, available here: https://github.com/openai/openai-openapi/blob/master/openapi.yaml.

The generation works correctly, but when running tsc, I'm seeing issue like this:

src/generated/openai.ts:2117:5 - error TS2322: Type 'Promise<unknown>' is not assignable to type 'Promise<TypeOf<TEndpoint["response"]>>'.
  Type 'unknown' is not assignable to type 'TypeOf<TEndpoint["response"]>'.
    Type 'unknown' is not assignable to type 'string | { object: "model"; id: string; created: number; owned_by: string; } | { object: "list"; data: { object: "model"; id: string; created: number; owned_by: string; }[]; } | { object: "fine_tuning.job"; error: { code: string; message: string; param: string | null; } | null; status: "validating_files" | "queued" | "running" | "succeeded" | "failed" | "cancelled"; id: string; model: string; created_at: number; fine_tuned_model: string | null; finished_at: number | null; hyperparameters: { n_epochs: number | "auto"; }; organization_id: string; result_files: string[]; trained_tokens: number | null; training_file: string; validation_file: string | null; } | { object: "list"; data: { object: "fine_tuning.job"; error: { code: string; message: string; param: string | null; } | null; status: "validating_files" | "queued" | "running" | "succeeded" | "failed" | "cancelled"; id: string; model: string; created_at: number; fine_tuned_model: string | null; finished_at: number | null; hyperparameters: { n_epochs: number | "auto"; }; organization_id: string; result_files: string[]; trained_tokens: number | null; training_file: string; validation_file: string | null; }[]; has_more: boolean; } | { object: "file"; status: "error" | "uploaded" | "processed"; id: string; created_at: number; bytes: number; filename: string; purpose: "fine-tune" | "fine-tune-results" | "assistants" | "assistants_output"; status_details?: string | undefined; } | { object: "list"; data: { object: "file"; status: "error" | "uploaded" | "processed"; id: string; created_at: number; bytes: number; filename: string; purpose: "fine-tune" | "fine-tune-results" | "assistants" | "assistants_output"; status_details?: string | undefined; }[]; } | { object: "list"; data: { object: "fine_tuning.job.event"; message: string; id: string; created_at: number; level: "error" | "info" | "warn"; }[]; } | { object: "assistant"; id: string; model: string; name: string | null; description: string | null; tools: ({ type: "code_interpreter"; } | { type: "retrieval"; } | { function: { name: string; description?: string | undefined; parameters?: unknown; }; type: "function"; })[]; created_at: number; instructions: string | null; file_ids: string[]; metadata?: unknown; } | { object: string; data: { object: "assistant"; id: string; model: string; name: string | null; description: string | null; tools: ({ type: "code_interpreter"; } | { type: "retrieval"; } | { function: { name: string; description?: string | undefined; parameters?: unknown; }; type: "function"; })[]; created_at: number; instructions: string | null; file_ids: string[]; metadata?: unknown; }[]; has_more: boolean; first_id: string; last_id: string; } | { object: "thread.run"; status: "queued" | "failed" | "cancelled" | "in_progress" | "requires_action" | "cancelling" | "completed" | "expired"; id: string; model: string; tools: ({ type: "code_interpreter"; } | { type: "retrieval"; } | { function: { name: string; description?: string | undefined; parameters?: unknown; }; type: "function"; })[]; created_at: number; instructions: string; file_ids: string[]; thread_id: string; assistant_id: string; required_action: { type: "submit_tool_outputs"; submit_tool_outputs: { tool_calls: { function: { name: string; arguments: string; }; type: "function"; id: string; }[]; }; } | null; last_error: { code: "server_error" | "rate_limit_exceeded"; message: string; } | null; expires_at: number; started_at: number | null; cancelled_at: number | null; failed_at: number | null; completed_at: number | null; metadata?: unknown; } | { object: string; data: { object: "thread.run"; status: "queued" | "failed" | "cancelled" | "in_progress" | "requires_action" | "cancelling" | "completed" | "expired"; id: string; model: string; tools: ({ type: "code_interpreter"; } | { type: "retrieval"; } | { function: { name: string; description?: string | undefined; parameters?: unknown; }; type: "function"; })[]; created_at: number; instructions: string; file_ids: string[]; thread_id: string; assistant_id: string; required_action: { type: "submit_tool_outputs"; submit_tool_outputs: { tool_calls: { function: { name: string; arguments: string; }; type: "function"; id: string; }[]; }; } | null; last_error: { code: "server_error" | "rate_limit_exceeded"; message: string; } | null; expires_at: number; started_at: number | null; cancelled_at: number | null; failed_at: number | null; completed_at: number | null; metadata?: unknown; }[]; has_more: boolean; first_id: string; last_id: string; } | { object: "thread"; id: string; created_at: number; metadata?: unknown; } | { object: "thread.message"; id: string; content: ({ type: "image_file"; image_file: { file_id: string; }; } | { type: "text"; text: { value: string; annotations: ({ type: "file_citation"; text: string; file_citation: { file_id: string; quote: string; }; start_index: number; end_index: number; } | { type: "file_path"; text: string; start_index: number; end_index: number; file_path: { file_id: string; }; })[]; }; })[]; role: "user" | "assistant"; created_at: number; file_ids: string[]; thread_id: string; assistant_id: string | null; run_id: string | null; metadata?: unknown; } | { object: string; data: { object: "thread.message"; id: string; content: ({ type: "image_file"; image_file: { file_id: string; }; } | { type: "text"; text: { value: string; annotations: ({ type: "file_citation"; text: string; file_citation: { file_id: string; quote: string; }; start_index: number; end_index: number; } | { type: "file_path"; text: string; start_index: number; end_index: number; file_path: { file_id: string; }; })[]; }; })[]; role: "user" | "assistant"; created_at: number; file_ids: string[]; thread_id: string; assistant_id: string | null; run_id: string | null; metadata?: unknown; }[]; has_more: boolean; first_id: string; last_id: string; } | { object: "thread.run.step"; type: "tool_calls" | "message_creation"; status: "failed" | "cancelled" | "in_progress" | "completed" | "expired"; id: string; created_at: number; thread_id: string; assistant_id: string; last_error: { code: "server_error" | "rate_limit_exceeded"; message: string; } | null; cancelled_at: number | null; failed_at: number | null; completed_at: number | null; run_id: string; step_details: { type: "message_creation"; message_creation: { message_id: string; }; } | { type: "tool_calls"; tool_calls: ({ type: "code_interpreter"; id: string; code_interpreter: { input: string; outputs: ({ type: "logs"; logs: string; } | { type: "image"; image: { file_id: string; }; })[]; }; } | { type: "retrieval"; id: string; retrieval?: unknown; } | { function: { name: string; arguments: string; output: string | null; }; type: "function"; id: string; })[]; }; expired_at: number | null; metadata?: unknown; } | { object: string; data: { object: "thread.run.step"; type: "tool_calls" | "message_creation"; status: "failed" | "cancelled" | "in_progress" | "completed" | "expired"; id: string; created_at: number; thread_id: string; assistant_id: string; last_error: { code: "server_error" | "rate_limit_exceeded"; message: string; } | null; cancelled_at: number | null; failed_at: number | null; completed_at: number | null; run_id: string; step_details: { type: "message_creation"; message_creation: { message_id: string; }; } | { type: "tool_calls"; tool_calls: ({ type: "code_interpreter"; id: string; code_interpreter: { input: string; outputs: ({ type: "logs"; logs: string; } | { type: "image"; image: { file_id: string; }; })[]; }; } | { type: "retrieval"; id: string; retrieval?: unknown; } | { function: { name: string; arguments: string; output: string | null; }; type: "function"; id: string; })[]; }; expired_at: number | null; metadata?: unknown; }[]; has_more: boolean; first_id: string; last_id: string; } | { object: "assistant.file"; id: string; created_at: number; assistant_id: string; } | { object: string; data: { object: "assistant.file"; id: string; created_at: number; assistant_id: string; }[]; has_more: boolean; first_id: string; last_id: string; } | { object: "thread.message.file"; id: string; created_at: number; message_id: string; } | { object: string; data: { object: "thread.message.file"; id: string; created_at: number; message_id: string; }[]; has_more: boolean; first_id: string; last_id: string; }'.

2117     return this.fetcher('get', this.baseUrl + path, params[0]);
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

src/generated/openai.ts:2126:5 - error TS2322: Type 'Promise<unknown>' is not assignable to type 'Promise<TypeOf<TEndpoint["response"]>>'.
  Type 'unknown' is not assignable to type 'TypeOf<TEndpoint["response"]>'.
    Type 'unknown' is not assignable to type '{ object: string; id: string; deleted: boolean; } | { object: "file"; id: string; deleted: boolean; } | { object: "assistant.deleted"; id: string; deleted: boolean; } | { object: "thread.deleted"; id: string; deleted: boolean; } | { object: "assistant.file.deleted"; id: string; deleted: boolean; }'.

2126     return this.fetcher('delete', this.baseUrl + path, params[0]);
         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~


Found 2 errors in the same file, starting at: src/generated/openai.ts:2117

Interestingly, it doesn't have issues with the first method in the ApiClient class. I am running TypeScript 5.0.2, for that matter, typed-openapi isd on 0.2.0.

@astahmer
Copy link
Owner

got it, feel free to send a PR 🙏

@mhaligowski
Copy link
Author

@astahmer happy to do that, if only had some understanding of where does the error come from. any hints?

@nilshartmann
Copy link
Contributor

Hi @astahmer , @mhaligowski !

I think the problem is, that Fetcher in ApiClient does not know about the response type of a concrete request, because it is used as a member on ApiClient for all requests. And that makes totally sense, because it's just to load "anything" from "anyurl". I am not sure if it is possible to somehow solve this in typescript.

I could think of some other solutions:

  1. It would be possible to add a type cast in the return statement of the generated get/post/... methods in ApiClient code:
  // <ApiClient.get>
get<Path extends keyof GetEndpoints, TEndpoint extends GetEndpoints[Path]>(
  path: Path,
  ...params: MaybeOptionalArg<TEndpoint["parameters"]>
): Promise<TEndpoint["response"]> {
  return this.fetcher("get", this.baseUrl + path, params[0]) as Promise<TEndpoint["response"]> 
}
  1. It also would be possible to use zod's parse function here. That would not only infer the return type correctly, but also would verify at runtime, that the received response matches the received data:
  // <ApiClient.get>
get<Path extends keyof GetEndpoints, TEndpoint extends GetEndpoints[Path]>(
  path: Path,
  ...params: MaybeOptionalArg<z.infer<TEndpoint["parameters"]>>
): Promise<z.infer<TEndpoint["response"]>> {
  
  return this.fetcher("get", this.baseUrl + path, params[0])
    .then((unknownResponse) => EndpointByMethod["get"][path]["response"].parse(unknownResponse));
}

I think from a zod-perspective, choice 2 would be the better choice: eventhough it is more "opionionated", it would ensure that the received response is actually correct, while in choice one it's possible to receive a wrong payload, but TypeScript would think the payload would ever be the correct type. On the other hand, the fetcher implementation must make sure, that if it receives a status code that it does not want to parse (like http 400), it rejects the promise, so that the then function with the parse statement is not invoked. Using Axios as fetcher lib that would be default behaviour, when someone is using fetch instead, it must be implemented manually in the provided Fetcher function:

  const api = createApiClient((method, url, params) =>
     fetch(url, { method, body: JSON.stringify(params) })
      .then((res) => res.ok ? res.json() : throw new Error("invalid response")),
   );

Option number 3 would be to change the return type of the get, put, ...-methods to Promise<unknown> so it matches the Fetcher return type. It would then be up to the caller to cast or parse the returned unknown.

In any case I think, Fetcher function must return a Promise<unknown>:

  type Fetcher = (
  method: Method,
  url: string,
  parameters?: EndpointParameters | undefined,
) => Promise<unknown>;

What do you think?

@astahmer
Copy link
Owner

hey, I think option 1 (type casting) looks like a quick & easy solution here


I feel like using a runtime solution (in option 2) should be done in the fetcher itself tho, so that anyone can opt-in to this kind of behaviour

the point of this "headless" library was to not be opinionated on the underneath fetcher being used (fetch, axios, ky, etc) and how (validating input/output/both/none), as said in this issue I'm completely open to having a predefined fetcher implementation that satisfies most people's need

even better, if we start having predefined fetcher implementation if we could (through a config option/CLI flag) specify which one to use (none | validate-input etc)

all that said, I definitely don't want to repeat the openapi-zod-client scenario where everyone have very specific needs/opinion leading to more maintenance work and config options going nuts


option 3, I don't understand why this is needed, doing that would nullify most of the typings benefits from typed-openapi ? if this is really what's needed I recommend just doing a string .replace("Promise<Endpoint["response"]>;", "Promise<unknown"> after the file has been generated

@nilshartmann
Copy link
Contributor

@astahmer : I created a pull request with option 1. If there is anything I should change, please let me know.

Option 3 was a lousy workaround only to make the generated file compile and also make the consumer of the ApiClient aware that the returned object is not validated (in terms of zod).

@nilshartmann
Copy link
Contributor

Thanks @astahmer for merging my pull requests. I tested the newest version with my own api description and no more compile error arises! 🥳

@astahmer
Copy link
Owner

awesome ! thank you for the PRs 🙏

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

No branches or pull requests

3 participants