Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[Swift3] decoders call parents decoders to fix issue with class inheritance #5369

Merged
merged 1 commit into from
Apr 17, 2017
Merged

[Swift3] decoders call parents decoders to fix issue with class inheritance #5369

merged 1 commit into from
Apr 17, 2017

Conversation

julienfouilhe
Copy link
Contributor

@julienfouilhe julienfouilhe commented Apr 11, 2017

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

I was having problems with classes inheriting from other classes, fields defined in parent classes were nil. I fixed it by calling the parent's decoder in the child's decoder and passing the child's instance.

@wing328
Copy link
Contributor

wing328 commented Apr 12, 2017

@julienfouilhe thanks for the PR.

cc @jaz-ah @Edubits @jgavris @hexelon

@jaz-ah
Copy link
Contributor

jaz-ah commented Apr 12, 2017

@julienfouilhe could you regenerate the sample files and put in here as well? I wanted to see what the output looks like now...

@julienfouilhe
Copy link
Contributor Author

julienfouilhe commented Apr 12, 2017

@jaz-ah, it should be in the commit. Most of it is in Models.swift, and the diff is not displayed on github unless you click because it's too large. :)

Edit: Oh, you mean those other files in the samples. Give me a few secs

@jaz-ah
Copy link
Contributor

jaz-ah commented Apr 12, 2017

@julienfouilhe ah I meant the actual sample output -i.e. the sample/test project checked into the tree...

@julienfouilhe
Copy link
Contributor Author

julienfouilhe commented Apr 12, 2017

@jaz-ah I ran swift3-petstore-all.sh. Tell me if that's not what you needed. I didn't push those at first because I thought it was just reversing the order of some args in the sample files.

@jaz-ah
Copy link
Contributor

jaz-ah commented Apr 12, 2017

@julienfouilhe I must have missed it - I see it now - looks good - thx! +1 @wing328

@julienfouilhe
Copy link
Contributor Author

julienfouilhe commented Apr 12, 2017

@jaz-ah Yes I just ran them and amended my commit. ;)

I don't think there's a test for that though. If you want to see where the bug is, you would need to add an endpoint to the Petstore API /cats returning Cat objects and not Pet. Before this commit, it didn't go through the Pet decoder, and thus didn't fill fields described in Pet

@wing328
Copy link
Contributor

wing328 commented Apr 17, 2017

@julienfouilhe thanks for the PR.

@jaz-ah thanks for reviewing the change.

@wing328 wing328 merged commit b1a39ac into swagger-api:master Apr 17, 2017
@wuf810
Copy link
Contributor

wuf810 commented Apr 18, 2017

I am double checking but the changes in this PR have broken my swagger generated code .

This line in the template

_ = Decoders.decode(clazz: {{parent}}.self, source: source, instance: result)

generates the following code for classes in the Models.swift file :

_ = Decoders.decode(clazz: String.self, source: source, instance: result)

As a result the decode function is looking for a class decoder named "String".

NB The classes this fails on do use AdditionalProperties (which @danwoodel and I are currently fixing)…and I think this might be the issue as the current implementation of AdditionalProperties handling in the Swift3 codegen is completely broken in that it tries to make the class a subclass of a string collection.

This I would guess is what is causing the {{parent}} to mistakenly return it the type as String.

Comments/thoughts anyone?

@dwoodel
Copy link
Contributor

dwoodel commented Apr 18, 2017

I think it might be a good idea to add some models in the PetStore example with some additional properties.

@julienfouilhe
Copy link
Contributor Author

julienfouilhe commented Apr 18, 2017

We should check if decoders["\({{parent}}.self)"] exists too. I should have done that.

Done in #5416

@wing328
Copy link
Contributor

wing328 commented Apr 19, 2017

I think it might be a good idea to add some models in the PetStore example with some additional properties.

We do have that in petstore-with-fake-endpoints-models-for-testing.yaml, which covers a lot more edge cases.

Currently, the Swift Petstore is using petstore.yaml (the basic one) and I'll create a task to track updating Swift Petstore to use petstore-with-fake-endpoints-models-for-testing.yaml

@wing328
Copy link
Contributor

wing328 commented Apr 19, 2017

Created #5419 for tracking

@wing328
Copy link
Contributor

wing328 commented Apr 19, 2017

@dwoodel actually swift3 petstore is already using the fake petstore. Please feel free to submit a PR to add additional test cases to fake petstore so as to cover more edge cases.

@wing328
Copy link
Contributor

wing328 commented Apr 19, 2017

@julienfouilhe when you've time I wonder if you can make the same change against 2.3.0 branch instead as I got merge conflict when syncing the changes from master to 2.3.0. Thanks!

@julienfouilhe
Copy link
Contributor Author

@wing328 Done!

@wing328 wing328 changed the title fix(swift3): decoders call parents decoders too [Swift3] decoders call parents decoders to fix issue with class inheritance Apr 22, 2017
davidgri pushed a commit to davidgri/swagger-codegen that referenced this pull request May 11, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants