OpenLiberty / open-liberty

Open Liberty is a highly composable, fast to start, dynamic application server runtime environment
https://openliberty.io
Eclipse Public License 2.0
1.15k stars 591 forks source link

[REQ] Make OpenAPI produce more usable error messages #12936

Open bmarwell opened 4 years ago

bmarwell commented 4 years ago

Is your feature request related to a problem? Please describe.

When defining a @Header in the @ApiResponse, I wanted to use a string wrapper class like so:

@APIResponse(
          responseCode = "201",
          description = "task created successfully",
          headers = {@Header(
              name = "task-id",
              description = "ID of the created task.",
              required = true,
              allowEmptyValue = false,
              schema = @Schema(implementation = TaskId.class)
          )}
      )
@POST
@Path("/")
public Response postTask(TaskContents body);

While this code does not produce a valid OpenAPI output, the error message from OpenLiberty is a bit misleading:

[ERROR   ] CWWKO1650E: Validation of the OpenAPI document produced the following error(s): 

 - Message: The "task-id" Header Object must contain either a schema property or a content property, Location: #/paths/~1task/post/responses/201/headers/task-id

Well… it does contain a schema. It is just not valid. Also, a @Header may not contain content, despite the error message suggests so.

Describe the solution you'd like

  1. Warn about the invalid @Schema for @Header, do not silently ignore it. => What fields are actually needed? Does the type need to be SchemaType.STRING?
  2. (maybe) warn the user that potential string-wrapping classes are not supported and will not be evaluated. With "type wrapping classes" I mean classes which only have one getter method (e.g. @JsonbValue T getValue()). @JsonbValue is currently not supported, though. Think of it as @JsonValue from Jackson.

Describe alternatives you've considered

Reading https://openliberty.io/guides/microprofile-openapi.html. Maybe a hint would be useful why which fields are populated in which annotation.

Additional context

yeekangc commented 4 years ago

@arturdzm @leochr @arthurdm

msmiths commented 4 years ago

@bmhm I am little confused about why this issue has been raised as an enhancement request and not a bug. When I add the @APIResponse annotation that you have specified to an example JAX-RS resource in an OpenLiberty server using the mpOpenAPI-1.1 feature, the relevant section of the OpenAPI document that is generated is as follows:

      responses:
        201:
          description: task created successfully
          headers:
            bars-id:
              description: ID of the created bar.
              required: true
              style: simple

However, the same section that is generated when using the SmallRye implementation is as follows:

      responses:
        201:
          description: task created successfully
          headers:
            bars-id:
              description: ID of the created bar.
              required: true
              allowEmptyValue: false
              style: simple
              schema:
                $ref: '#/components/schemas/UUID'

This suggests that the OpenAPI document that is generated by OpenLiberty is incomplete and that the corresponding validation message is correct, as it relates to the schema property.

Regarding the content property, I believe that the header validation has been implemented based on the wording of the OpenAPI specification, which states that:

The Header Object follows the structure of the Parameter Object...

... and the Parameter object section states:

A parameter MUST contain either a schema property, or a content property, but not both.

Also, it appears that the use of the content property within a header has been the cause of some confusion for a while... see the following issue for an example: https://github.com/OAI/OpenAPI-Specification/issues/996.

However, given that I am unable to find an example of using the content property in the context of a header, and the fact that the content property is not defined on the header annotation, I will modify relevant validation message on the next version of the feature to remove any mention of the content property.

bmarwell commented 4 years ago

I am little confused about why this issue has been raised as an enhancement request and not a bug.

Hey! Well, I wasn't so sure tbh. I thought it may just be an incomplete error message. I do not have a lot of experience with OpenAPI/Swagger yet. Also, I had NPEs in Liberty's CXF implementation which were regarded as "correct" and downgraded to an RFE by IBM. 🤷🏼‍♂️ But If I discuvered a bug, the better.

Thanks for looking into it. From what I understand, I agree to your findings and am looking forward to the fixes. Thanks!

msmiths commented 4 years ago

@bmhm Sorry to be a pain, but please could you raise a separate issue to track the bug in the generation of the OpenAPI document? We can continue to use this issue to track the enhancement request for the validation message.