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

DefaultGenerator: ignore only form param schemas #74

Merged
merged 7 commits into from
May 24, 2018

Conversation

jmini
Copy link
Member

@jmini jmini commented May 16, 2018

Fix for #50.

This looks for all used schema to compute the unused one.

@wing328
Copy link
Member

wing328 commented May 17, 2018

I tested this one with

git checkout -b jmini-issue50 master
git pull https://github.com/jmini/openapi-generator.git issue50

and ran ./bin/ruby-petstore.sh but got the following:

--- a/samples/client/petstore/ruby/README.md
+++ b/samples/client/petstore/ruby/README.md
@@ -109,38 +109,11 @@ Class | Method | HTTP request | Description
 
 ## Documentation for Models
 
- - [Petstore::AdditionalPropertiesClass](docs/AdditionalPropertiesClass.md)
- - [Petstore::Animal](docs/Animal.md)
- - [Petstore::AnimalFarm](docs/AnimalFarm.md)
  - [Petstore::ApiResponse](docs/ApiResponse.md)
- - [Petstore::ArrayOfArrayOfNumberOnly](docs/ArrayOfArrayOfNumberOnly.md)
- - [Petstore::ArrayOfNumberOnly](docs/ArrayOfNumberOnly.md)
- - [Petstore::ArrayTest](docs/ArrayTest.md)
- - [Petstore::Capitalization](docs/Capitalization.md)
- - [Petstore::Cat](docs/Cat.md)
- - [Petstore::Category](docs/Category.md)
- - [Petstore::ClassModel](docs/ClassModel.md)
  - [Petstore::Client](docs/Client.md)
- - [Petstore::Dog](docs/Dog.md)
- - [Petstore::EnumArrays](docs/EnumArrays.md)
- - [Petstore::EnumClass](docs/EnumClass.md)
- - [Petstore::EnumTest](docs/EnumTest.md)
- - [Petstore::FormatTest](docs/FormatTest.md)
- - [Petstore::HasOnlyReadOnly](docs/HasOnlyReadOnly.md)
- - [Petstore::List](docs/List.md)
- - [Petstore::MapTest](docs/MapTest.md)
- - [Petstore::MixedPropertiesAndAdditionalPropertiesClass](docs/MixedPropertiesAndAdditionalPropertiesClass.md)
- - [Petstore::Model200Response](docs/Model200Response.md)
- - [Petstore::ModelReturn](docs/ModelReturn.md)
- - [Petstore::Name](docs/Name.md)
- - [Petstore::NumberOnly](docs/NumberOnly.md)
  - [Petstore::Order](docs/Order.md)
  - [Petstore::OuterComposite](docs/OuterComposite.md)
- - [Petstore::OuterEnum](docs/OuterEnum.md)
  - [Petstore::Pet](docs/Pet.md)
- - [Petstore::ReadOnlyFirst](docs/ReadOnlyFirst.md)
- - [Petstore::SpecialModelName](docs/SpecialModelName.md)
- - [Petstore::Tag](docs/Tag.md)
  - [Petstore::User](docs/User.md)

A lot of models are no longer auto-generated.

@jmini
Copy link
Member Author

jmini commented May 17, 2018

Ok...
@wing328 thank you a lot for this test.

I did not check in detail, but I guess that those model definitions are not used in the petstore-with-fake-endpoints-models-for-testing.yaml


I still do not get the precise requirements are for this getUnusedSchemas() method and why It was added in the first place.

The point, if you work with the OAS2 proposed in #50, some Model classes are missing (code is not compiling) so this is a regression compared to Swagger-Codegen 2.x. @etherealjoy has also reported a regression with "aspnetcore" generator.

We need to do something.

@wing328
Copy link
Member

wing328 commented May 17, 2018

Sure let me take another look at that problem tomorrow and see if I can come up with a better solution.

There's a workaround to solve that problem by removing "application/x-www-form-urlencoded"

@jimschubert
Copy link
Member

Something to consider as well, some folks (like me) generate models from models-only specs. Will this break that use case? If so, I think we'll need to make it conditional.

@wing328
Copy link
Member

wing328 commented May 18, 2018

Something to consider as well, some folks (like me) generate models from models-only specs. Will this break that use case? If so, I think we'll need to make it conditional.

Yes, I'm aware of such use case.

@jmini
Copy link
Member Author

jmini commented May 18, 2018

@wing328: let summarize it that way:

== The current implementation does:
In DefaultGenerator.generate() in unusedSchemas you call ModelUtils.getUnusedSchemas(openAPI) to get the list of schemas used in a body-request with MimeType: application/x-www-form-urlencoded or multipart/form-data.

== I think what the correct implementation should do is:
... get the list of schemas used in a body-request with MimeType: application/x-www-form-urlencoded or multipart/form-data that are not used somewhere else (for example in a json mime type as it is the case in my issue #50)

If you confirm this, I can rework the PR based on this requirement.

@wing328
Copy link
Member

wing328 commented May 18, 2018

As discussed you've pointed out a use case that's not covered by my current implementation, please feel free to come up with a better solution.

@jmini
Copy link
Member Author

jmini commented May 22, 2018

Input @wing328 :

in openapi v2 spec, there's form parameter
but in openapi v3 spec, it becomes part of the requestbody
and in our code, we've used the flatten feature in openapi parser v3
which means those form parameters defined in the requestbody will be generated as models
these models are not necessary
developers upgrading their spec to v3 will find a lot of models generated but not defined by them in the component section
that's why i check the MIME type to see if it's for form parameters
and removed those if found


I think that the ModelUtils.getSchemasUsedOnlyInFormParam(OpenAPI) now does what it should (according to what was discussed here).

@jmini
Copy link
Member Author

jmini commented May 23, 2018

@wing328 I have tested it with ./bin/ruby-petstore.sh and for me, it now works as expected. Can you review?

@jmini jmini changed the title Rewrite ModelUtils.getUnusedSchemas(OpenAPI) DefaultGenerator: ignore only form param schemas May 23, 2018
unusedSchemas.remove(getSimpleRef(mediaType.getSchema().get$ref()));
}
}
*/
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@jmini what about removing this commented code block?

}
*/

private static void visitOpenAPI(OpenAPI openAPI, OpenAPISchemaVisitor visitor) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest adding a 1-liner explaining what this function does.

@jmini
Copy link
Member Author

jmini commented May 24, 2018

@wing328 nice catch, I have removed the dead code and add a javadoc on the private method.

Thank you a lot for this review.

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.

3 participants