openapi-ts icon indicating copy to clipboard operation
openapi-ts copied to clipboard

feat: add request headers option

Open whosnero opened this issue 1 year ago • 14 comments

We have the requirement to set request headers differently for different parts of the business logic. This PR allows to put in some additional request headers in each operation call. The new parameter is made optional because we’re sure that most users won’t need it. It is furthermore an object because we plan on adding more options that we require in our use cases in the future. (e.g. support for request/response validation and selection among multiple media types).

Cc @eugenk

whosnero avatar May 15 '24 14:05 whosnero

Review PR in StackBlitz Codeflow Run & review this pull request in StackBlitz Codeflow.

🦋 Changeset detected

Latest commit: 220564d5629254636cdbb6fabbffb3ae6cfcd1a4

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
@hey-api/openapi-ts Minor

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

changeset-bot[bot] avatar May 15 '24 14:05 changeset-bot[bot]

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Comments Updated (UTC)
hey-api-docs ✅ Ready (Inspect) Visit Preview 💬 Add feedback May 15, 2024 2:51pm

vercel[bot] avatar May 15 '24 14:05 vercel[bot]

@whosnero can you use request interceptors to resolve your issue?

jordanshatford avatar May 16 '24 20:05 jordanshatford

support for request/response validation and selection among multiple media types

@whosnero can you expand on this please?

mrlubos avatar May 16 '24 20:05 mrlubos

Hi! I'm taking over parts of the conversation for @whosnero here.

can you use request interceptors to resolve your issue

Probably we could, but this sounds to me a little dirty. Why introduce another level of indirection and add another technology/pattern if you can specify arguments for a function call where the function call happens?

The generated API client is a convenient OOP way to send HTTP requests. Keeping this in mind, every operation call is an HTTP request. Developers who use this client are aware of it. So they have at least some HTTP semantics in mind when using the client. Allowing (not requiring) them to add optional HTTP properties would be decent, don't you agree? :)

eugenk avatar May 17 '24 04:05 eugenk

support for request/response validation and selection among multiple media types can you expand on this please?

I'm happy to! This is a longer tangent, though. I will split this into two posts for a clearer structure.

Let's begin with "selection among multiple media types": Let's say you have an OpenAPI spec like

paths:
  /foo:
    get:
      operationId: getFoo
      responses:
        "200":
          description: OK
          content:
            application/vnd.myapp.foo.get.v2+json:
              schema:
                type: object
                required:
                  - description
                  - createdAt
                properties:
                  description:
                    type: string
                  createdAt:
                    type: string
                    format: date-time
            application/vnd.myapp.foo.get.v1+json:
              schema:
                type: object
                required:
                  - text
                properties:
                  text:
                    type: string
                    enum:
                      - foo1
                      - foo2

Here we have two media types for the foo resource: application/vnd.myapp.foo.get.v1+json and application/vnd.myapp.foo.get.v2+json. The current generated client always sends Accept: application/vnd.myapp.foo.get.v2+json because this media type the first one in the spec. In most cases, this is totally fine. In an ideal world, these two media types are only different representations of the exact same backend resource. And from perspective of functionality, it does not matter which one is used by the client.

However, as we all know, the real world is messy. Sometimes you run into edge cases where the backend needs to do some filtering depending on the Accept header such that the backend might respond with HTTP-404 for one media type and with HTTP-200 for the other. We don't live in an ideal world - real-world projects always become messy after a while.

In our project, there are client-applications that, in some business-cases need to request one media type and in some other business cases need to request another (during a migration phase).

Long story short: It would be nice to allow to generate code that includes support for all specified media types of each endpoint. The client-application could choose which one to use on every operation call, defaulting to the one that is already selected. The same goes for request bodies of PUT/PATCH/POST etc. as in this spec:

paths:
  /foo:
    get: [...]
    put:
      operationId: putFoo
      requestBody:
        required: true
        content:
          "application/vnd.myapp.foo.put.v2+json":
            schema:
              type: object
              required:
                - description
              properties:
                description:
                  type: string
          "application/vnd.myapp.foo.put.v1+json":
            schema:
              type: object
              required:
                - text
              properties:
                text:
                  type: string
                  enum:
                    - foo1
                    - foo2
      responses:
        "204":
          description: OK

eugenk avatar May 17 '24 05:05 eugenk

support for request/response validation and selection among multiple media types can you expand on this please?

