Fixed property mapping to be inheritance-based #121
Conversation
…pe chain to find the appropriate property mapping rather than just searching by property name. That way properties with the same name on different classes won't conflict. Added new test classes and updated unit/integration tests.
…is correctly handled. (Tests already checked that unmapped duplicate property is skipped.)
| /** | ||
| * Class which has a property with the same name as Cat. Used to test handling | ||
| * of prototype-based property mapping search. Property has the same name, but | ||
| * an incompatible type, and is initialized (so it's present) but purposely left |
There was a problem hiding this comment.
Property has the same name, but an incompatible type
That's I exactly how I experienced the problem: I had two sibling classes that had properties of the same name, but in one case the property was optional and in the other one it was required.
If the properties' types are compatible, the behavior is still wrong (definition is possibly taken from the wrong class), but it doesn't have any effect.
There was a problem hiding this comment.
So I think your test ensures that the logic works correctly now.
src/json2typescript/json-convert.ts
Outdated
| /* Find mapping by iterating up the prototype chain to find a matching mapping, rather than | ||
| * just searching by property name. */ | ||
| let prototype = Object.getPrototypeOf(instance); | ||
| while (prototype != null) { |
There was a problem hiding this comment.
Are you sure that your loop always terminates? Is there no such thing as a reference to self?
Following the docs, it should be fine:
The prototype of the given object. If there are no inherited properties, null is returned.
There was a problem hiding this comment.
As long as the object is constructed normally, it should be fine. You're right though, it's of course possible that someone has manually set the prototype to some self-referential or circular link. I'll modify it to check for self as well.
There was a problem hiding this comment.
Actually, something I've just learned is that you can't in fact create circular prototype references. That is, you can, but it will cause an error: TypeError: Cyclic __proto__ value. So we don't need to check for self - the chain will eventually always terminate.
There was a problem hiding this comment.
That's good to know! Otherwise this would be a quite frustrating user experience ;-)
src/json2typescript/json-convert.ts
Outdated
| /* Find mapping by iterating up the prototype chain to find a matching mapping, rather than | ||
| * just searching by property name. */ | ||
| let prototype = Object.getPrototypeOf(instance); | ||
| while (prototype != null) { |
There was a problem hiding this comment.
Probably you could write while (prototype !== null) so that it doesn't do type conversions.
There was a problem hiding this comment.
To be honest, I did that because I was lazy and wanted to catch undefined as well as null. But it's probably safer to check for both explicitly, I'll fix that as well. Thanks!
|
Have you considered setting up github-ci tests? It is fairly straight-forward. |
|
The github-ci automated testing definitely sounds useful, but I wouldn't want to add that in this pull request. (Also, I had to modify my karma.conf.js to get the tests to run in my IDE, since it won't allow PhantomJS anymore.) If @andreas-aeschlimann is interested, maybe you or I could create a new pull request to add it? |
…tly check for null/undefined separately, and added note on circular prototype chain error. Also fixed formatting in test file.
|
I would be glad to help. I already did the setup in another project and it works well. |
|
Thank you @cmolodo and @tobiasschweizer for digging deeper in this issue. It is correct that the approach to handle class inheritance was not well working in some special cases. The current approach just searches for any property with the same name instead of properly going up the inheritance tree. The approach of @cmolodo to climb up the inheritance tree is elegant and of course the correct way to do it. I will merge this PR now. |
Modified getClassPropertyMappingOptions() to step through the prototpe chain to find the appropriate property mapping rather than just searching by property name. That way properties with the same name on different classes won't conflict.
Also added new test classes and updated unit/integration tests.