swagger-api / swagger-codegen

swagger-codegen contains a template-driven engine to generate documentation, API clients and server stubs in different languages by parsing your OpenAPI / Swagger definition.
http://swagger.io
Apache License 2.0
17.03k stars 6.03k forks source link

Typescript: Dates Are Not Deserialized #2776

Open pixelshaded opened 8 years ago

pixelshaded commented 8 years ago

If you have a property in a definition that is a string with date-time format, the typescript generated model makes that property a Date type. However, the response in the client is given verbatim. It never looks for date strings and actually transforms them in to Date objects. So the project will build, but fails at runtime if you try to access Date specific methods since it's actually a string.

I imagine this relates to: https://github.com/swagger-api/swagger-codegen/issues/1951

Since this PR focuses on deserializing dates in the response. I would suggest making those properties strings until deserialization is supported, otherwise the type checking isn't actually accurate.

pixelshaded commented 8 years ago

Noticing this for format: byte as well. Sets data type to ByteArray but is not deserializing that from the string.

wing328 commented 8 years ago

@pixelshaded thanks for reporting the issue. Would you have cycle to port the solution from JS to TS: https://github.com/swagger-api/swagger-codegen/pull/2030 ?

pixelshaded commented 8 years ago

Potentially.

Honestly I've got a workflow where the source swagger spec goes through a processor before going in to codegen. In this processor, I essentially make up for any short comings in codegen - remove format date-time and byte so I get strings as the data type, along with removing enums from definitions because they dont get generated correctly atm.

I may be able to take some time this weekend to start looking at these issues.

moanrose commented 8 years ago

I would suggest that the desiralization of date are not handled by swagger codegen, or that it is optional, because the proper place for this would be in the $httpProvider.defaults.transformResponse collection as described here http://aboutcode.net/2013/07/27/json-date-parsing-angularjs.html

wing328 commented 8 years ago

