[Typescript][Node] Property renaming fix when deserializing JSON #4264

TiFu · 2016-11-26T23:15:43Z

First: First time contributor, so feel free to yell at me :-).

PR checklist

Read the contribution guildelines.
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.

Second Point: Is it enough if i recreated the samples for typescript-node (e. g. I don't have to recreate typescript-angular samples)? I only changed one file in swagger-codegen/src/main/resources/typescript-node.

Description of the PR

Reference: #2766
This also fixes #2776

The Typescript-Node-Client did not parse the JSON returned from requests, so that properties, which were renamed (e. g. from mounted_at in the response to mountedAt in the model), still appeared as the non-renamed version (e. g. mounted_at) in the model object.

This PR fixes this issue by adding property maps and parsing the JSON recursively.

NOTE:
I'm not sure if this change breaks backwards compatibility, so I filed the PR against 2.3.0.

I hope that I did not miss any cases, I need to handle. It would be great if someone with more experience with swagger-codegen, than I have, could take a look at the code.

The previous version did not parse the JSON-Response at all, so that properties, which where renamed (e. g.due to modelPropertyNaming config option), were not correctly parsed. E. g.: modelPropertyNaming=camelCase & property 'mounted_at' in json, would be renamed to mountedAt in the model. This was not parsed at all in the typescript-node client api, so that the actual model still had the mounted_at property, but not mountedAt. See swagger-api#2766 for additional details

TiFu · 2016-11-28T11:07:07Z

@wing328 I just read the discussion in PR #4267. I will update my PR to use Map[baseName, type] and Map[baseName, name] maps in the model and create a general deserializer/serializer.
Compared to that my solution is ugly at best.

wing328 · 2016-11-28T11:14:54Z

@TiFu I would suggest we wait till @gregjacobs to update his PR so that we can reuse the same solution in all Typescript API clients (node, angular, angular2, typescript)

TiFu · 2016-12-18T15:51:24Z

@wing328
I have some questions:

How can I fix the issue appveyor is reporting? see [1]
How can I fix the issue travis is reporting? see [2]
- Is there a way to trigger a rebuild?
The PHP Object serializer [0] has a method for sanitizing the filename which seems to be used when the Content-Disposition header is present. Why is that sanitization necessary?

[0] https://github.com/swagger-api/swagger-codegen/blob/master/samples/client/petstore/php/SwaggerClient-php/lib/ObjectSerializer.php

[1]

git clone https://github.com/wing328/swagger-samples
fatal: destination path 'swagger-samples' already exists and is not an empty directory.
Command exited with code 128

[2]

[INFO] ------------------------------------------------------------------------
[INFO] Building Swagger Petstore - Javascript Client 1.0-SNAPSHOT
[INFO] ------------------------------------------------------------------------
[INFO] 
[INFO] --- exec-maven-plugin:1.2.1:exec (npm-install) @ swagger-petstore-javascript ---
npm WARN package.json swagger_petstore@1.0.0 No repository field.
npm WARN deprecated jade@0.26.3: Jade has been renamed to pug, please install the latest version of pug instead of jade
npm WARN deprecated graceful-fs@2.0.3: graceful-fs v3.0.0 and before will fail on node releases >= v7.0. Please update to graceful-fs@^4.0.0 as soon as possible. Use 'npm ls graceful-fs' to find it in the tree.
npm WARN deprecated minimatch@0.2.14: Please update to minimatch 3.0.2 or higher to avoid a RegExp DoS issue
npm ERR! Linux 4.4.0-51-generic
npm ERR! argv "/home/travis/.nvm/versions/node/v4.1.2/bin/node" "/home/travis/.nvm/versions/node/v4.1.2/bin/npm" "install"
npm ERR! node v4.1.2
npm ERR! npm  v2.14.4
npm ERR! code ECONNRESET
npm ERR! errno ECONNRESET
npm ERR! syscall read```

wing328 · 2016-12-18T16:19:17Z

You can ignore it (or rebase on the latest master to fix it).
I've restarted that particular job manually. To trigger a rebuild, please close and reopen the PR (which will trigger all the tests).
sanitizeFilename is for sanitizing filename to remove relative path (e.g. ../../../newfile.txt) so as to prevent potential security issues when storing the file in a relative path.

TiFu · 2016-12-22T20:55:16Z

Some testing from the community would be appreciated :-) Thanks!

wing328 · 2017-01-04T16:18:53Z

@TiFu thanks for the PR. If it's too much work, I wonder if you can add some tests for serialization/deserialization of models similar to what we've done for PHP models: https://github.com/swagger-api/swagger-codegen/blob/master/samples/client/petstore/php/SwaggerClient-php/tests/OrderApiTest.php#L43-L132

TiFu · 2017-01-04T18:14:54Z

I'll look into it next week and update my PR.

Edit: I can't seem to find a time slot to implement this. I'll do it when I've more time again (probably around end of February).

wing328 · 2017-01-23T07:10:37Z

@TiFu please take your time. I would recommend writing tests cases for the Pet object, which contains nested objects Tag, Category: https://github.com/swagger-api/swagger-codegen/blob/master/modules/swagger-codegen/src/test/resources/2_0/petstore.yaml#L653

- primitives were serialized as strings instead of their respective type - added test case for pet

TiFu · 2017-02-03T17:01:58Z

@wing328 Can you confirm that this issue also happens on the 2.3.0 branch? If I recreate the sample for ts-node and then run the test with mvn verify, the same issue happens.

It seems like a change in PetAPI.uploadFile(args) causes this issue (the type of the last argument file changed from any to Buffer). I have commited a fix in 6178cb0 .

If it works in 2.3.0, I will have to dig deeper and check how I introduced this bug.

wing328 · 2017-02-03T17:06:10Z

@wing328 Can you confirm that this issue also happens on the 2.3.0 branch? If I recreate the sample for ts-node and then run the test with mvn verify, the same issue happens.

Let me try with a new branch and update the TS node petstore sample to confirm...

wing328 · 2017-02-03T17:08:29Z

Btw, the tests now look good after you pushed 6178cb0

https://travis-ci.org/swagger-api/swagger-codegen/builds/198072272

wing328 · 2017-02-03T17:38:28Z

Here is the job for updated TS Node Petstore based on 2.3.0: https://travis-ci.org/swagger-api/swagger-codegen/builds/198085651 (still running)

wing328 · 2017-02-03T17:45:40Z

UPDATE: https://travis-ci.org/swagger-api/swagger-codegen/builds/198085651 finished without issue.

TiFu · 2017-02-03T19:53:08Z

The issue you noticed is caused by the typemapping from file => Buffer. This mapping was added to TypeScriptNodeCodegen.java almost a year ago. Somehow the ts node tests still contain the previous any type mapping defined in AbstractTypeScriptCodegen.java.
Another thing I noticed is, that TypeScriptNodeCodegen uses typeMapping.put("file", "Buffer");, whereas AbstratcTypeScriptCodegen uses typeMapping.put("File", "any"); (note the capitalization of file)
Does that make any difference or was that changed recently?

The issue you saw was fixed on master in 55443da on 17 January 2017 (with the same fix I pushed a couple hours ago)
However, the fix was not applied to 2.3.0, which is why this error popped up.

I have no idea why this did not appear earlier. Maybe it was a caching issue on my side so that the tests did not get updated properly or the behaviour of typeMapping was changed recently.

Like you said above the tests are fine now. Do I need to do anything else to get this PR merged?

wing328 · 2017-02-05T14:53:23Z

Like you said above the tests are fine now. Do I need to do anything else to get this PR merged?

No, I believe we all appreciate your effort in this PR and I'll merge shortly so that other developers can enjoy the enhancements you put into the Typescript generators.

wing328 · 2017-02-05T14:56:00Z

Another thing I noticed is, that TypeScriptNodeCodegen uses typeMapping.put("file", "Buffer");, whereas AbstratcTypeScriptCodegen uses typeMapping.put("File", "any"); (note the capitalization of file)

Ideally we want to make it consistent across all TS generators. I'll create another issue to track this.

cc @Vrolijkx

* Implemented fix for missing json to property mapping in typescript-node The previous version did not parse the JSON-Response at all, so that properties, which where renamed (e. g.due to modelPropertyNaming config option), were not correctly parsed. E. g.: modelPropertyNaming=camelCase & property 'mounted_at' in json, would be renamed to mountedAt in the model. This was not parsed at all in the typescript-node client api, so that the actual model still had the mounted_at property, but not mountedAt. See swagger-api#2766 for additional details * Updated samples for typescript-node * Reverted initial changes to api.mustache * Draft for object serializer for typescript-node * Fixed missing variable error in ObjectSerializer in typescript-node * Fix for body return type * Fixed attributeTypeMaps when polymorphism is used * Added ObjectSerializer support for polymorphism * Code formatting in typescript-node api.mustache * Fixed primitive type bug & added tests for ts-node - primitives were serialized as strings instead of their respective type - added test case for pet * Code Formatting in ts-node client test See samples/client/petstore/typescript-node/npm/client.ts * Replaced tabs with 4 spaces and improved code formatting * Recreated security test for typescript-node * Read sample.png with fs.readFileSync instead of a stream

cenkentimist · 2017-04-06T14:52:19Z

What's the rational for transforming snake case to camel case anyway? What about just leaving things as they are? given the number of bugs caused by _ conversion this might make sense? I myself prefer camel case to snake case but the current state of things basically guarantees that '_' causes bugs. It also adds unnecessary decode overhead and complexity.

@TiFu has anyone tested what happens if there's a model with both
hiThere and hi_there

wing328 · 2017-04-06T16:01:33Z

@cenkentimist

What's the rational for transforming snake case to camel case anyway?

We want the property naming to conform to the style guide: https://github.com/Microsoft/TypeScript/wiki/Coding-guidelines#names

What about just leaving things as they are?

There's an option you can do so:

	modelPropertyNaming
	    Naming convention for the property: 'camelCase', 'PascalCase', 'snake_case' and 'original', which keeps the original name (Default: camelCase)`

Would that meet your requirement?

cenkentimist · 2017-04-06T16:06:04Z

@wing328 That's awesome.
edit: Seems in general language specific flags are mostly undocumented?

wing328 · 2017-04-06T16:12:46Z

@cenkentimist another reason is that other API client generators (e.g. PHP, Python) also has a serializer/deserializer for JSON and we want to make the TS generators more consistent with other generators.

wing328 · 2017-04-06T16:22:04Z

Seems in general language specific flags are mostly undocumented?

It's documented in config-help: java -jar modules/swagger-codegen-cli/target/swagger-codegen-cli.jar config-help -l typescript-angular2

cenkentimist · 2017-04-06T16:25:42Z

@wing328 Oh. I feel bad for not noticing that myself. Thanks for the clue-hammer.

TiFu · 2017-04-06T17:18:50Z

@cenkentimist This would create a conflict in the model (duplicate attribute name) and the code will not compile.

TiFu added 2 commits November 26, 2016 23:48

Updated samples for typescript-node

b347727

wing328 added the Client: TypeScript label Nov 28, 2016

wing328 modified the milestones: v2.2.2, v2.3.0 Nov 28, 2016

wing328 added the Enhancement: Feature label Nov 28, 2016

Reverted initial changes to api.mustache

494d1a7

TiFu mentioned this pull request Dec 16, 2016

Initial implementation to deserialize date-time strings into Date objects for TypeScript-Angular2 #4267

Closed

3 tasks

TiFu added 3 commits December 17, 2016 20:26

Draft for object serializer for typescript-node

97c28b2

Fixed missing variable error in ObjectSerializer in typescript-node

ebcbd9f

Fix for body return type

6a64e46

TiFu added 3 commits December 21, 2016 14:32

Fixed attributeTypeMaps when polymorphism is used

b460ef5

Added ObjectSerializer support for polymorphism

4ebf97f

Code formatting in typescript-node api.mustache

099dffa

TiFu closed this Dec 22, 2016

TiFu reopened this Dec 22, 2016

wing328 mentioned this pull request Jan 15, 2017

[typescript-angular2] Generated model attributes always start with lowercase char #4558

Closed

TiFu and others added 2 commits January 25, 2017 10:57

Fixed primitive type bug & added tests for ts-node

8a12f76

- primitives were serialized as strings instead of their respective type - added test case for pet

Merge branch '2.3.0' into typescriptPropertyNamingFix

6fbb2cc

TiFu closed this Jan 25, 2017

TiFu reopened this Jan 25, 2017

Read sample.png with fs.readFileSync instead of a stream

6178cb0

wing328 merged commit 8e036f2 into swagger-api:2.3.0 Feb 5, 2017

wing328 mentioned this pull request Feb 5, 2017

file mapping in Typescript generators #4715

Open

soycabanillas mentioned this pull request Feb 10, 2017

[typescript-angular2] Template literals on url path does not match variable names #4745

Closed

This was referenced Feb 13, 2017

[Angular2] attributes are snake_cased #4774

Open

[typescript-fetch] response parser doesn't match the generated model #4785

Open

[typescript-angular2] Handle date type correctly #3105

Closed

wing328 mentioned this pull request Feb 22, 2017

[Java] Elision of underscore characters in property names leads to invalid Java #4805

Open

wing328 added the Breaking change (without fallback) label Mar 23, 2017

wing328 changed the title ~~Typescript Property Renaming Fix~~ [Typescript] Property renaming fix when deserializing JSON Mar 23, 2017

wing328 changed the title ~~[Typescript] Property renaming fix when deserializing JSON~~ [Typescript][Node] Property renaming fix when deserializing JSON Apr 29, 2017

This was referenced Jul 11, 2017

Deserialization failed due to change of property name #4857

Open

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

Open

defmonk0 mentioned this pull request Oct 19, 2017

[Typescript-Angular] Service generation incorrectly attempting to use parameter. #6762

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Typescript][Node] Property renaming fix when deserializing JSON #4264

