Fix #4857 typescript-angular2 client deserialization failed due to change of property name #6024

ard61 · 2017-07-11T14:52:13Z

PR checklist

Read the contribution guidelines.
Ran the shell/batch script under ./bin/ to update Petstore sample so that CIs can verify the change. (For instance, only need to run ./bin/{LANG}-petstore.sh and ./bin/security/{LANG}-petstore.sh if updating the {LANG} (e.g. php, ruby, python, etc) code generator or {LANG} client's mustache templates)
Filed the PR against the correct branch: master for non-breaking changes and 2.3.0 branch for breaking (non-backward compatible) changes.

Description of the PR

Fixes issue #4857 Deserialization failed due to change of property name for the typescript-angular2 client, re-using the solution in PR #4264:

adds a property name map to each model, turning them from an interface into a class
adds global objects tracking created classes and enums
adds a class, ObjectSerializer, responsible for translating property names (and serializing/deserializing dates), and makes the generated api use this class at serialization/deserialization.

Breaks backward compatibility I think, because we change the models from interfaces to classes, which will cause issues for users reusing the models in their code and assigning object literals (because of the added constructor / properties).
If that is undesirable, I'm happy to refactor the property name maps into a separate entity, leaving the models as interfaces.

…s.mustache

wing328 · 2017-07-11T16:01:45Z

Thanks for the PR but your commit (as shown in the Commits tab) is not linked to your Github account, which means this PR won't count as your contribution in https://github.com/swagger-api/swagger-codegen/graphs/contributors.

Let me know if you need help fixing it.

Ref: https://github.com/swagger-api/swagger-codegen/wiki/FAQ#how-can-i-update-commits-that-are-not-linked-to-my-github-account

wing328 · 2017-07-11T16:02:36Z

cc @Vrolijkx @taxpon @sebastianhaas @kenisteward @TiFu

ard61 · 2017-07-11T16:41:48Z

Thanks for pointing that out! Somehow I hadn't added the e-mail address I use for commits to my GitHub account...

grEvenX · 2017-07-12T20:36:03Z

samples/client/petstore/typescript-angular2/default/api/pet.service.ts

@@ -117,7 +118,7 @@ export class PetService {
                if (response.status === 204) {
                    return undefined;
                } else {
-                    return response.json() || {};
+                    return ObjectSerializer.deserialize(response.json(), "Array&lt;Pet&gt;") || {};


Shouldn't this be "Array<Pet>" instead of "Array<Pet>"?

This seems to be related to {{returnType}} in line 99 in api.service.mustache.This is also an issue for the ts-node code generator.
Is the returnType encoded by the program generating the input for mustache?

PLease use {{{returnType}}} instead to avoid HTML encoding performed by mustache.

I'll submit a PR to fix the issue in ts-node.

[Edit] See #6043

Thanks! Fixed now.

sebastianhaas · 2017-07-12T21:27:18Z

I will take at look at this on weekend.

TiFu · 2017-07-13T09:50:12Z

modules/swagger-codegen/src/main/resources/typescript-angular2/api.service.mustache

@@ -254,7 +255,7 @@ export class {{classname}} {
            method: {{httpMethod}},
            headers: headers,
 {{#bodyParam}}
-            body: {{paramName}} == null ? '' : JSON.stringify({{paramName}}), // https://github.com/angular/angular/issues/10612
+            body: {{paramName}} == null ? '' : JSON.stringify(ObjectSerializer.serialize({{paramName}}, "{{dataType}}")), // https://github.com/angular/angular/issues/10612


This causes the same issue {{returnType}} caused. => {{{dataType}}} to prevent html encoding

ard61 · 2017-07-17T19:09:44Z

modules/swagger-codegen/src/main/resources/typescript-angular2/types.mustache

+            if (!typeMap[type]) { // dont know the type
+                return data;
+            }
+            let instance = new typeMap[type]();


types.mustache:133
ObjectSerializer.deserialize (and hence the API methods) returns an instance of the modal class.
Before this fix, the API methods returned a plain object.
They are type-compatible, but some testing frameworks e.g. Jasmine, do more extended checks for equality:

class Foo { prop: string } describe('Jasmine type checking', () => { it('fails', () => { let foo = new Foo(); foo.prop = 'Hello'; expect(foo).toBeEqual({'Hello'}); // Error: 'Expected foo to be a type of Object, but is Foo(prop: "Hello")' }); });

Which means that some users (me included) would be having to rewrite unit-tests.

ard61 · 2017-07-17T19:18:21Z

I'm also having second thoughts about adding the property name maps directly in the models, because (if I understand this correctly) they are exposed for the users to reuse in their own code.

Since the property name maps are there just to enable the ObjectSerializer to function correctly, it seems to me that it would make sense to separate them from the models proper.

Any thoughts?

…he model interface and reverted classes back to interfaces.

ard61 · 2017-07-17T19:36:10Z

Here are the changes I would do. I'll revert the commits if you think we should leave the property name maps as they currently are, in the model class.

TiFu · 2017-07-17T22:13:56Z

IMO the whole serializer should be hidden from the user. It is an implementation detail of the API module and the user should not rely on this functionality. This enables us to change it without breaking the code of users. The user should only interact with the model objects and the api objects.

kenisteward · 2017-07-17T22:17:58Z

@TiFu I second this

…

On Mon, Jul 17, 2017, 6:14 PM Tino Fuhrmann ***@***.***> wrote: IMO the whole serializer should be hidden from the user. It is an implementation detail of the API module and the user should not rely on this functionality. This enables us to change it without breaking the code of users. The user should only interact with the model objects and the api objects. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#6024 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AMPLtftH9wFtE2Ds8dQtfqUN6uEe0PPyks5sO9ysgaJpZM4OUW9E> .

iamsamwood · 2017-09-26T20:19:02Z

Is this PR still being developed?

ard61 added 6 commits July 11, 2017 14:59

Fix 4857: property name maps in angular2-typescript.

c74f649

Deserialize data with JSON before calling ObjectSerializer.deserialize()

88271da

Bugfix, with JSON-stringifying request body

e9637d3

Divested the ObjectSerializer class from api.service.mustache to type…

a13cd31

…s.mustache

Whitespace fix.

62791a7

Regenerated Petstore sample files - passes integration tests.

a7e4d31

ard61 changed the title ~~Fix issue4857~~ Fix #4857 typescript-angular2 client deserialization failed due to change of property name Jul 11, 2017

wing328 added this to the v2.3.0 milestone Jul 11, 2017

wing328 added Client: TypeScript Enhancement: General labels Jul 11, 2017

grEvenX reviewed Jul 12, 2017

View reviewed changes

wing328 mentioned this pull request Jul 13, 2017

[Angular2] attributes are snake_cased #4774

Open

TiFu reviewed Jul 13, 2017

View reviewed changes

TiFu mentioned this pull request Jul 13, 2017

[Typescript-node] Fixed html escape bug in returnType and dataType #6043

Merged

3 tasks

ard61 added 2 commits July 13, 2017 14:56

Fix HTML escaping of special characters in types.

30cc460

Regenerated test samples, passes integration test.

722e632

wing328 added Admin: 2.2.3 backlog and removed Admin: 2.2.3 backlog labels Jul 16, 2017

ard61 commented Jul 17, 2017

View reviewed changes

ard61 added 2 commits July 17, 2017 20:19

Divested property name maps from model classes; instead put them in t…

a24e34c

…he model interface and reverted classes back to interfaces.

Regenerated test samples, passes integration tests.

429d9a1

wing328 changed the base branch from 2.3.0 to master July 27, 2017 11:14

wing328 modified the milestones: v2.3.0, v2.4.0 Dec 18, 2017

wing328 removed the Admin: 2.2.3 backlog label Dec 18, 2017

frantuma added the Language: typescript label Jun 27, 2018

frantuma force-pushed the master branch from 4ba5164 to 949f0e6 Compare May 20, 2025 09:13

Fix #4857 typescript-angular2 client deserialization failed due to change of property name #6024

Are you sure you want to change the base?

Fix #4857 typescript-angular2 client deserialization failed due to change of property name #6024

Conversation

ard61 commented Jul 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Description of the PR

Uh oh!

wing328 commented Jul 11, 2017

Uh oh!

wing328 commented Jul 11, 2017

Uh oh!

ard61 commented Jul 11, 2017

Uh oh!

grEvenX Jul 12, 2017

Choose a reason for hiding this comment

Uh oh!

TiFu Jul 13, 2017

Choose a reason for hiding this comment

Uh oh!

wing328 Jul 13, 2017

Choose a reason for hiding this comment

Uh oh!

TiFu Jul 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ard61 Jul 13, 2017

Choose a reason for hiding this comment

Uh oh!

sebastianhaas commented Jul 12, 2017

Uh oh!

TiFu Jul 13, 2017

Choose a reason for hiding this comment

Uh oh!

ard61 Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ard61 commented Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ard61 commented Jul 17, 2017

Uh oh!

TiFu commented Jul 17, 2017

Uh oh!

kenisteward commented Jul 17, 2017 via email

Uh oh!

iamsamwood commented Sep 26, 2017

Uh oh!

Uh oh!

ard61 commented Jul 11, 2017 •

edited

Loading

TiFu Jul 13, 2017 •

edited

Loading

ard61 Jul 17, 2017 •

edited

Loading

ard61 commented Jul 17, 2017 •

edited

Loading