@moanrose thanks for the suggestion. For other langauges (e.g. C#, Java, Ruby, Python, etc), the auto-generated SDK handles the deserialization of string back into datetime object in the language automatically so doing the same in Angular 1or 2 API client will make the developer experience more consistent.

If it's more common for Angular 1 or 2 developers to handle the deserialization in their own program, then one workaround I can think of is to document the datetime string as type: string in the OpenAPI/Swagger spec instead.

(Re-post: https://github.com/swagger-api/swagger-codegen/issues/3105#issuecomment-226740310)

gregjacobs commented 7 years ago

@moanrose I agree with @wing328 that the deserialization should happen as part of the generated code.

The article you linked to simply provides one option for a place to deserialize date objects in Angular code, but it doesn't necessarily mean that it's the "proper" place to deserialize date objects. In fact, from a software engineering perspective, I would argue that $httpProvider.defaults.transformResponse is not a good place to put this sort of transformation, as it is tying your code specifically to the Angular 1 framework. This makes upgrading your code to Angular 2, or switching frameworks in general more difficult.

janslow commented 7 years ago

There are also the other TypeScript clients (e.g., TypeScript Fetch) which don't have a feature like $httpProvider.defaults.transformResponse, so deserializing needs to happen in the generated code.

dinvlad commented 7 years ago

So we already define DateTime fields in swagger.json as

"DateTime": {
  "format": "date-time",
  "type": "string"
},

However, they do not get deserialized from string (ex. "2017-06-20T20:58:29.104Z") and instead are defined to be of Date type in the generated code (at least, for Angular 2). Do we still expect this conversion to happen automatically at some point?

EDIT: using 2.3.0 branch from a week ago.

EDIT2: to work around that, currently I have to name the field as Date instead of DateTime. Then, Codegen complains that there is a name collision with existing language type, and proceeds to rename that type as ModelDate. I then just have to interpret ModelDate as string.

Thanks

moanrose commented 7 years ago

What we ended up doing, and what works for us is a combination of the following

We are using JSJoda - LocalDate, LocalDateTime on the client to keep dates and date times seperated.

We have configured Spring Boot to serialize all dateTime to a zoned datetime format in UTC - which get deserialized using Local Time Zone on the client. The Dates gets serialized as ISO 8601 Date

We use regular expressions to substitute properties with matching date formats, to instances of corresponding JSJoda date types - in a recursive fashion.

We have modified both the AbstractTypeScriptClientCodegen, api.mustache, and model.mustache to accomplish this

dinvlad commented 7 years ago

FWIW, I opted to pass the date fields through the Date pipe in Angular to display them in local time. With that, the input can either be an ISO string or a Date object. But if we test these with Date objects, technically the test input differs from the runtime input of ISO strings, even though the results look the same. So it's just a matter of being pedantic for now.

But if we haven't used pipes, the manual deserialization to local time (e.g., using toLocaleString()) would incorrectly assume that we get Date objects, and would pass unit tests while failing in production (unless we catch that in e2e). A workaround for that would be to manually create Date objects out of those fields first, e.g. new Date(data.createdAt).toLocaleString(). That would handle both Date and string inputs equally, although we couldn't unit-test with strings due to type being enforced to be Date (unless we also use the ModelDate workaround from above).

All in all, this is still inconsistent behavior, in that it defines the date-time fields to be of Date type, while not doing anything on the real outputs to turn them into Dates. We need to either keep them defined as strings (as they are in Swagger), or do an explicit conversion in the generated code after http.request(). Since the latter requires more work, it'd be easiest to just keep them as strings. Or to define them to be of compound type string | Date to reduce the potential of incompatibilities with existing clients, while also handling the correct runtime type (string), which could be assumed during unit tests.

jamieathans commented 7 years ago

@dinvlad I like Swagger date-time translating to typescript compound type string | Date and would be an easy fix.

It would also be easy to write mapping utility functions:

function toDate(dateOrString: string | Date): Date;

function toISODateString(dateOrString: string | Date): string;

karlvr commented 6 years ago

I've run into this problem and I've made a fork and a branch to fix it for our use case. We are using the typescript-fetch config with React + Redux.

https://github.com/karlvr/swagger-codegen/tree/correct-types

The changes I've made:

My implementation is very typescript-fetch specific, and the introduction of the getWireType(Property) method next to getTypeDeclaration(Property) is a bit ridiculous, given the duplicated code that would be needed. Works for me. If it's of any interest as a more general solution I'd be happy to look at an alternative approach.

wing328 commented 6 years ago

@karlvr thanks for sharing the solution. Do you mind submitting a PR so that we can more easily review the fix?

macjohnny commented 6 years ago

I suggest to configure the type mapping as suggested in https://github.com/swagger-api/swagger-codegen/issues/5587#issuecomment-368805748

Serg046 commented 6 years ago

Some minutes ago I was going to fix an issue by @wing328 way described here. However I see 7 typescript templates! They are pretty different and don't have a common shared logic. I don't understand which exactly template should be fixed in scope of the current "bug". Maybe we should extract http request logic to a separate file and reuse it in all templates. But then it's strange to have templates like typescript-fetch.

Also it looks like at least one template doesn't have datetime issue: see typescript-node

I see there is a beautiful common javascript template. It would be cool to have typescript analog. So what should be the next step?

tsiq-jeremy commented 6 years ago

I think I'm experiencing the same issue, but wanted to double check. The main issue is when using type string and format date. The code it generates requires an input parameter of a string, but then tries to call a function on it as if it is a Javascript Date object. This seems to be an issue of Serialization, not Deserialization though.

The second bug that this issue is not addressing is that regardless if the format is date or date-time it uses toISOString to format the date. date should use another function to get the format to be YYYY-MM-DD as specified here.

Are there any plans to fix these bugs? Should I open up a PR to fix these issues in typescript-fetch?

Part of swagger spec:

{
  "name": "myDate",
  "type": "string",
  "format": "date",
}

typescript-fetch produced:

myFunction(myDate?: string, options: any = {}): FetchArgs {
    ... 
    const localVarQueryParameter = {} as any;

    if (myDate !== undefined) {
        localVarQueryParameter['myDate'] = (myDate as any).toISOString();
    }
    ...
},
juancarrey commented 4 years ago

If you are using autogenerated-config.json you can add this config so it gets autogenerated to: startDate: string | Date

"typeMappings": {
    "Date": "string | Date"
  }
Phyllon commented 2 years ago

Any news on this topic? This bug seems still to exist.