Skip to content

Conversation

@roni-frantchi
Copy link

...And applied changes based on @fehguy feedback here: #1741 (comment)

Now, the parent attribute is no longer part of the generated JSON (or model for that matter), so it is now valid in accordance to the spec.

…into 'master' of https://github.com/sapienstech/swagger-core

# Conflicts:
#	modules/swagger-annotations/pom.xml
#	modules/swagger-core/pom.xml
#	modules/swagger-core/src/test/resources/Cat.json
#	modules/swagger-hibernate-validations/pom.xml
#	modules/swagger-jaxrs/pom.xml
#	modules/swagger-jersey-jaxrs/pom.xml
#	modules/swagger-jersey2-jaxrs/pom.xml
#	modules/swagger-models/pom.xml
#	modules/swagger-mule/pom.xml
#	modules/swagger-servlet/pom.xml
#	pom.xml
# Conflicts:
#	modules/swagger-core/src/main/java/io/swagger/jackson/ModelResolver.java
@morpheus9999
Copy link

Any news on this issue?

@roni-frantchi
Copy link
Author

@morpheus9999 still waiting for someone to review my work.. it's now been more than 6 months...

@ErikGrimes
Copy link

@roni-frantchi Will your changes cover the case where there are multiple abstract subtypes in the inheritance hierarchy? For example:

abstract class A {};
abstract class B extends A {};
class C extends B {};

@roni-frantchi
Copy link
Author

roni-frantchi commented Aug 15, 2017

@ErikGrimes Yes they do.
But one would have to use the API annotations in this particular case.
e.g.

interface I {...};

@ApiModel(parent = I.class)
abstract class A implements I {...};

@ApiModel(parent = A.class)
class B extends A {...};

@ErikGrimes
Copy link

  1. How does parent relate to subtypes?
  2. Will this fix apply to swagger-core's native understanding of jackson's annotations?

@roni-frantchi
Copy link
Author

@ErikGrimes:

  1. I'm not sure what you mean by subTypes;
    Setting the parent is then used to inverse and add child (subTypes?) nodes to the specified parent. See this line: https://github.com/swagger-api/swagger-core/pull/2037/files#diff-b96cf6dc444f477102be00b2ab470836R109

  2. I'm not entirely sure what you mean by that... Could you provide an example?

@ErikGrimes
Copy link

ErikGrimes commented Aug 16, 2017

@roni-frantchi

  1. ApiModel has a subtypes attribute that's used to describe inheritance.
@ApiModel(value="RootType", discriminator = "type", subTypes = {AbstractType1.class, ConcreteType1.class})
public class RootType {
}

@ApiModel
public class AbstractType1 extends RootType {
    @ApiModelProperty
    public String value;
}

@ApiModel
public class ConcreteType1 extends AbstractType1 {
}

I expect swagger-core to mirror Jackson's behavior, where you list the concrete subtypes using @ApiModel(subTypes={}) and it walks the inheritance tree, creating definitions for each subtype in the tree. Instead, it produces a definition for each leaf in the tree, pushing the properties for intermediate types down into the leaf type.

definitions:
  AbstractType1:
    allOf:
    - $ref: "#/definitions/RootType"
    - type: "object"
      properties:
        value:
          type: "string"
  ConcreteType1:
    allOf:
    - $ref: "#/definitions/RootType"
    - type: "object"
      properties:
        value:
          type: "string"
  RootType:
    type: "object"
    discriminator: "type"

I haven't really found a description of how parent is intended to be used or how it differs from subtypes.

So, I'm trying to understand:

Will this patch fix the problems with using intermediate subtypes defined using the subtypes attribute described above?

How is the parents attribute intended to differ from subtypes? What should it be used for?

  1. As I understand it, swagger-core consumes Jackson annotations to create the specification. So, you don't need to replicate the information already contained in the Jackson annotations by adding Swagger annotations. Here is an example of Jackson annotations
@JsonTypeInfo(
		  use = JsonTypeInfo.Id.NAME,
		  include = JsonTypeInfo.As.PROPERTY, 
		  property = "type")
		@JsonSubTypes({ 
		  @Type(value = ConcreteType1.class, name = ConcreteType1")
		})
class ParentType {}

class AbstractType1 extends ParentType {}

class ConcreteType1 extends AbstractType1 {}

Will this patch generate definitions for subtypes as expected when the information is contained in the Jackson annotations instead of in ApiModel annotations?

@roni-frantchi
Copy link
Author

roni-frantchi commented Jan 9, 2018

@ErikGrimes Say your project has n+1 modules.
A single core module, and set of extension-0...extension-n modules who depend on your core.
In your core module you would have a Base class and your extensions have an Extension0...ExtensionN classes, all of which inherit from your Base class.

You cannot use the subtypes property, as it would:

  1. Be impossible to list them as Classes, due to compile time dependencies.
  2. A nightmare to list and maintain their fully qualified String representation.
  3. Impossible to add an n+1 extension without recompiling your core.

@frantuma frantuma changed the base branch from master to 1.5 April 30, 2018 15:32
@francesco995
Copy link

@ErikGrimes Say your project has n+1 modules.
A single core module, and set of extension-0...extension-n modules who depend on your core.
In your core module you would have a Base class and your extensions have an Extension0...ExtensionN classes, all of which inherit from your Base class.

You cannot use the subtypes property, as it would:

  1. Be impossible to list them as Classes, due to compile time dependencies.
  2. A nightmare to list and maintain their fully qualified String representation.
  3. Impossible to add an n+1 extension without recompiling your core.

We have the same problem,
Have you found a solution?
We also use swagger-codegen to generate the clients, so those classes that are not included in the core package need to be rewritten.

@roni-frantchi
Copy link
Author

@francesco995
We've worked off a fork based on this PR ever since. It's been quite a while as you can see, and have yet to hear back from @ErikGrimes or a core maintainer who'd be able to merge this PR in on what should else should I fix in order to move forward with this one.

@ErikGrimes
Copy link

@roni-frantchi I haven't really looked at this in awhile, but now I'm using a code a schema first approach with the OpenAPI generator tool. I've forked the tool to improve inheritance support. Hopefully, I'll be able to get it into a pull request and have it merged back upstream. So, I've bypassed the problem more that solved it.

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 this pull request may close these issues.

4 participants