Nswag: Cannot use non-string Date Time type with Interface DTOs

[danzel]

If you configure NSwag like so:

Then call a method that returns a date-time or date, the field that should contain a moment will just contain a string.

If you use DTO Type Style: Class, everything works fine as the init method on the generated class converts it.

Is this something we should handle? Or is this why you should be using Type Style: Class instead?
(I expect the same bug happens with Date Time type: Date, but haven't tested)

[grovesNL]

I think in this case it expects the user to take care of deserializing the string into the correct type. For example, in my generated clients I override the reviver function for JSON.parse. The reviver function detects strings in a format I expect (i.e. ISO 8601) and converts them into a moment.

[RicoSuter]

Yes, if you choose interface the response is provided as-is, no conversion happens. This is why the type class exists in the first place... we should hide the date time type in the ui if interface is chosen...

however you can provide a reviver function to the client which converts the string to moment but the reviver only gets a property name and value and you have to decide whether to convert or not (the schema is not available).

This is why i always prefer class over interface (only downside is that much more code is generated). Also the properties are correctly renamed to match the js style (lower camel case).

[grovesNL]

only downside is that much more code is generated

One upside for the plain objects/generated interfaces approach is that I can easily copy them to change properties (i.e. for use with Redux).

[danzel]

Ah I see! Thanks.
Sounds like we should still allow the user to select this, but perhaps show a warning in the UI when it is set.

[RicoSuter]

Ah yes, maybe this is the reason why I still show this option - if you provide a reviver to satisfy the interface, everything is fine :-)

[RicoSuter]

BTW: There is also an option to generate a clone() method. But I see the point - this is why both options exist.

[RicoSuter]

Can we close this?

[danzel]

Yeah, closing.

[simeyla]

The schema is not available

The schema is not available in a reviver yes - but it is available to @RSuter and his code generation genius :-)

Ah yes, maybe this is the reason why I still show this option - if you provide a reviver to satisfy the interface, everything is fine :-)

That's what I thought, and I got away with it for a while until I returned the following string : "1000050" and it got converted into a moment by my reviver!

This is my simple reviver:

export const jsonReviver = (key: string, value: any) => 
{
    // http://momentjs.com/docs/#/parsing/is-valid/
    if (value) 
    {
        const date = moment(value, moment.ISO_8601);
        if (date.isValid())
        {
            return date;
        }
    }

    return value; 
}

I thought that the ISO_8601 parsing was supposed to be strict, but it turns out there are many valid formats and the string 1000050 actually gets converted to a valid date February 19th 1000 !!! This is because it gets broken down to Year=1000, Month=January, Date=50 which becomes February 19!!!

Anyway beyond that - parsing EVERY SINGLE value that comes back is just insane. I return thousands of rows of data sometimes and knowing that it is going through the reviver for every single property isn't very comforting.

Traditionally I've always preferred classes, but with the power of typescript, switching to interfaces has really turned out to be far more powerful. You don't need to endlessly convert between types, and very importantly you can create 'local' entities that extend generated interfaces. I've been really amazed what I can do. So for me there's definitely no going back to classes.

One horrible - but easy - fix is to make a JsonDateConverter that just prepends MOMENT- to the beginning - so the existing JsonReviver can look for that - however again it's got speed concerns and it's going to break your API for other clients.

I have since updated my reviver to the following which should be A LOT safer:

if (value && (typeof value === 'string') && value.length == 33)

But then I got thinking....

If you're ever up for it I think the following could work quite well:

• Global Reviver is not necessary
• Add a simple RxJS map function to convert only the moment properties
• It uses the spread ... syntax for speed, clarity and miminal generated code. Much faster than reviver!
• We are still using interfaces / no classes - which already specify moment.Moment for properties
• One additional big plus for ... is that we don't need endless assignment code. It does most of the work for us.

