ferdikoomen / openapi-typescript-codegen

NodeJS library that generates Typescript or Javascript clients based on the OpenAPI specification
MIT License
2.89k stars 519 forks source link

Optional flag which changes string to date in case of format data-time or date #396

Open NicoVogel opened 3 years ago

NicoVogel commented 3 years ago

In the project I am working on, we use openapi v3. The schema contains objects which use the type string with the format date-time.

example:

ExampleType:
  title: ExampleType
  type: object
  properties:
    id:
      type: integer
    validFrom:
      type: string
      format: date-time
      example: "2012-10-27T17:18:30.966Z"

We did not want to convert the date everytime and therefore we created a middleware for the server and client-side that automatically converts it from string to Date. When creating the models via the openapi-typescript-gencode tool, the output models use the type string (as intended). But in our case we need it to be of type Date, which is a special case. Therefore an optional flag to enable this feature would be great.

What is needed?

It needs to remember the format value and in case of date or date-time the output type should be Date instead of string.

About implementing it

I would try to implement it and in case that anyone wants to help me, that would be great.

NicoVogel commented 3 years ago

I was actually able to implement it by simply passing a new flag into the handlebars and implementing the logic within the interface file (as can be seen here).

Even though the code works, I am sure you would not merge such a solution. I will try to implement the logic before passing the modle to the handlebars (e.g. here). But my current approach does not work yet.

NicoVogel commented 3 years ago

Finally, I had time to implement the feature outside of hbs (as can be seen here). I also intent to implement some tests.

Tamno commented 3 years ago

I would also like to have this feature. It would be interesting to merge that.

NicoVogel commented 3 years ago

I have not had time yet to implement the tests. Maybe next week.

ferdikoomen commented 3 years ago

@NicoVogel Sounds interesting, i know that at my current project we are also using this formatting. However the issue i see is: How do we convert back the date to a string (when we for instance send the model back to the server). The exact date-time format is not strictly specified in the spec as far as i know, but some API's need a specific string notation: https://swagger.io/docs/specification/data-models/data-types/ https://tools.ietf.org/html/rfc3339#section-5.6

NicoVogel commented 3 years ago

@ferdikoomen I did not consider this point, because the conversion was done automatically from the frameworks used. In my case Angular and Express. Also we only used date-time until now, therefore I did not notice the issue yet. I also imagine that date could be problematic, because the interceptors have no way of knowing weather a JS Date should be converted a string with the date or date-time format.

As long as only date-time is used, I see no problem.

In case of date, its a different story. To provide a generic solution for this some sort of object would be needed that contains information about which parameter should be parsed to a date string instead of date-time.

Example

Here an example that outlines a possible generic parser information

Schema

ExampleType:
  title: ExampleType
  type: object
  properties:
    id:
      type: integer
    validFrom:
      type: string
      format: date
      example: "2012-10-27"

[...]

SomeDate:
  title: SomeDate
  type: string
  format: date

Endpoint

/test:
    get:
      summary: Some text
      responses:
        "200":
          description: Some text
          content:
            application/json:
              schema:
                $ref: "#/components/schemas/ExampleType"
    post:
      summary: Some text
      parameters:
        - $ref: "#/components/parameters/SomeDate"
      requestBody:
        content:
          application/json:
            schema:
              $ref: "#/components/schemas/ExampleType"

Generated Interface

export interface ExampleType {
    id: number;
    validFrom: Date;
}

Generated Parser Information for Server

This file contains information about how the response needs to be changed before sending it back to the client.

/test:
    get:
      200:
        object:
          validFrom: date

The file could be located next to the index.ts in the generated directory and a possible name would be parserinfo-server.yaml.

Maybe use json instead of yaml to remove the need for a library in order to import it at runtime.

Generated Parser Information for Client

This file contains information about how the request needs to be changed before sending it to the server.

/test:
    post:
      parameters:
        SomeDate: date
      body:
        object:
          validFrom: date

The file could be located next to the index.ts in the generated directory and a possible name would be parserinfo-client.yaml.

Maybe use json instead of yaml to remove the need for a library in order to import it at runtime.

Sum up

The important part is that the interceptors have all information necessary in order to parse the messages accordingly. This example should contain everything needed in order to create a generic parser. Maybe some extra logic is needed for nullable and required, but this is only an example, therefore I skipped that part.

I think this is a lot of functionality just to get date working as expected. Hence my pull request for this issue will only address date-time and maybe I will create a second pull request which contains the necessary features for date.

@ferdikoomen Please leave your thoughts on this and point out if I missed anything.

NicoVogel commented 3 years ago

@NicoVogel Sounds interesting, i know that at my current project we are also using this formatting. However the issue i see is: How do we convert back the date to a string (when we for instance send the model back to the server). The exact date-time format is not strictly specified in the spec as far as i know, but some API's need a specific string notation: https://swagger.io/docs/specification/data-models/data-types/ https://tools.ietf.org/html/rfc3339#section-5.6

As far as I am concerned, date-time is pretty much strict. The only non strict information would be that the separator T can be upper or lower case (same goes for Z). Maybe I misunderstand this, but it does not seem like a problem to me.

Here is a similar interceptor which we also use: AngularRxJs5DateHttpInterceptor.ts

ferdikoomen commented 3 years ago

@NicoVogel let me have a look at some other production samples and get back to you. If date-time is indeed standard then that would be nice to auto-convert 🙂

NicoVogel commented 3 years ago

@ferdikoomen here a list of some frameworks and there conversion:

Express

Express is using JSON.stringify in case of res.json (source) and this is converting a Date via the toISOString() function (source)

Fastify

Fastify uses fast-json-stringify and it also uses toISOString (source)

Koa

Has no automatic JSON parsing (source)

NicoVogel commented 3 years ago

@ferdikoomen here a the list for the frontends:

Angular

uses JSON.stringify() (source)

axios

uses JSON.stringify() (source)

fetch

has no automatic parsing (source)

ferdikoomen commented 3 years ago

@Nico i've added this to the roadmap, seems like a feature that is worth investigation!

gavinkerr commented 1 year ago

I would love this flag too...

IMO the date/date-time should be mapped to Date() in the same way that format: int32 is already mapped to number... I would be happy for a flag to avoid breaking changes. (I assume it will still work for Swagger 2.0 too)

can this go back on the milestones?

mrlubos commented 6 months ago

Hey everyone, this feature is supported in @nicolas-chaulet/openapi-typescript-codegen. Any feedback would be appreciated!

leoerlandsson commented 6 months ago

Yes, please, merge into main :)

mrlubos commented 6 months ago

@leoerlandsson feel free to use the package I mentioned earlier, I'm not sure this is going to get merged anytime soon in this one