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

Incompatibility with Swagger annotations #319

Open
innokenty opened this issue Aug 30, 2021 · 6 comments
Open

Incompatibility with Swagger annotations #319

innokenty opened this issue Aug 30, 2021 · 6 comments

Comments

@innokenty
Copy link

So I'm trying to generate the swagger API from code with swagger annotations.
It all works well except Problem.statusType field.

Description

I am describing my endpoint's responses like this:

@Operation(
    description = "Retrieve entity by id",
    responses = {
        @ApiResponse(
            responseCode = "404",
            description = "Entity not found",
            content = @Content(
                schema = @Schema(implementation = Problem.class),
                mediaType = APPLICATION_PROBLEM_JSON_VALUE))})

But because Problem.statusType field is not an integer as per API specification, but a class – the generated swagger output is wrong.

Expected Behavior

The following component are generated for swagger yaml:

Problem:
  type: object
  properties:
    instance:
      type: string
      format: uri
    type:
      type: string
      format: uri
    parameters:
      type: object
      additionalProperties:
        type: object
    title:
      type: string
    status:
      type: integer # <------- this
    detail:
      type: string

Actual Behavior

The following components are generated for swagger yaml:

Problem:
  type: object
  properties:
    ...
    status:
      $ref: '#/components/schemas/StatusType'
StatusType:
  type: object
  properties:
    statusCode:
      type: integer
      format: int32
    reasonPhrase:
      type: string

Possible Fix

Do you think this can be fixed at the library level? Or should I create a bug in problem spring web adapter better?

@innokenty innokenty added the Bug label Aug 30, 2021
@innokenty
Copy link
Author

Well it's probably not a bug, but I'm not sure what it is.
I think it should be tackled though.

@bushwakko
Copy link

bushwakko commented May 3, 2022

This is because the implementation used by the problem-detail library, has StatusType in the POJO, and registers a Jackson-Module to the ObjectMapper, that provides code to serialize the StatusType class to ensure the serialized output is an integer. The code that builds the model in Swagger (ModelResolver), however, tries to makes its own interpretation of the Problem class. Since the Module contains serialization code, and not any declarative config/metadata, it doesn't take into account how that object is serialized by Jackson's ObjectMapper.

I'm not sure what the solution is though, other than it needs to take into account all the information Jackson has.

It would have to analyze the Jackson-configuration, find which classes are serialized differently, and account for that in the model. Alternatively, the ModelResolver could instead populate and serialize a dummy object, analyze the resulting json and try to make a model based on that, or at least see where the json differs from its current model.

Edit: The ModelResolver method "resolve()" that does the interpretation, is 750 (!) lines long, and thus very hard to understand.

@innokenty
Copy link
Author

Exactly. And I think this is the only place that's different, so I think the best solution would be to make the model same as json. Maybe there is a reason though, why it was made different..

@Solverj
Copy link

Solverj commented May 3, 2022

I was thinking the same, maybe the reason for this is on a higher level. Perhaps swagger wants to serialize everything "raw" and not be modelled differently as an application grows with more registered modules on how to serialize such that you maintain the integrity of your contracts - this is pure speculation though.

This might be more of a question for swagger than zalando perhaps.

@bushwakko
Copy link

The biggest technical issue here is that without a declarative description of the serialization rules in the POJO or in the Jackson-Module/ObjectMapper, it's very hard to deduce what the actual output is. It could be possible to pick up the serialization functions, but the output is either Object or String, so you won't know what type it actually is.

A hacky way to figure it out, could be to populate each field in the POJO with a value describing the type of the field, then serializing the object and then mapping back from the string-values on each json property to a type, for use in the model.

@whiskeysierra
Copy link
Collaborator

so I think the best solution would be to make the model same as json. Maybe there is a reason though, why it was made different..

In a nutshell: https://whiskeysierra.github.io/knowledge/primitive-obsession/

The code that builds the model in Swagger (ModelResolver), however, tries to makes its own interpretation of the Problem class. Since the Module contains serialization code, and not any declarative config/metadata, it doesn't take into account how that object is serialized by Jackson's ObjectMapper.

That approach is flawed by design, because it can't possibly detect cases that aren't defined in a declarative way. The use of an imperative converter (a feature that Jackson supports out of the box) breaks it.

It could be possible to pick up the serialization functions, but the output is either Object or String, so you won't know what type it actually is.

Yes, that would actually be a) easier to implement, b) simpler in design and c) more robust. (Also probably a bit slower, but someone would need to benchmark that.)

Zalando follows an API first principle which contradicts the usage of tools that annotate code and generate the API from that.
You shouldn't expect any support here.
If you're willing to put time into this and you find a solution that is acceptable in terms of maintenance over time, I'm sure there would be a way to get that in.

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

No branches or pull requests

4 participants