• This is added into the pipe after transformResult. Note: This is an incomplete example just to show where the code goes - it has obvious issues (solved later)

  // The simple case is here for a single entity array
  return this.transformResult(url_, response_, (r) => this.processSearch(<any>r))
  
              // here 'r' is of type CustomerSearchResult []
              .pipe(map(r => {
  
                  // r is CustomerSearchResult
                  return r && r.map(x => (
                      { ...x, 
                         lastShipmentDt: moment(x.lastShipmentDt) 
                      }  
                  ));
              }));
  

So this takes an array of CustomerSearchResult and using spread operator ... sets all the existing properties on the object and then creates moment objects for the date fields

Of course it get more complicated than this, with nested objects - but you do have the schema.

I think the following would work quite well - each entity type has its own reviver:

You would have the following:

• Reviver for each generated entity type (all generated by nswag)
• Reviver for arrays
• You add moment converters for date properties
• The spread operator does most of the work :-)
• You only need to generate code when something is an entity type, array, date or date array.
• You don't technically need revivers for types with no properties of type array, date or entity.
• Note that to avoid adding properties that weren't in the original payload we take advantage of the fact that ...(undefined) doesn't add anything to an object.

So the client method works like this (after transformResult) :

 // here 'r' is of type CustomerSearchResult [] so we start with reviveArray
 .pipe(map(r => {
     reviveArray(r, _reviver_CustomerSearchResult)
  }));

 // revive an array by passing each item into the correct reviver for that type
 reviveArray(obj: any[], reviver: (obj: any) => any) {
     return obj && obj.map(x => reviver(x));
 }

 _reviver_CustomerSearchResult(obj: CustomerSearchResult)
 {
     return {...obj, 
                 ... (obj.hasOwnProperty('orders') ? reviveArray(obj.orders, _reviver_Order) : undefined )
                };
 } 

 _reviver_Order(obj: Order)
 {
     return {...obj, 
                 ... ( obj.orderDate ? { orderDate: moment(obj.orderDate) } : undefined ),
                 ... ( obj.shipDate ? { shipDate: moment(obj.shipDate) } : undefined)
                };
 }

Technically wouldn't be a 'breaking change', but it may reveal bugs depending upon how people are currently handling this scenario. I would think it would need to be an additional option to Create moment objects automatically that is selected by default.

I think this could work and would be minimal code generated - certainly a lot less than classes and you could probably use the same basic structure?

This would give 'out of the box' support for interfaces with full moment conversion.

What do you think :-) ?

[RicoSuter]

Interesing idea - can you create a new issue in NJsonSchema with all this text? :-) and reference this issue here... otherwise we will lose track of it because the issue is closed...

[simeyla]

Will do soon :-) Been a busy week.
Would have been much busier without NSwag though!

On Fri, Jan 18, 2019 at 2:13 AM Rico Suter [e-mail] wrote:

Interesing idea - can you create a new issue in NJsonSchema with all this
text? :-) and reference this issue here... otherwise we will lose track of
it because the issue is closed...

—
You are receiving this because you commented.
Reply to this email directly, view it on GitHub
https://github.com/RSuter/NSwag/issues/980#issuecomment-455495790, or mute
the thread
https://github.com/notifications/unsubscribe-auth/ADfKVhMXTOf4HxauraegPVEz8VRbH15Pks5vEZ5cgaJpZM4PuOTA
.

[admir86]

any updates on this? would be the perfect solution! :)

[jannesiera]

@RicoSuter

We ran into a similar issue here. We used to generate to Classes and had little problems. However, the DTO constructors are currently generated with optional arguments (data?). This caused us to run into some bugs in the development cycle when we passed an undefined value. These could have been prevented at compile time if the argument wasn't optional (which it should never be imo).

So we changed to interfaces.

As @simeyla says it is frequently much more developer-friendly to work with interfaces. For this reason alone we would prefer to stick with interfaces.

Another issue is that, in some projects, we mix the generated typescript client with a signalR client (generated by SigSpec). We can mix the use of both perfectly when using interfaces. However, this isn't the case with classes. _Note that classes also inherit from interfaces, and this might be enough to get them to work together. I'd have to review this one._

What we didn't realize is that we now broke a bunch of our DateTime related code, since it isn't well documented that momentjs parsing doesn't happen with interfaces.

So currently we have the choice between:

