swagger-codegen icon indicating copy to clipboard operation
swagger-codegen copied to clipboard

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

Open ard61 opened this issue 8 years ago • 9 comments

PR checklist

  • [x] Read the contribution guidelines.
  • [x] 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)
  • [x] 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.

ard61 avatar Jul 11 '17 14:07 ard61

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 avatar Jul 11 '17 16:07 wing328

cc @Vrolijkx @taxpon @sebastianhaas @kenisteward @TiFu

wing328 avatar Jul 11 '17 16:07 wing328

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

ard61 avatar Jul 11 '17 16:07 ard61

I will take at look at this on weekend.

sebastianhaas avatar Jul 12 '17 21:07 sebastianhaas

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?

ard61 avatar Jul 17 '17 19:07 ard61

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.

ard61 avatar Jul 17 '17 19:07 ard61

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.

TiFu avatar Jul 17 '17 22:07 TiFu

@tifu I second this

On Mon, Jul 17, 2017, 6:14 PM Tino Fuhrmann [email protected] 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 https://github.com/swagger-api/swagger-codegen/pull/6024#issuecomment-315900658, or mute the thread https://github.com/notifications/unsubscribe-auth/AMPLtftH9wFtE2Ds8dQtfqUN6uEe0PPyks5sO9ysgaJpZM4OUW9E .

kenisteward avatar Jul 17 '17 22:07 kenisteward

Is this PR still being developed?

iamsamwood avatar Sep 26 '17 20:09 iamsamwood