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

[C#] Fixed invalid generation of C# children models in allOf clauses #1127

Merged
merged 6 commits into from
Oct 15, 2018

Conversation

rubms
Copy link
Contributor

@rubms rubms commented Sep 27, 2018

PR checklist

  • Read the contribution guidelines.
  • Ran the shell 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). Windows batch files can be found in .\bin\windows\.
  • Filed the PR against the correct branch: master, 3.3.x, 4.0.x. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

Description of the PR

Fixed the #1126 issue, that causes the auto-generated C# children models not to compile because they attempt to invoke a BaseValidate method in the parent model that is not implemented. In order to fix the issue:

  • DefaultCodegen.java now sets the hasChildren of CodegenModel to true in models that have children.
  • The modelGeneric.mustache now uses hasChildren, instead of discriminator, in order to decide whether to implement a BaseValidate method (for children to run the parent model validations). In addition, now the BaseValidate method of the parent is explicitly invoked (in order to prevent infinite recursion in models that are both, children and parents.

c.c. Core (as DefaultCodegen.java has been modified).
@wing328 (2015/07)
@jimschubert (2016/05)
@cbornet (2016/05)
@jaz-ah (2016/05)
@ackintosh (2018/02)
@JFCote (2018/03)
@jmini (2018/04)

c.c. C# @mandrean (2017/08) @jimschubert (2017/09)

…up-to-date, that are failing in CI.

This partially reverts commit 2168df0.
@jimschubert
Copy link
Member

@rubms Thanks for the contribution. I've tested it locally with the spec in the linked issue and verified the compilation errors before the fix are now resolved with this code change.

@wing328 before merging, I wanted to get your take on the change. Moving from discriminator to hasChildren is potentially a breaking change because it would require anyone with customized templates to update their customizations from discriminator to hasChildren. However, I would assume that anyone affected by this bug "in the wild" would have some local changes to accommodate, so I'm on the fence of whether this is a change to the template contract. What's your take on this being considered more a bug fix than a breaking change? Also, do you think this would potentially need to be fixed across other generators using discriminator in the same way? With this change, I feel like the two seemingly identical options will have diverged.

@wing328
Copy link
Member

wing328 commented Oct 15, 2018

@jimschubert as discussed, it's a bug fix but we don't classify it as a breaking change.

@wing328 wing328 merged commit ded765b into OpenAPITools:master Oct 15, 2018
@wing328 wing328 changed the title [C#] Fixed issue #1126 with invalid generation of C# children models in allOf clauses [C#] Fixed invalid generation of C# children models in allOf clauses Oct 16, 2018
@wing328
Copy link
Member

wing328 commented Oct 16, 2018

@rubms thanks again for the fix, which has been included in the v3.3.1 release: https://twitter.com/oas_generator/status/1052020299821080577

@rubms rubms deleted the Fix_issue_#1126 branch October 16, 2018 07:39
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this pull request Feb 27, 2019
…dren models in allOf clauses (OpenAPITools#1127)

* Run ./bin/utils/ensure-up-to-date to re-generate samples run in the CI.

* Fixed issue OpenAPITools#1126. DefaultCodegen now sets the hasChildren property of CodegenModel when children models are added to the model. Changed the modelGeneric.mustache template to decide whether to include a base validation model (for children to inherit) based on the hasChildren property, and not the discriminator property.

* Run the ./bin/utils/ensure-up-to-date script after fixing the issue OpenAPITools#1126

* Reverted modification in go samples, performed by ./bin/utils/ensure-up-to-date, that are failing in CI.

This partially reverts commit 2168df0.
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.

None yet

3 participants