[Typescript][Node] Property renaming fix when deserializing JSON #4264

TiFu commented Nov 26, 2016 •

edited

Loading

TiFu commented Nov 28, 2016

wing328 commented Nov 28, 2016

TiFu commented Dec 18, 2016

wing328 commented Dec 18, 2016 •

edited

Loading

TiFu commented Dec 22, 2016 •

edited

Loading

wing328 commented Jan 4, 2017

TiFu commented Jan 4, 2017 •

edited

Loading

wing328 commented Jan 23, 2017

TiFu commented Feb 3, 2017 •

edited

Loading

wing328 commented Feb 3, 2017

wing328 commented Feb 3, 2017

wing328 commented Feb 3, 2017

wing328 commented Feb 3, 2017

TiFu commented Feb 3, 2017 •

edited

Loading

wing328 commented Feb 5, 2017

wing328 commented Feb 5, 2017

cenkentimist commented Apr 6, 2017

wing328 commented Apr 6, 2017

cenkentimist commented Apr 6, 2017 •

edited

Loading

wing328 commented Apr 6, 2017

wing328 commented Apr 6, 2017

cenkentimist commented Apr 6, 2017

TiFu commented Apr 6, 2017

[Typescript][Node] Property renaming fix when deserializing JSON #4264

[Typescript][Node] Property renaming fix when deserializing JSON #4264