Second part: request/response validation

Currently, we are trusting the backend to send us properly structured data and naively assure the user that the type is actually correct. However, the backend might have a bug that causes properties to be undefined/null or to have the wrong type, etc.

We have all the information we need to actually validate the data that we receive from the backend. The spec is already there. We could use ajv or something similar to actually assure the user that the backend has sent us valid data. Of course, this costs some performance, so I would not make it the default. However, I would like to give users the choice to activate validation. It could even be an option of the code-generator to include or exclude validation logic.

If we allow to validate the backend's responses, it's just consistent to also allow to validate the client's request. It's not only about the types, it's also about format of a string, min/max of a number, length of an array, etc.. Of course, this should also be an option, off by default.

So this has been a long tangent, not really on topic of this pull request. I hope I could clarify our intentions.

Both would benefit us in our project quite a lot and we think we're not the only ones who would. I think this project is the correct place to implement such features and we are happy to contribute.

eugenk avatar May 17 '24 05:05 eugenk

@eugenk first part: in clients, you'll be able to pass headers per each request. I believe that should take care of this issue. For different bodies, we need to update generated types if that's not already being generated correctly today. You'll still be able to mismatch these and send header one with body two, but to add support for that would involve way more typing work than I am willing to invest at the moment. I believe this solution would be acceptable, what do you think?

mrlubos avatar May 17 '24 06:05 mrlubos

@eugenk second part: 100% agree, and this is on the roadmap. I am pretty sure I heard this request before. It's just going to take some time to get there, bear with me...

mrlubos avatar May 17 '24 06:05 mrlubos

Sorry for taking so long to answer. I have been off for a few days.

@eugenk first part: in clients, you'll be able to pass headers per each request

Oh yes. I did not know about this. This sounds perfect for that case!

@eugenk second part: 100% agree, and this is on the roadmap

Is this about validation? So no action required from our side?

I have also other things in mind that would be very useful to us and probably also useful to others. What's the correct place to talk about these things?

eugenk avatar May 22 '24 10:05 eugenk

@eugenk feel free to open a new issue or discussion if one doesn't already exist! Have you seen the new Fetch API client I published yesterday?

mrlubos avatar May 22 '24 10:05 mrlubos

Have you seen the new Fetch API client I published yesterday?

Thanks for letting me know! :) I have just played around with it and it looks like this would solve some of our issues with generated clients. Especially, this gives you access to the response object which is really important because you often need not only the response body but also the response headers (e.g. the pagination headers). I love it!

For different bodies, we need to update generated types if that's not already being generated correctly today. You'll still be able to mismatch these and send header one with body two, but to add support for that would involve way more typing work than I am willing to invest at the moment. I believe this solution would be acceptable, what do you think?

Right, this is quite some work around the generated types. I'm just thinking out loud: For the PUT request of the spec above, currently this type is generated:

export type PutFooData = {
  body: {
    description: string;
  };
}

and if we instead generate this:

export type PutFooData = {
  contentType?: "application/vnd.myapp.foo.put.v2+json",
  body: {
    description: string;
  };
} | {
  contentType: "application/vnd.myapp.foo.put.v1+json",
  body: {
    text: "foo1" | "foo2";
  };
};

the user would be able to select the media type like this:

await putFoo({
  client,
  body: { description: "abc" },
});

await putFoo({
  client,
  contentType: "application/vnd.myapp.foo.put.v2+json",
  body: { description: "abc" },
});

await putFoo({
  client,
  contentType: "application/vnd.myapp.foo.put.v1+json",
  body: { text: "foo1" },
});

For the request "Accept" Header this would be more involved. I can see why you are hesitant to put in the typing work.

eugenk avatar May 23 '24 06:05 eugenk

Yep! The current objective is to get the Axios client out, and once that's done, I'll be able to converge on common interface for both clients. I worry I could end up over optimising for fetch if I get into these details, and would need to undo some of that work to get it working with Axios.

Are you able to open a new issue for the most critical issues that need to be fixed in order to make this work for you with the new Fetch API client?

mrlubos avatar May 23 '24 08:05 mrlubos

Sorry for responding so late. I will add issues in Github as soon as I find time to take a deep look at your current code and formulate it in a useful way. Your development speed is remarkable! With all the things going on on my side, I can't keep up with your progress 😅

This PR can be closed though.

eugenk avatar Jul 03 '24 05:07 eugenk