1. Classes, but poor typesafety and unable to mix with SigSpec
2. Interface, but manual mapping of DateTimes

This is a big problem for us. I would love to see something like what @simeyla suggested where the parsing functions are not baked-in with the classes, but are extracted so they can be used with the interfaces. This way we could leverage code-generation to avoid manual mapping of dates but still retain all other advantages.

I'd be happy to help you make this happen and contribute.

[RicoSuter]

since it isn't well documented that you momentjs parsing doesn't happen with interfaces

Yes that is the big downside of interfaces: We cannot correctly deserialize date/times and rename properties (e.g. "foo-bar" to "fooBar"). The interfaces just represent the plain JSON and not the ideal/described type with dates and nice properties, etc.

The solution would be to move to a completely reviver based solution but this is a huge effort...

[jannesiera]

What about something like this?

Old:

export class LogItemDto implements ILogItemDto {
    deviceId?: string | undefined;
    timestampUtc?: moment.Moment | undefined;
    description?: string | undefined;
    eventPayload?: any | undefined;
    id?: string | undefined;

    constructor(data?: ILogItemDto) {
        if (data) {
            for (var property in data) {
                if (data.hasOwnProperty(property))
                    (<any>this)[property] = (<any>data)[property];
            }
        }
    }

    init(data?: any) {
        if (data) {
            this.deviceId = data["deviceId"];
            this.timestampUtc = data["timestampUtc"] ? moment(data["timestampUtc"].toString()) : <any>undefined;
            this.description = data["description"];
            this.eventPayload = data["eventPayload"];
            this.id = data["id"];
        }
    }

    static fromJS(data: any): LogItemDto {
        data = typeof data === 'object' ? data : {};
        let result = new LogItemDto();
        result.init(data);
        return result;
    }

    toJSON(data?: any) {
        data = typeof data === 'object' ? data : {};
        data["deviceId"] = this.deviceId;
        data["timestampUtc"] = this.timestampUtc ? this.timestampUtc.toISOString() : <any>undefined;
        data["description"] = this.description;
        data["eventPayload"] = this.eventPayload;
        data["id"] = this.id;
        return data; 
    }
}

New:

export class LogItemDtoUtil {
    static init(data?: any) {
        var result = {} as any;
        if (data) {
            result.deviceId = data["deviceId"];
            result.timestampUtc = data["timestampUtc"] ? moment(data["timestampUtc"].toString()) : <any>undefined;
            result.description = data["description"];
            result.eventPayload = data["eventPayload"];
            result.id = data["id"];
        }
        return result as ILogItemDto;
    }

    static fromJS(data: any): ILogItemDto {
        data = typeof data === 'object' ? data : {};
        let result = LogItemDtoUtil.init(data);
        return result;
    }

    static toJSON(dto: ILogItemDto, data?: any) {
        data = typeof data === 'object' ? data : {};
        data["deviceId"] = dto.deviceId;
        data["timestampUtc"] = dto.timestampUtc ? dto.timestampUtc.toISOString() : <any>undefined;
        data["description"] = dto.description;
        data["eventPayload"] = dto.eventPayload;
        data["id"] = dto.id;
        return data;
    }
}

What I did:

1. Take the old class, and rename it to MyClassUtil.
2. Remove constructor and make all methods static.
3. Make those methods return the interface (ISomething).
4. Fix self-references by intializing an object in init and passing an additional parameter to toJSON.

This way you can simply:

1. Replace all calls MyClass.fromJS(...) by MyClassUtil.fromJS(...).
2. Replace all references to classes by interfaces. Or rather keep the current interface generation but add the Utils.

NOTE: ToJson doesn't seem to be used? Seems like JSON.stringify is called before the actual request. Init is only called as part of fromJS() and could possibly be inlined.

This way it seems straightforward to refactor and keep the same behavior, but also support parsing for interfaces. Or is there something obvious (or less obvious) I'm missing here?

[jannesiera]

@RicoSuter should I open a new issue for this?

[RicoSuter]

Looks like that would be a third option besides interface and class... please create a new issue in http://njsonschema.org