Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly
Sign up
Appearance settings

Feature/success code responses #7674

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

Open
ignaciomolina wants to merge 7 commits into swagger-api:master
base: master
Choose a base branch
Loading
from ignaciomolina:feature/successCodeResponses

Conversation

@ignaciomolina
Copy link
Contributor

@ignaciomolina ignaciomolina commented Feb 16, 2018 •
edited
Loading

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: 3.0.0 branch for changes related to OpenAPI spec 3.0. Default: master.
  • Copied the technical committee to review the pull request if your PR is targeting a particular programming language.

@bbdouglas @JFCote @sreeshas @jfiala @lukoyanov @cbornet @jeff9finger

Description of the PR

  • Add attribute to CodegenOperations to obtain the response code
  • Add response code to Java Play Framework Controller template

Copy link
Contributor

#7583 @JFCote

Copy link
Contributor

@JFCote JFCote left a comment

Choose a reason for hiding this comment

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

It look good and with proper review from the core team, I think it will be a very good addition to swagger-codegen. We need to make sure to generate the samples of every generator affected by this and make sure they still compile and run test properly.


if (operation.getResponses() != null && !operation.getResponses().isEmpty()) {
Response methodResponse = findMethodResponse(operation.getResponses());
Map.Entry<String, Response> methodResponse = findMethodResponse(operation.getResponses());
Copy link
Contributor

Choose a reason for hiding this comment

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

Since your are changing stuff in many other generators, did you make sure to generate all theses generators? Does it create any changes to the samples?

Copy link
Contributor Author

@ignaciomolina ignaciomolina Feb 16, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

this change affects to other generators but only changes code generated for java play framework, because successCode is used nowhere else

* @return default method response or <tt>null</tt> if not found
*/
protected Response findMethodResponse(Map<String, Response> responses) {
protected Entry<String, Response> findMethodResponse(Map<String, Response> responses) {
Copy link
Contributor

Choose a reason for hiding this comment

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

If I understand your code correctly, this seems to be a change to the CORE of swagger codegen since it change the way we handle return status. At the beginning, I though it was only Play Framework generator that was missing feature to return the good code but since you needed to add stuff in the DefaultCodegen, I guess it was missing from all codegen.

That is making this change request more important and I think that core members should review it too.

@wing328 Can you check this and also tag some core members to review this.

Thanks!

tzimisce012 and ignaciomolina reacted with thumbs up emoji
} ],
"responses" : {
"200" : {
"201" : {
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm surprised the json changed... If I'm not mistaken, this is simply a dump from swagger-parser in json format. Why is it now using the 201 code instead of 200. Did I miss something?

Copy link
Contributor

@tzimisce012 tzimisce012 Feb 16, 2018

Choose a reason for hiding this comment

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

Agreed.

Copy link
Contributor Author

@ignaciomolina ignaciomolina Feb 16, 2018

Choose a reason for hiding this comment

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

my mistake, I did a few tests and I add the file to the commit without notice it

Copy link
Contributor Author

How is it going? @wing328 😅

Copy link
Contributor

wing328 commented Feb 27, 2018

@ignaciomolina let me review tomorrow. Sorry for the delay.
ignaciomolina reacted with thumbs up emoji

Copy link
Contributor

wing328 commented Feb 28, 2018 •
edited
Loading

@ignaciomolina the change breaks the Pistache generator:

[main] INFO io.swagger.parser.Swagger20Parser - reading from modules/swagger-codegen/src/test/resources/2_0/petstore.yaml
[main] INFO io.swagger.codegen.AbstractGenerator - writing file /root/src/github.com/swagger-api/swagger-codegen/samples/server/petstore/pistache-server/model/ApiResponse.h
[main] INFO io.swagger.codegen.AbstractGenerator - writing file /root/src/github.com/swagger-api/swagger-codegen/samples/server/petstore/pistache-server/model/ApiResponse.cpp
[main] INFO io.swagger.codegen.AbstractGenerator - writing file /root/src/github.com/swagger-api/swagger-codegen/samples/server/petstore/pistache-server/model/Category.h
[main] INFO io.swagger.codegen.AbstractGenerator - writing file /root/src/github.com/swagger-api/swagger-codegen/samples/server/petstore/pistache-server/model/Category.cpp
[main] INFO io.swagger.codegen.AbstractGenerator - writing file /root/src/github.com/swagger-api/swagger-codegen/samples/server/petstore/pistache-server/model/Order.h
[main] INFO io.swagger.codegen.AbstractGenerator - writing file /root/src/github.com/swagger-api/swagger-codegen/samples/server/petstore/pistache-server/model/Order.cpp
[main] INFO io.swagger.codegen.AbstractGenerator - writing file /root/src/github.com/swagger-api/swagger-codegen/samples/server/petstore/pistache-server/model/Pet.h
[main] INFO io.swagger.codegen.AbstractGenerator - writing file /root/src/github.com/swagger-api/swagger-codegen/samples/server/petstore/pistache-server/model/Pet.cpp
[main] INFO io.swagger.codegen.AbstractGenerator - writing file /root/src/github.com/swagger-api/swagger-codegen/samples/server/petstore/pistache-server/model/Tag.h
[main] INFO io.swagger.codegen.AbstractGenerator - writing file /root/src/github.com/swagger-api/swagger-codegen/samples/server/petstore/pistache-server/model/Tag.cpp
[main] INFO io.swagger.codegen.AbstractGenerator - writing file /root/src/github.com/swagger-api/swagger-codegen/samples/server/petstore/pistache-server/model/User.h
[main] INFO io.swagger.codegen.AbstractGenerator - writing file /root/src/github.com/swagger-api/swagger-codegen/samples/server/petstore/pistache-server/model/User.cpp
Exception in thread "main" java.lang.RuntimeException: Could not process operation:
 Tag: Tag {
	name: user
	description: Operations about user
	externalDocs: io.swagger.models.ExternalDocs@e1e57bab
	extensions:{}}
 Operation: createUser
 Resource: post /user
 Definitions: {Order=io.swagger.models.ModelImpl@6b904c51, Category=io.swagger.models.ModelImpl@ba13e696, User=io.swagger.models.ModelImpl@9db581f0, Tag=io.swagger.models.ModelImpl@d651f3db, Pet=io.swagger.models.ModelImpl@c9d5d174, ApiResponse=io.swagger.models.ModelImpl@16a9e5a9}
 Exception: For input string: "default"
	at io.swagger.codegen.DefaultGenerator.processOperation(DefaultGenerator.java:935)
	at io.swagger.codegen.DefaultGenerator.processPaths(DefaultGenerator.java:814)
	at io.swagger.codegen.DefaultGenerator.generateApis(DefaultGenerator.java:434)
	at io.swagger.codegen.DefaultGenerator.generate(DefaultGenerator.java:749)
	at io.swagger.codegen.cmd.Generate.run(Generate.java:285)
	at io.swagger.codegen.SwaggerCodegen.main(SwaggerCodegen.java:35)
Caused by: java.lang.NumberFormatException: For input string: "default"
	at java.lang.NumberFormatException.forInputString(NumberFormatException.java:65)
	at java.lang.Integer.parseInt(Integer.java:580)
	at java.lang.Integer.parseInt(Integer.java:615)
	at io.swagger.codegen.languages.PistacheServerCodegen.fromOperation(PistacheServerCodegen.java:166)
	at io.swagger.codegen.DefaultGenerator.processOperation(DefaultGenerator.java:884)
	... 5 more

For the full log, please click on the link to the Shippable CI job.

Copy link
Contributor

wing328 commented Feb 28, 2018

I was able to repeat the issue locally by running ./bin/pistache-server-petstore.sh.

Copy link
Contributor Author

Shippable doesn't show me the logs, it says that I have to download it but I don't see any download button

Copy link
Contributor

wing328 commented Feb 28, 2018

@ignaciomolina can you try https://app.shippable.com/download/jobConsoles?jobId=5a965e1b9e550508008cef87? You will need to register an account at shippable and log in to obtain a valid token.

Copy link
Contributor Author

@wing328 solved!

Copy link
Contributor

wing328 commented Mar 1, 2018 via email •
edited
Loading

Thanks. I will review tonight.

Copy link
Contributor

wing328 commented Mar 1, 2018

cc the following technical committees as the change affects multiple generators:
ignaciomolina and stkrwork reacted with thumbs up emoji

Copy link
Contributor

wing328 commented Mar 8, 2018

If no further question/feedback, I"ll merge this PR on coming Friday.
ignaciomolina reacted with thumbs up emoji

Copy link
Contributor

edrevo commented Apr 26, 2018

@wing328 , it would be great if we could merge this PR :)

@ignaciomolina there is a merge conflict.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@JFCote JFCote JFCote left review comments

@tzimisce012 tzimisce012 tzimisce012 left review comments

Reviewers whose approvals may not affect merge requirements

At least 1 approving review is required to merge this pull request.

Assignees

No one assigned

Projects

None yet

Milestone

v2.4.0

Development

Successfully merging this pull request may close these issues.

AltStyle によって変換されたページ (->オリジナル) /