Skip to content

Java support inheritance #1094

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

Closed

Conversation

ahgittin
Copy link

as per #1015 (and #946) this is now optional and disabled by default. this results in extra unnecessary code, masking fields in some cases, and breaks code generated for jax-rs in certain cases (enums with inheritance).

as noted by @xhh at #1050 this is not a perfect fix, but it improves the situation significantly (restoring master branch behaviour, as the current situation is a regression; my spec builds fine with master but not with develop_2.0 ?).

@wing328
Copy link
Contributor

wing328 commented Aug 20, 2015

@ahgittin do you mind sharing the spec with us assuming it can be shared publicly?

@ahgittin
Copy link
Author

@wing328 here's a cut-down test case whose -l jaxrs makes for invalid java without this, valid java with this:

swagger: '2.0'
info:
  title: buggy inheritance for java
  version: "0.0.1"
definitions:
  Super:
    properties:
      my_enum:
        type: string
        enum: [ A, B ]

  Sub:
    allOf:
    - $ref: '#/definitions/Super'
    - type: object
    properties:
      another:
        type: string

paths:
  /home:
    get:
      summary: with develop_2.0 prior to #1094 the java generated fails

@ahgittin ahgittin force-pushed the java-support-inheritance branch from e7bf725 to 7c90403 Compare August 20, 2015 11:59
@ahgittin
Copy link
Author

also noticed this had some strange commits; i've rebased so it's just the one-line change

@webron
Copy link
Contributor

webron commented Aug 20, 2015

FWIW, that's not inheritance, that's composition. In this case, Sub and Super have no parent-child relationship. The end result in this case would be that Sub would include the same properties as Super but will not inherit from it.

@ahgittin
Copy link
Author

sorry @webron i mis-typed the example, it's now correct, and it definitely is implemented as inheritance (maybe it shouldn't be):

public class Sub extends Super {

  public enum MyEnumEnum {
     A,  B, 
  }

redeclaring the enum breaks compilation as Super.getMyEnum() returns the parent's MyEnumEnum so Sub.getMyEnum() has an incompatible return type

HTH

@webron
Copy link
Contributor

webron commented Aug 20, 2015

Not sure what's changed, but as long as there's no discriminator, there's no inheritance (from the spec's point of view).

@wing328
Copy link
Contributor

wing328 commented Aug 20, 2015

If we look at the Java client in the "master" branch, we'll find that it implements allOf using extends in Java, which is not the correct implementation as Ron has pointed out that allOf is model composition (not inheritance)

@xhh previously attempt to fix allOf with #946 (copy parent properties) but it does not implement discriminator (should be implemented as extends in Java) due to time constraint.

@tandrup introduced supportsInheritance to let people choose the behaviour with #1015.

Before someone from the community implements discriminator, we can merge this PR for the time being to keep the old (master) behaviour if no one has any concern.

@webron
Copy link
Contributor

webron commented Aug 20, 2015

I'd suggest putting a hold on the discriminator implementation as there's a technical detail with the spec we need to resolve first. It'll be done with in the upcoming weeks, and will come with an implementation (or suggestion for implementation). Sorry for the trouble on this one.

fehguy and others added 10 commits August 24, 2015 00:39
as per swagger-api#1015 (and swagger-api#946) this is now optional and disabled by default.
this results in extra unnecessary code, masking fields in some cases, and
breaks code generated for jax-rs in certain cases (enums with inheritance).
@ahgittin ahgittin force-pushed the java-support-inheritance branch from 7c90403 to 813d519 Compare August 24, 2015 10:06
@ahgittin
Copy link
Author

closing and will resubmit against master as per change to swagger PR process

@ahgittin
Copy link
Author

closed in favour of #1120

@steverice
Copy link

there's a technical detail with the spec we need to resolve first

@webron are you referring to OAI/OpenAPI-Specification#403?

@webron
Copy link
Contributor

webron commented Oct 28, 2015

That is correct. It should be resolved soon.

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.

6 participants