hey-api / openapi-ts

✨ Turn your OpenAPI specification into a beautiful TypeScript client
https://heyapi.vercel.app
Other
1.01k stars 83 forks source link

Help catch non declared query or path parameters #1000

Open mikavilpas opened 1 week ago

mikavilpas commented 1 week ago

Description

Hi, and thanks for this wonderful project!

At work, my team ran into an issue where we were accidentally passing in path parameters instead of query parameters, causing a profile search api to return a much larger dataset than what we expected 🙂

diff --git a/packages/server/src/services/employments/temporaryEmployments.ts b/packages/server/src/services/employments/temporaryEmployments.ts
index f69e2a0..a2cf2aa 100644
--- a/packages/server/src/services/employments/temporaryEmployments.ts
+++ b/packages/server/src/services/employments/temporaryEmployments.ts
@@ -9,7 +9,7 @@ export const getTemporaryEmployments = async (profileId: string): Promise<V1Temp
   try {
     const response = await EmploymentService.searchTemporaryEmployments({
       client,
-      path: { profileId },
+      query: { profileId },
     })
     return response.data
   } catch (error) {

The generated data type for this api does not declare path options in the openapi specification. It's defined as /v0/temporaryEmployment/search:.

It looks like the defaults come from

interface RequestOptionsBase<ThrowOnError extends boolean> extends Config<ThrowOnError> {
    path?: Record<string, unknown>;
    query?: Record<string, unknown>;
    url: string;
}

Is there a way to disallow specifying parameters that are not defined by the API specification?

mrlubos commented 1 week ago

@mikavilpas Can you share your OpenAPI spec?

mikavilpas commented 1 week ago

I can't share the full spec, but I created a reproduction setup here https://github.com/mikavilpas/hey-api-reproduction-1000

Let me know if this helps.

mrlubos commented 1 week ago

@mikavilpas Oh so you're asking for strict types? That is, unless something is explicitly defined as a possible value by the spec, don't allow it to be passed?

mikavilpas commented 1 week ago

Yes, that's it! If there's any way to opt into this kind of behaviour, it would be perfect.

I'm building a new openapi client for our company's internal api with my team, and it would be great to enforce strict adherence to the spec like this.

mrlubos commented 1 week ago

I'd need to think about it, this would have to be done on the (fetch/axios) client level. If you can think of anything feel free to open a pull request! We'd need to modify the types to either be more strict or loose as they're now depending on some generic variable

mikavilpas commented 1 week ago

I have an idea. It seems like it might be enough to add a missing path or query parameter into the generated types as an explicitly disallowed parameter:

diff --git a/examples/openapi-ts-fetch/src/client/types.gen.ts b/examples/openapi-ts-fetch/src/client/types.gen.ts
index 9dbad3c8..3d90d297 100644
--- a/examples/openapi-ts-fetch/src/client/types.gen.ts
+++ b/examples/openapi-ts-fetch/src/client/types.gen.ts
@@ -118,6 +118,7 @@ export type UpdatePetResponse = Pet;
 export type UpdatePetError = unknown;

 export type FindPetsByStatusData = {
+  path?: undefined;
   query?: {
     /**
      * Status values that need to be considered for filter

https://github.com/user-attachments/assets/bb4b923c-e0be-4f0c-bb65-b43518ea8811

mrlubos commented 1 week ago

This would then shift the ownership over strictness to the generator instead of the client, correct? I could get behind that. My immediate reaction is we'd want a types.strict optional boolean flag. Did you have a different idea?

mikavilpas commented 1 week ago

Yeah I think it would have to be done in each client with this solution 🤔

mikavilpas commented 1 week ago

Oh wait, did I get it backwards?

mrlubos commented 1 week ago

As long as the client understands path?: undefined (or even never) and resolves it to disallow arbitrary values, that would accomplish what we need here. I like this idea!