swagger-api / swagger-codegen

swagger-codegen contains a template-driven engine to generate documentation, API clients and server stubs in different languages by parsing your OpenAPI / Swagger definition.
http://swagger.io
Apache License 2.0
16.91k stars 6.03k forks source link

[Spring] Bug @ApiResponse ignores different responses #3371

Closed frasch1712 closed 7 years ago

frasch1712 commented 8 years ago
Description

Swagger Spring Boot Code generation ignores different schema references in responses.

Actually the following is generated:

@ApiResponses(
    value =
        {
            @ApiResponse(code = 200, message = "Successful response", response = Product.class),
            @ApiResponse(code = 400, message = "Bad Request", response = Product.class),
        }
)

I would expect the following "response = Error.class for code = 400" (see below for the yaml definition):

@ApiResponses(
    value =
        {
            @ApiResponse(code = 200, message = "Successful response", response = Product.class),
            @ApiResponse(code = 400, message = "Bad Request", response = Error.class),
        }
)
Swagger-codegen version

master branch

Swagger declaration file content or url

Declaration in yaml (extract):

paths:
   ...
   get:
   ...
      responses:
        200:
          description: Successful response
          schema:
            $ref: "#/definitions/Product"
        400:
          description: Bad Request
          schema:
            $ref: "#/definitions/Error"
Command line used for generation

java -Dmodels -Dapis -jar swagger-codegen-cli.jar generate -i swagger.yaml -l spring

additional config:

{
  "modelPackage": "...",
  "apiPackage": "...",
  "interfaceOnly": true,
  "hideGenerationTimestamp": true,
  "dateLibrary":"java8",
  "java8": true,
  "library": "spring-boot"
}
wing328 commented 8 years ago

@frasch1712 I don't think the custom error model is supported at the moment. May I know if you've cycle to contribute the enhancement?

cc @cbornet

frasch1712 commented 8 years ago

I investigated a bit in it and think it is only a small change in the mustache template. So I will try to contribute the enhancement.

ok11 commented 8 years ago

And just for the record -- the problem is that the template uses returnTypeproperty, which gets resolved in the higher (operation) context, and thus gives really an operation return type, Product in the example above; instead the template should use baseType, which is a property of the response. So, a bug in the api.mustache . I have fixed locally, will wait until the pull request by @frasch1712 gets accepted into the upstream.

wing328 commented 8 years ago

@ok11 I don't see any PR from @frasch1712 : https://github.com/swagger-api/swagger-codegen/pulls/frasch1712

For the time being, I would suggest you to file one for review. Thanks.

fdasoghe commented 7 years ago

I have this same issue. Is there any progress on it? Or maybe a workaround?

ebautistabar commented 7 years ago

Hi, first of all congrats on this amazing project! Now on to the issue, just a couple of comments:

wing328 commented 7 years ago

@ebautistabar

are there plans to merge #4801 for the next release?

Yes, we want to merge it. Did you have a chance to test the first? Does it work for you?

when setting a 200 response and a default response in the yaml file, the generator creates just 2 @ApiResponse objects, both containing code 200; I think this point was not raised in the issue so far but it would be nice if it were fixed too

I would recommend opening a new issue for tracking so as to more easily draw attention from the community.

ebautistabar commented 7 years ago

@wing328 I have tested the PR and it seems to solve the issue described by @frasch1712.

I will open a new issue to describe the other problem.

gcernier-amadeus commented 7 years ago

Would it be possible to merge #4801 so that I will have a possibility to provide another fix of another issue I spotted around the same feature, before v2.3.0 milestone (so that this version will contain the 2 fixes)?

wing328 commented 7 years ago

@gcernier-amadeus I'll try to merge it later today after resolving the merge conflicts.

wing328 commented 7 years ago

Closed via #4801. Thanks for the fix by @gcernier-amadeus

Please pull the latest master to give it a try.