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#] Child models in allOf clause not compiling because parent does not implement BaseValidate #1126

Closed
rubms opened this issue Sep 27, 2018 · 2 comments

Comments

@rubms
Copy link
Contributor

rubms commented Sep 27, 2018

Description

The auto-generated C# models that are children in allOf clauses don't compile because they are attempting to invoke a BaseValidate method that should be implemented by the parent object, but is not implemented.

openapi-generator version

v3.3.0 (master snapshot)

OpenAPI declaration file content or url
swagger: '2.0'
info:
  description: "This spec is for testing the wrong generation of childrend models in allOf clauses."
  version: 1.0.0
  title: Child allOf model test case
  license:
    name: Apache-2.0
    url: 'http://www.apache.org/licenses/LICENSE-2.0.html'
schemes:
  - http
paths:
  /test/:
    get:
      tags:
        - Test
      summary: Test
      description: Test
      operationId: getTest
      responses:
        '200':
          description: successful operation
          schema:
            $ref: '#/definitions/Child'

definitions:
  Parent:
    type: object
    properties:
      parentProperty:
        type: string
  Child:
    allOf:
    - $ref: "#/definitions/Parent"
    - type: object
      properties:
        childProperty:
          type: string
Command line used for generation

java -jar openapi-generator-cli.jar generate -g csharp -i .\test.yml -o test

Steps to reproduce

Auto-generate the OpenAPI specification above using the csharp generator. The generated Child model does not compile because it uses a BaseValidate method that should be implemented in Parent, but is not.

Suggest a fix/enhancement

The problem is originated in 2 points.

On the one hand, DefaultCodegen.java fails to correctly mark the hasChildren property of CodegenModel when the model has children:

            // Let parent know about all its children
            for (String name : allModels.keySet()) {
                CodegenModel cm = allModels.get(name);
                CodegenModel parent = allModels.get(cm.getParent());
                // if a discriminator exists on the parent, don't add this child to the inheritance hierarchy
                // TODO Determine what to do if the parent discriminator name == the grandparent discriminator name
                while (parent != null) {
                    if (parent.getChildren() == null) {
                        parent.setChildren(new ArrayList<CodegenModel>());
                    }
                    parent.getChildren().add(cm);
                    parent.hasChildren = true; // <- IT SHOULD MARK THAT THE PARENT HAS CHILDEN HERE
                    if (parent.getDiscriminator() == null) {
                        parent = allModels.get(parent.getParent());
                    } else {
                        parent = null;
                    }
                }
            }

In the other hand, the template for the generation of C# models, modelGeneric.mustache, uses the discriminator property in order to decide whether to implement a base validation method or not.

{{#discriminator}}
        /// <summary>
        /// To validate all properties of the instance
        /// </summary>
        /// <param name="validationContext">Validation context</param>
        /// <returns>Validation Result</returns>
        IEnumerable<System.ComponentModel.DataAnnotations.ValidationResult> IValidatableObject.Validate(ValidationContext validationContext)
        {
            return this.BaseValidate(validationContext);
        }

        /// <summary>
        /// To validate all properties of the instance
        /// </summary>
        /// <param name="validationContext">Validation context</param>
        /// <returns>Validation Result</returns>
        protected IEnumerable<System.ComponentModel.DataAnnotations.ValidationResult> BaseValidate(ValidationContext validationContext)
        {
{{/discriminator}}
{{^discriminator}}
        /// <summary>
        /// To validate all properties of the instance
        /// </summary>
        /// <param name="validationContext">Validation context</param>
        /// <returns>Validation Result</returns>
        IEnumerable<System.ComponentModel.DataAnnotations.ValidationResult> IValidatableObject.Validate(ValidationContext validationContext)
        {
{{/discriminator}}

As discriminator only makes sense for anyOf, this means that the auto-generated code in case of allOf will always be wrong. hasChildren should be used, instead of discriminator.

In addition, given the possibility of a child object also being the parent of other objects, it is necessary to prevent an infinite recursive call in modelGeneric.mustache

            {{^isArrayModel}}
            {{^isMapModel}}
            foreach(var x in BaseValidate(validationContext)) yield return x;
            {{/isMapModel}}
            {{/isArrayModel}}
            {{/parent}}

In case of being a parent object and having a parent, the BaseValidate object would recursively and infinitely be calling itself. Instead it should call the BaseValidate method of the parent:

            {{^isArrayModel}}
            {{^isMapModel}}
            foreach(var x in base.BaseValidate(validationContext)) yield return x;
            {{/isMapModel}}
            {{/isArrayModel}}
            {{/parent}}
rubms pushed a commit to rubms/openapi-generator that referenced this issue Sep 27, 2018
…n 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.
@rubms
Copy link
Contributor Author

rubms commented Sep 27, 2018

Filed the #1127 pull request that fixes this issue.

wing328 pushed a commit that referenced this issue Oct 15, 2018
…in allOf clauses (#1127)

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

* Fixed issue #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 #1126

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

This partially reverts commit 2168df0.
@rubms
Copy link
Contributor Author

rubms commented Oct 16, 2018

Fixed with the merge of #1127 to master. Thanks @jimschubert and @wing328 !

@rubms rubms closed this as completed Oct 16, 2018
A-Joshi pushed a commit to ihsmarkitoss/openapi-generator that referenced this issue 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
Labels
None yet
Projects
None yet
Development

No branches or pull requests

1 participant