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

Fixed unexpected model generation bug. #204

Merged
merged 2 commits into from
Jan 23, 2019

Conversation

jiachen1120
Copy link
Contributor

@jiachen1120 jiachen1120 commented Jan 21, 2019

@NicholasAzar @stevehu
This PR is related to API-46 issue. Look forward to your review.

  • Added schema type check
  • Added properties number check

RFC for this issue: https://github.com/networknt/light-rfcs/blob/master/light-codegen/0003-model-generation-bug.md

- Added schema type check
- Added properties number check
@DSchrupert DSchrupert self-requested a review January 21, 2019 15:11
@ddobrin
Copy link
Contributor

ddobrin commented Jan 21, 2019

Is this change backwards compatible with the classes generated by the light-codegen at this time?

@ddobrin ddobrin merged commit 80ef35f into networknt:develop Jan 23, 2019
@243826
Copy link
Contributor

243826 commented Feb 23, 2019

It looks like this fix introduces another bug where the name where the model generation for the types is suppressed but the types are not replaced with their corresponding primitive boxed types or array types. It results in non compiling code.

e.g. I had schemas

"UserId": {
  "type": "string",
   "minLength" : 8,
   "maxLength: 32
},
"Pair": {
  "type": "object",
  "required": [
      "primary",
      "secondary"
  ],
  "properties": {
     "primary" : {
        "$ref" : "#/components/schemas/UserId"
      },
     "secondary" : {
        "$ref" : "#/components/schemas/UserId"
      }
  }
}

I would get the following model for Pair

class Pair {
  UserId primary;
  UserId secondary;
  
  // irrelevant details deleted
}

but will not get the model for UserId. Obviously the code would not compile.

@jiachen1120
Copy link
Contributor Author

jiachen1120 commented Feb 23, 2019

@243826 Thank you for pointing this out. All property's types start with $ref at this stage will be set to its own name. This is incorrect and should be replaced with the type it references.

@stevehu
Copy link
Contributor

stevehu commented Feb 23, 2019

@jiachen1120 @243826 There is a utility @ddobrin built to bundle multiple specification files and resolve the references in the components. I am wondering if we should try it out see what is the final spec before feeding it to the light-codegen.

@243826
Copy link
Contributor

243826 commented Feb 24, 2019 via email

@stevehu
Copy link
Contributor

stevehu commented Feb 24, 2019

https://github.com/networknt/openapi-bundler is the repository and document is in the readme. In addition, this document might help. https://doc.networknt.com/development/best-practices/openapi3/#guidelines

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

Successfully merging this pull request may close these issues.

5 participants