Conversation

TiFu commented Nov 26, 2016 • edited Loading

PR checklist

Description of the PR

TiFu commented Nov 28, 2016

wing328 commented Nov 28, 2016

TiFu commented Dec 18, 2016

wing328 commented Dec 18, 2016 • edited Loading

TiFu commented Dec 22, 2016 • edited Loading

wing328 commented Jan 4, 2017

TiFu commented Jan 4, 2017 • edited Loading

wing328 commented Jan 23, 2017

TiFu commented Feb 3, 2017 • edited Loading

wing328 commented Feb 3, 2017

wing328 commented Feb 3, 2017

wing328 commented Feb 3, 2017

wing328 commented Feb 3, 2017

TiFu commented Feb 3, 2017 • edited Loading

wing328 commented Feb 5, 2017

wing328 commented Feb 5, 2017

cenkentimist commented Apr 6, 2017

wing328 commented Apr 6, 2017

cenkentimist commented Apr 6, 2017 • edited Loading

wing328 commented Apr 6, 2017

wing328 commented Apr 6, 2017

cenkentimist commented Apr 6, 2017

TiFu commented Apr 6, 2017

TiFu commented Nov 26, 2016 •

edited

Loading

wing328 commented Dec 18, 2016 •

edited

Loading

TiFu commented Dec 22, 2016 •

edited

Loading

TiFu commented Jan 4, 2017 •

edited

Loading

TiFu commented Feb 3, 2017 •

edited

Loading

TiFu commented Feb 3, 2017 •

edited

Loading

cenkentimist commented Apr 6, 2017 •

edited

Loading