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

Typescript: Dates Are Not Deserialized #2776

Open
pixelshaded opened this issue May 4, 2016 · 18 comments
Open

Typescript: Dates Are Not Deserialized #2776

pixelshaded opened this issue May 4, 2016 · 18 comments

Comments

@pixelshaded
Copy link
Contributor

pixelshaded commented May 4, 2016

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:
#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
Copy link
Contributor Author

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

@wing328
Copy link
Contributor

wing328 commented May 6, 2016

@pixelshaded thanks for reporting the issue. Would you have cycle to port the solution from JS to TS: #2030 ?

@pixelshaded
Copy link
Contributor Author

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
Copy link

moanrose commented Jun 9, 2016

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
Copy link
Contributor

wing328 commented Jun 17, 2016

@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: #3105 (comment))

@wing328 wing328 modified the milestones: v2.2.0, v2.3.0 Jul 7, 2016
@wing328 wing328 modified the milestones: v2.2.1, v2.2.2 Aug 8, 2016
@gregjacobs
Copy link

@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
Copy link

janslow commented Nov 27, 2016

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
Copy link

dinvlad commented Jun 20, 2017

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
Copy link

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
Copy link

dinvlad commented Jun 21, 2017

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
Copy link

@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
Copy link

karlvr commented Dec 20, 2017

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:

  • Allow the Swagger date type to be declared as a Date in the param creator, so our code can pass a Date, and then the param creator formats the Date to a string using toISOString() (which was existing code, but the method parameter type was string so it didn't work).
  • Fixed the toISOString() handling of the Swagger date type to not include the time component.
  • Change model class properties to their wire format, so instead of Date properties the properties are declared string. This is a true representation of the data, and we can then use other date handling functions to convert as we need to (nicely the date-fns package accepts strings or Dates so it's mostly painless).

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
Copy link
Contributor

wing328 commented Jan 3, 2018

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

@macjohnny
Copy link
Contributor

I suggest to configure the type mapping as suggested in #5587 (comment)

@Serg046
Copy link

Serg046 commented Apr 28, 2018

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
Copy link
Contributor

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
Copy link

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
Copy link

Phyllon commented Oct 12, 2022

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests