8000 Add Polymorphism Support by Fyro-Ing · Pull Request #1152 · springfox/springfox · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add Polymorphism Support #1152

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.

Sign up for GitHub

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

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

Add Polymorphism Support #1152

wants to merge 1 commit into from

Conversation

Fyro-Ing
Copy link
@Fyro-Ing Fyro-Ing commented Jan 22, 2016 •
edited by dilipkrish
Loading

I've added "Polymorphism support" (discriminator on parent, and allOf on child(s)) with springfox

{
  "definitions": {
    "Pet": {
      "type": "object",
      "discriminator": "petType",
      "properties": {
        "name": {
          "type": "string"
        },
        "petType": {
          "type": "string"
        }/
      },
      "required": [
        "name",
        "petType"
      ]
    },
    "Cat": {
      "description": "A representation of a cat",
      "allOf": [
        {
          "$ref": "#/definitions/Pet"
        },
        {
          "type": "object",
          "properties": {
            "huntingSkill": {
              "type": "string",
              "description": "The measured skill for hunting",
              "default": "lazy",
              "enum": [
                "clueless",
                "lazy",
                "adventurous",
                "aggressive"
              ]
            }
          },
          "required": [
            "huntingSkill"
          ]
        }
      ]
    },
    "Dog": {
      "description": "A representation of a dog",
      "allOf": [
        {
          "$ref": "#/definitions/Pet"
        },
        {
          "type": "object",
          "properties": {
            "packSize": {
              "type": "integer",
              "format": "int32",
              "description": "the size of the pack the dog is from",
              "default": 0,
              "minimum": 0
            }
          },
          "required": [
            "packSize"
          ]
        }
      ]
    }
  }
}

On java models :
Parent -> @apimodel(discriminator="id")
Child -> @apimodel(parent = Parent.class)

see Fyro-Ing@43923f5

@Fyro-Ing Fyro-Ing mentioned this pull request Jan 22, 2016
Closed
@dilipkrish dilipkrish added the PR label Jan 29, 2016
@dilipkrish dilipkrish added this to the 2.4.0 milestone Jan 29, 2016
@dilipkrish
Copy link
Member

@Fyro-Ing This pr works but I'd like to re-work some aspects of it. Comments inlined

