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

Support for additional reference types (other than JDK8 and Guava Optional) #4103

Closed
dpeger opened this issue Jan 26, 2022 · 8 comments · Fixed by #4106
Closed

Support for additional reference types (other than JDK8 and Guava Optional) #4103

dpeger opened this issue Jan 26, 2022 · 8 comments · Fixed by #4106

Comments

@dpeger
Copy link
Contributor

dpeger commented Jan 26, 2022

I'm using reference types (i.e. AtomicReference<>) in my domain objects to model the three-state semantic of set (value is available), not set (no value available or not of interest) and unset (clear existing value). This is supported out-of-the-box by Jackson and is mapped to <value>, undefined and null in JSON/JavaScript respectively.

Everything work pretty well except for the generated API documentation. I'm using swagger 1.6.2 with spring (not boot) and CXF. The problem is that reference types are not "unwrapped" in the generated documentation.

For example the type MyDomainObject has a single String field description. In Java this value is wrapped by AtomicReference as outlined above:

@JsonInclude(value = Include.NON_NULL)
public class MyDomainObject {
  private AtomicReference<String> description;

  public AtomicReference<String> getDescription() {
    return description;
  }

  public void setDescription(AtomicReference<String> description) {
    this.description = description;
  }
}

And the corresponding type definition in the generated swagger looks like this:

definitions:
  MyDomainObject:
    type: "object"
    properties:
      description:
        $ref: "#/definitions/AtomicReferenceString"

  AtomicReferenceString:
    type: "object"
    properties:
      opaque:
        type: "string"
      acquire:
        type: "string"
      plain:
        type: "string"

But this is not correct as a JSON value for this type would look like this:

{
  description: "Some valuable text"
}

That is the swagger should simply look like this:

MyDomainObject:
  type: "object"
  properties:
    description:
      type: "string"

I tried to find a workaround for this problem and found that both JDK8 and Guava's Optional are supported by swagger out-of-the-box in the described way. However (IMHO) using Optional to model the described 3-state semantic kind of contradicts the intention of Optional as you'd have to perform null-checks on the Optional field - just my opinion. I know that AtomicReference was invented with another intention in mind as well but, as this is supported by jackson-databind (without additional modules), I think this is currently the best way to implement this semantic.

Browsing through the swagger code I found that support for additional reference types would be quite easy to add (OptionalUtils in 2.X and ModelResolver._isOptionalType in 1.6).

I'd happy to provide PRs for this enhancement but want to make sure this change is going to be accepted for both 1.6 and 2.X branches of swagger.


For completeness:

I posted related questions on stackoverflow and smartbear already but got no feedback, as of now

@dpeger
Copy link
Contributor Author

dpeger commented Mar 10, 2022

@frantuma, @HugoMario I hate to mention random maintainers here but is there any chance this issue and the linked PRs (#4105, #4106) will get some love anytime soon?

@dpeger
Copy link
Contributor Author

dpeger commented Jun 17, 2022

@frantuma, @HugoMario another 3 month later. Could someone perhaps have a look? The fix is non-breaking, straight forward and will add support for the same reference types supported by swagger.

@mjvmroz
Copy link

mjvmroz commented Jun 21, 2022

On a related note, it'd be great to be able to parameterize Swagger with a custom ObjectMapper or get a hook to modify it, so reference types external to the Jackson Databind project can be used.

Example use-cases are for Fugue and Arrow Option types (example).

Is this something that maintainers would be open to?

@dpeger
Copy link
Contributor Author

dpeger commented Jun 22, 2022

@MiriChan as far as I understand the kotlin sources in your example my PRs would address the Option types as well as ResolvedType.isReferenceType will return true for these types

@mjvmroz
Copy link

mjvmroz commented Jun 23, 2022

Yeah that's my understanding too, provided that Swagger's ObjectMapper has those modules registered.

From my glance around the codebase earlier I thought it wasn't possible to BYO or customise Swagger's ObjectMapper instance, but after some cursory searches it looks like I was wrong.

Anyway, psyched for this change to land. Hope it gets some love soon!

frantuma pushed a commit that referenced this issue Nov 2, 2022
In addition to the hard coded `Optional` reference types the type information from jackson is used to unwrap additional reference types (e.g. `AtomicReference`)
frantuma pushed a commit that referenced this issue Nov 2, 2022
In addition to the hard coded `Optional` reference types the type information from jackson is used to unwrap additional reference types (e.g. `AtomicReference`)
frantuma pushed a commit that referenced this issue Nov 2, 2022
In addition to the hard coded `Optional` reference types the type information from jackson is used to unwrap additional reference types (e.g. `AtomicReference`)
@frantuma
Copy link
Member

frantuma commented Nov 2, 2022

Thanks!! merged #4105 and #4106

On the side ObjectMapper used by resolver can be customized by providing a objectMapperProcessorClass in configuration

@RickHuizing
Copy link

My project is experiencing issues because of this commit, schema generation fails when using OptionalInts as type for a field. It seems like these were not classified as 'reference types' before and also do not have 'contained types', which leads to a null-pointer.

@dpeger
Copy link
Contributor Author

dpeger commented Aug 31, 2023

@RickHuizing would you mind to open a separate issue for your problem. Best with some MVCE to help in reproducing the bug.

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

Successfully merging a pull request may close this issue.

4 participants