dilipkrish
dilipkrish reviewed Jan 29, 2016
View reviewed changes
springfox-core/src/main/java/springfox/documentation/schema/Model.java
@@ -47,7 +48,8 @@ public Model(
String baseModel,
String discriminator,
List<String> subTypes,
String example) {
String example,
Model parent) {
Copy link
Member

Choose a reason for hiding this comment

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

I'd like tho use ModelRef here directly assuming some other plugin is responsible to dig out the Model from the @ApiModel#parent(). Typically by modifying the ModelDependencyProvider or creating a new implementation (plugin?) i.e. Parent => is a dependency of => Subclass

Secondly, we're anyways turning around and resolving it to a RefModel in the mapper.

@dilipkrish dilipkrish modified the milestones: 2.4.0, 2.5.0 Feb 8, 2016
@rcruzper
Copy link

it would be possible to add this feature in the milestone 2.4.0?

@dilipkrish
Copy link
Member

I could try be if we finish out this pr based on feedback it might be possible

@roni-frantchi
Copy link

Thanks for this.
We really need this to get our project going. This comes into play especially when you have mixings of accessor types where a parent class is serialized by field access and the child by property - flattening of the properties yields wrong results.

I see this feature is now pushed to milestone 2.5.0, is there an ETA for 2.5.0 ?

Thanks.

@roni-frantchi
Copy link

@dilipkrish @Fyro-Ing I think there's may be an issue with the above -
After having merged to 2.4.0 and testing the code above, it fails when attempting to inherit form a model not explicitly referenced by any other model explicitly used in the API.
I attempted using .additionalModels on my docket but that didn't help.
The only way around it was to actually include a reference to the parent class as a member.

So for the following:

@ApiModel(parent = Parent.class)
public class Child extends Parent {
...
}

will only work for me if Parent is explicitly referenced somewhere in the exposed API.
to workaround it I would have to:

@ApiModel(parent = Parent.class)
public class Child extends Parent {
    private Parent dummyReference;
...
}

Or perhaps add a dummy API...

Here's the stack trace the above fails if not using the workaround:

java.lang.IllegalStateException: Optional.get() cannot be called on an absent value
    at com.google.common.base.Absent.get(Absent.java:47) ~[guava-18.0.jar:na]
    at springfox.documentation.swagger.schema.ApiModelBuilder.apply(ApiModelBuilder.java:69) ~[springfox-swagger-common-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at springfox.documentation.schema.plugins.SchemaPluginsManager.model(SchemaPluginsManager.java:58) ~[springfox-schema-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at springfox.documentation.schema.DefaultModelProvider.modelBuilder(DefaultModelProvider.java:107) ~[springfox-schema-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at springfox.documentation.schema.DefaultModelProvider.modelFor(DefaultModelProvider.java:90) ~[springfox-schema-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at springfox.documentation.schema.DefaultModelProvider.dependencies(DefaultModelProvider.java:115) ~[springfox-schema-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at springfox.documentation.schema.CachingModelProvider.dependencies(CachingModelProvider.java:68) ~[springfox-schema-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at springfox.documentation.spring.web.scanners.ApiModelReader.populateDependencies(ApiModelReader.java:133) ~[springfox-spring-web-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at springfox.documentation.spring.web.scanners.ApiModelReader.read(ApiModelReader.java:76) ~[springfox-spring-web-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at springfox.documentation.spring.web.scanners.ApiListingScanner.scan(ApiListingScanner.java:88) ~[springfox-spring-web-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at springfox.documentation.spring.web.scanners.ApiDocumentationScanner.scan(ApiDocumentationScanner.java:69) ~[springfox-spring-web-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at springfox.documentation.spring.web.plugins.DocumentationPluginsBootstrapper.scanDocumentation(DocumentationPluginsBootstrapper.java:105) ~[springfox-spring-web-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at springfox.documentation.spring.web.plugins.DocumentationPluginsBootstrapper.onApplicationEvent(DocumentationPluginsBootstrapper.java:91) ~[springfox-spring-web-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at springfox.documentation.spring.web.plugins.DocumentationPluginsBootstrapper.onApplicationEvent(DocumentationPluginsBootstrapper.java:53) ~[springfox-spring-web-2.4.0-poly-2.4.0-gdba3710-2.jar:2.4.0-SNAPSHOT]
    at org.springframework.context.event.SimpleApplicationEventMulticaster.invokeListener(SimpleApplicationEventMulticaster.java:163) ~[spring-context-4.2.4.RELEASE.jar:4.2.4.RELEASE]
    at org.springframework.context.event.SimpleApplicationEventMulticaster.multicastEvent(SimpleApplicationEventMulticaster.java:136) ~[spring-context-4.2.4.RELEASE.jar:4.2.4.RELEASE]
    at org.springframework.context.support.AbstractApplicationContext.publishEvent(AbstractApplicationContext.java:381) ~[spring-context-4.2.4.RELEASE.jar:4.2.4.RELEASE]
    at org.springframework.context.support.AbstractApplicationContext.publishEvent(AbstractApplicationContext.java:335) ~[spring-context-4.2.4.RELEASE.jar:4.2.4.RELEASE]
    at org.springframework.context.support.AbstractApplicationContext.finishRefresh(AbstractApplicationContext.java:855) ~[spring-context-4.2.4.RELEASE.jar:4.2.4.RELEASE]
    at org.springframework.boot.context.embedded.EmbeddedWebApplicationContext.finishRefresh(EmbeddedWebApplicationContext.java:140) ~[spring-boot-1.3.2.RELEASE.jar:1.3.2.RELEASE]
    at org.springframework.context.support.AbstractApplicationContext.refresh(AbstractApplicationContext.java:541) ~[spring-context-4.2.4.RELEASE.jar:4.2.4.RELEASE]
    at org.springframework.boot.context.embedded.EmbeddedWebApplicationContext.refresh(EmbeddedWebApplicationContext.java:118) ~[spring-boot-1.3.2.RELEASE.jar:1.3.2.RELEASE]
    at org.springframework.boot.SpringApplication.refresh(SpringApplication.java:766) [spring-boot-1.3.2.RELEASE.jar:1.3.2.RELEASE]
    at org.springframework.boot.SpringApplication.createAndRefreshContext(SpringApplication.java:361) [spring-boot-1.3.2.RELEASE.jar:1.3.2.RELEASE]
    at org.springframework.boot.SpringApplication.run(SpringApplication.java:307) [spring-boot-1.3.2.RELEASE.jar:1.3.2.RELEASE]
    at org.springframework.boot.SpringApplication.run(SpringApplication.java:1191) [spring-boot-1.3.2.RELEASE.jar:1.3.2.RELEASE]
    at org.springframework.boot.SpringApplication.run(SpringApplication.java:1180) [spring-boot-1.3.2.RELEASE.jar:1.3.2.RELEASE]

@martinsefcik
Copy link

Any updates on this ?

@roni-frantchi
Copy link

See fixes I applied to swagger-core:
swagger-api/swagger-core#1741

thought maybe it'll help this project too - it seems to fix the same symptom as described in my previous comment as well.

@dilipkrish dilipkrish modified the milestones: 2.5.0, 2.6.0 Jun 2, 2016
@benfowler
Copy link

Hi. Any word on this PR?

@dilipkrish
Copy link
Member

@benfowler needs some work... actually may need to be re-implemented

@dilipkrish dilipkrish mentioned this pull request Jul 10, 2016
Closed
@dilipkrish dilipkrish mentioned this pull request Aug 27, 2016
Closed
@dilipkrish dilipkrish modified the milestones: 2.6.0, 2.7.0 Sep 25, 2016
@cemo
Copy link
cemo commented Feb 27, 2018

@dilipkrish I know you are not actively working on this project butthis PR is really important and will solve a bunch of issues on are side too. Would you please review it?

@dilipkrish
Copy link
Member

@cemo I am actively working on this project. Just not this PR :) This solution will not work when supporting OpenAPI 3.0 thats why its not pulled in.

@ralfhecktor
Copy link

@dilipkrish Why will it not work? Where do you see any problems? Can you please elaborate on that a bit. Whats with backwards compatibility for OAS 2?

@dilipkrish
Copy link
Member

@ralfhecktor composition based inheritance was introduced in 3.0 (allOf, anyOf etc.) with better schema support. Discriminator based inheritance isn't ideal when there is better support. I'd rather think the problem through "whole brain" than hack the solution together.

@cemo
Copy link
cemo commented Mar 12, 2018

@dilipkrish do you have any idea when 3.0 support will be landed?

@dilipkrish
Copy link
Member
dilipkrish commented Mar 13, 2018 •
edited
Loading

Im hoping in June. If I get some help with PR's etc. may be earlier

@raderio
Copy link
raderio commented Apr 3, 2018

If I get some help with PR's etc. may be earlier

How can we help?

@dilipkrish
Copy link
Member

@raderio there are plenty of issues where I could use help

@raderio
Copy link
raderio commented Apr 5, 2018 •
edited by dilipkrish
Loading

Discriminator based inheritance isn't ideal when there is better support.

It is still supported in OpenAPI 3 https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/ Discriminator section, if it is better than why they support it?

Also discriminator is supported by Jackson https://gist.github.com/christophercurrie/8939489

https://github.com/Rebilly/ReDoc/blob/master/docs/images/discriminator-demo.gif

@dilipkrish
Copy link
Member

Im not denying there is support, just suggesting there are better ways of doing it @raderio

@raderio
Copy link
raderio commented Apr 6, 2018

If there are better ways of doing it, why OpenAPI 3 supports it, why they just don't dropped it? Maybe because in some cases discriminator fits better?
I think library/specification should give tools to allow to implement different solutions, to use or not to use the discriminator approach is choice of application side.

@dilipkrish
Copy link
Member

@raderio Will definitely revisit this PR when it comes time to look at open api 3 support

@ghost ghost removed the next label Apr 15, 2018
@dilipkrish
Copy link
Member

@Fyro-Ing thanks for your PR, while I didnt incorporate all of your changes I cherry picked the important changes you contributed to the ModelMapper 🤘

@raderio
Copy link

raderio commented Apr 17, 2018

@dilipkrish coooool
It will be available in next release? an ETA for next release?
Is it possible somehow to test/use before release?

@dilipkrish
Copy link
Member

@raderio targetting this weekend. Yes feedback will be awesome! You can use the SNAPSHOT build

@raderio
Copy link
raderio commented Apr 18, 2018

@dilipkrish I've tested, it works, big thanks.

@cemo
Copy link
cemo commented Apr 18, 2018

@raderio If you don't mind, would you please share your use case?

@dilipkrish
Copy link
Member

Thanks @raderio for confirming! 👍

@cemo here is the example use case 👇

eb6ca4d

Sorry, something went wrong.

@dilipkrish dilipkrish added this to the 2.9.0 milestone Apr 21, 2018
@dilipkrish dilipkrish mentioned this pull request Apr 21, 2018
Closed
@m-kay m-kay mentioned this pull request Jul 18, 2018
Open
@diogosouza
Copy link
diogosouza commented Jul 31, 2018 •
edited
Loading

Guys, is there any idea if this would be applied to springfox-swagger-ui as well?
Testing here, it doesn't change the ui's models presentation...

@dilipkrish
Copy link
Member

@diogosouza What are you seeing/not-seeing?

@MrDefinite
Copy link

Hi @dilipkrish,
I can see the the child class model in swagger ui, which has all the properties inherited from parent class. But there is no description telling which parent it is inherited from the ui. Is it possible to have such a tag on ui?

@dilipkrish
Copy link
Member

@MrDefinite not if its not supported in the spec

@zpf7879
Copy link
zpf7879 commented Sep 3, 2018

Hi @dilipkrish,
I have the same question as @MrDefinite. In this situation, it is difficult for my customer to use the API esp. for the PUT operation, e.g. create(Animal). First, the customer needs to find the derived class, and he also needs to remember to provide the /@ discriminator.

{
"@type": "Cat",
"id": 2,
"color": null,
"parent": null,
}

Any suggestion?

@dilipkrish
Copy link
Member

No. Don't use inheritance in an API. It exposes the type system and implementation language internals.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
PR
Projects
None yet
Milestone
2.9.0
Development

Successfully merging this pull request may close these issues.
0