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

v3 swagger-core module notes, major todo and open questions #2312

Closed
frantuma opened this issue Jul 14, 2017 · 7 comments
Closed

v3 swagger-core module notes, major todo and open questions #2312

frantuma opened this issue Jul 14, 2017 · 7 comments

Comments

@frantuma
Copy link
Member

branch for current effort: https://github.com/frantuma/swagger-core/tree/compound-models-tests-restructure

notable things related questions, and todo (point 10):

  1. added support for Discriminator support in swagger-annotations (see updated Schema annotation, and new annotation DiscriminatorMapping)

  1. users need to use Jackson @JsonSubTypes annotation on "parent" bean, if they want to be able to resolve "children" (e.g. beans with allOf property in @Schema annotation); this is similar to 2.0, but 2.0 allowed also to define a swagger @apimodel(subtypes=..) annotation.

As an example if we define a "parent" bean like:

    @JsonSubTypes({@JsonSubTypes.Type(value = Sub1Bean.class, name = "sub1")})
    static class BaseBean {
        public String type;
        public int a;
        public String b;
    }

and a "child":

    @io.swagger.oas.annotations.media.Schema(description = "Sub1Bean", allOf = {BaseBean.class})
    static class Sub1Bean extends BaseBean {
        public int c;
    }

when we resolve with a code like:

modelResolver = new ModelResolver(new ObjectMapper());
context = new ModelConverterContextImpl(modelResolver);
final Schema baseModel = context.resolve(BaseBean.class);

then both BaseBean and Sub1Bean are resolved

if a @JsonSubTypes is not declared on the parent, resolver won't be able to "know" about Sub1Bean which will not be resolved.


  1. we are resolving a "child" schema (e.g. with allOf) also without needing the "parent" (e.g. the schema/bean referenced in child allOf) to declare the "child" as subtype (i.e. with @JsonSubTypes); in 2.0 handling of @apimodel annotations that was instead mandatory.

e.g with:

    static class BaseBean {
        public String type;
        public int a;
        public String b;
    }

and a "child":

    @io.swagger.oas.annotations.media.Schema(description = "Sub1Bean", allOf = {BaseBean.class})
    static class Sub1Bean extends BaseBean {
        public int c;
    }

when we resolve with a code like:

final Schema baseModel = context.resolve(Sub1Bean.class);

then also BaseBean will be resolved; this was different in 2.0, where a @JsonSubTypes was needed on the parent. See temporarily commented code here


  1. atm anyOf (regardless if interfaces) are resolved not as ref model but inline; this is not true for oneOf or allOf. @fehguy @webron what would be the best behaviour here? in 2.0 ref model was used in allOf, possibly we can:

A. resolve anyOf, allOf, and oneOf as referenced models
B. resolve anyOf, allOf, and oneOf as inline models (duplicating possibly definitions)
C. resolve anyOf inline, only if referenced type is interface


  1. in case a @Schema (allOf = ..) annotation is present and/or a @JsonSubTypes is declared on the parent bean, and other properties are declared in the "child" bean, the result JSON will have an allOf field holding the parent schema refs, and will declare its own properties in the properties field.
    This is different from 2.0 behaviour, which "merged" also bean "own" properties into allOf array; I guess the former behaviour is possibly more correct, also possibly previous behaviour was related to the way allOf resolving was handled (parent/child "trigger' methods on ComposedModel).

As an example if we define a "parent" bean like:

    @JsonSubTypes({@JsonSubTypes.Type(value = Sub1Bean.class, name = "sub1")})
    static class BaseBean {
        public String type;
        public int a;
        public String b;
    }

and a "child":

    static class Sub1Bean extends BaseBean {
        public int c;
    }

the serialized JSON/YAML will look like:

{
    "BaseBean": {
        "type": "object",
        "properties": {
            "type": {
                "type": "string"
            },
            "a": {
                "type": "integer",
                "format": "int32",
            },
            "b": {
                "type": "string"
            }
        }
    },
    "Sub1Bean": {
        "allOf": [
            {
                "$ref": "#/components/schemas/BaseBean"
            }        
        ],
        "type": "object",
        "properties": {
            "d": {
                 "type": "integer",
                 "format": "int32"
            }
        }
    }
}


@webron @fehguy any thought about this?


  1. in 3.0 there is no currently no support for position field (annotations/model/resolver/spec); is it planned, or should we remove refs in tests, etc? any thought about this?

  1. reorganized/updated tests in swagger-core module; this includes fix/re-enable/refactor/remove what currently disabled, and reorganizing packaging

one note:

  • A. CompositionTest (see e.g. original disabled version -> new version) has been adapted to match what described above, therefore replacing AnyOf with JsonSubTypes (I guess possibly AnyOf was a leftover..)

@fehguy please have a look


  1. in 2.0 we had a field reference of @ApiModel which seemed to be resolved by replacing the String return type with a ref to the type set as value of reference (basically the string value); this is not present in 3.0, not sure what was the original use case; is it ok not to consider and remove related tests?

see e.g.
https://github.com/swagger-api/swagger-core/blob/master/modules/swagger-core/src/test/java/io/swagger/models/ModelWithReference.java
https://github.com/swagger-api/swagger-core/blob/master/modules/swagger-core/src/test/java/io/swagger/ModelWithReferenceTest.java


  1. there is no current support in annotation for extensions (while they are supported in models); should we add extensions to annotations similar to 2.0?

  1. major missing/todo:
  • A finalize tests refactor
  • B. finalize compound objects
  • C. extensions support
  • D. security full support (deserialize/resolve..)
  • E. cleanup/fix pom commented out code
  • F. cleanup/fix commented code in AbstractModelConverter.java
  • G. implement/refactor tests related to consume, produce and security
  • H. cleanup obsolete tests (e.g. @ApiModel(reference..) etc)
  • I. implement/fix MapProperty support currently commented out
  • J. cleanup
  • K. SpecFilter related area
  • L. bootstrapping
  • M. (rename all "swagger" to "openapi")
  • N. (rename all #/definitions path in tests to #/components/schemas)
  • O. implement 3.0 version of samples
@fehguy
Copy link
Contributor

fehguy commented Jul 14, 2017

Point 1

  • Consider renaming io.swagger.oas.media.DiscriminatorMapping to Discriminator. @webron ???

Point 2

  • Jackson annotations can be substituted with @Schema annotations, correct? Meaning, they're not required.

Point 3-- yes that makes sense.

Point 4

  • I believe all models should be resolved as $refs whenever possible. This gives the best possible chance for a good codegen output

Point 5

  • Yes, the example you gave is technically correct, and is "reversible" where the 2.0 solution was not. We run the risk of overlapping types if parent & child have incompatible definitions, but that's a design issue.

Point 6

  • We should remove position. It was never in the spec, and was added only to make tests pass. The switch to use LinkedHashMap has helped with this

Point 7--that's just fine

Point 8

  • Whoa I never saw that (or I don't remember). I don't think that serves any value.

Point 9

  • Yes, that would be good. I think the missing extensions is just an oversight

Point 10

  • Add "send a cupcake to @webron ) to your list.

Looks good, very complete. Thank you!

@frantuma
Copy link
Member Author

@fehguy thanks, one thing about point 2 (Jackson @jsonsubtypes annotations):

you say:

Jackson annotations can be substituted with @Schema annotations, correct? Meaning, they're not required.

we don't have currently @Schema equivalent to @jsonSubTypes, (as we had in 2.0 @apimodel), I would add subtypes field to @Schema annotation, is that right?

@fehguy
Copy link
Contributor

fehguy commented Jul 17, 2017

Hmm I'm not exactly sure. Arent the subtypes only needed for oneOf or anyOf ? If so I don't think it should be added to '@schrma'

@frantuma
Copy link
Member Author

@fehguy sorry missed your comment, subtypes are needed for any composition, mainly because when resolving starting from a "parent" (a class which is extended and/or used as "parent" in anyOf, allOf, oneOf annotations) we have otherwise no way to know we should resolve "children"; this was true also in 2.0, where either jackson JsonSubTypes or swagger annotation subtypes field were allowed and used

@frantuma
Copy link
Member Author

frantuma commented Jul 19, 2017

updated open questions and todo (includes areas spotted in swagger-jaxrs):

open questions

  1. point 5. above and related comments
  2. in 2.0 a @ApiImplicitParam exists, which would allow to specify a param independently; this does not exisist in 3.0, is that ok? should related tests be removed?

todo core

  1. implement @Schema "subtypes" similar to 2.0, for point 5 above

  2. Add deep clone to OAS 3.0 models #2227 clone implementation

  3. PojoTests complete last tests

  4. implement/fix MapProperty support currently commented out

  5. cleanup/fix commented code in AbstractModelConverter.java

  6. security full support (deserialize/resolve..)

  7. implement/refactor tests related to consume, produce and security

  8. ModelResolver "minor" TODOs (marked v3 swagger-core module notes, major todo and open questions #2312)

  9. ("Ref" schema and property resolving and serializing)

  10. examples and string array resolving (see commented out PojoTests)

  11. requiredProperties schema annotation field support

  12. overriding full support

  • Schema annotation type + format full support
  • Schema annotation implementation field full support

todo jaxrs

A. SpecFilter (includes replacing ReaderListener with filter run before and after)
B. Jaxrs-reader currently not processing subresources
C. Jaxrs-reader currently not processing extensions
D. complete ParameterProcessor implementation (see TODO)
E. have content field of Response annotation as array, and of Parameter as object, according to spec
F. @Schema type attribute is ignored in the Reader atm, so no way to specify type of list for responses/parameters and requestBody. if it is specified like @Schema(type="array", implementation=Airline.class) it doesn't work
G. OperationParser doesn't handle correctly string and primitive types in implementation field, at least for response

todo all

I. bootstrapping
II. implement 3.0 version of samples (at least one as proof of concept)
III. (rename all "swagger" to "openapi")
IV. (rename all #/definitions path in tests to #/components/schemas)
V. cleanup

frantuma added a commit that referenced this issue Jul 24, 2017
ref #2312 - enhanced 3.0 serialization, deserialization and resolving
frantuma added a commit that referenced this issue Aug 9, 2017
swagger integration and #2312 improvements and fixes
frantuma added a commit that referenced this issue Aug 9, 2017
ref #2312 - requiredProperties and `not` support
frantuma added a commit that referenced this issue Aug 11, 2017
@frantuma
Copy link
Member Author

frantuma commented Aug 11, 2017

First Release candidate's release notes draft, summarizing remaining TODOs of the list in comments above:

Release notes

swagger-core v2.0.0-rc0 Release candidate!

The Swagger team is proud to announce the rc0 release candidate of our main java library swagger-core. This release candidate provides official initial support for OpenApi 3.0’s main features. The missing features will be added in the next release candidates until the final release is available.

In order to better improve these libraries and tools, we ask that you start using them and provide us with feedback in form of issues (please use label 3.0 spec support) on the github repository. The issues are a huge help in finding what’s missing or not working properly (though note some issues are already submitted). We would also appreciate any PRs that improve existing or new tickets.

Be aware that the release notes contain the notable changes, but there may some changes that we missed in the list. The same applies for the Limitations section.

Notable Features:

  • First official release candidate of OpenApi 3.0 support. swagger-core now produces OpenAPI 3.0 specs only. swagger-core 2.0 version is not backward compatible with previous 1.x versions.
  • Available on Maven central, and the sources are in the 2.0 branch. PRs should be submitted against the 2.0 branch.
  • Swagger JAX-RS 2 support
  • Java 8
  • Consistent integration mechanism

Limitations

  • Resolve resource operations also when not annotated with @Operation (swagger-jaxrs2)
  • Implement subtypes field in Schema annotation, with related resolver processing (swagger-annotations / core)
  • Overriding full support (swagger-core)
    • Schema annotation type + format full support (swagger-core)
    • Schema annotation implementation field full support (swagger-core)
  • Filter / Reader listener support (swagger-jaxrs2)
  • Reader sub-resources support (swagger-jaxrs2)
  • Reader extensions full support (swagger-jaxrs2)
  • Annotations javadocs (swagger-annotations)
  • Annotations default values enhancement (swagger-annotations / core)
  • Integration layer enhancements:config file location enhanced loading, additional loaders, etc. (swagger-integration, swagger-jaxrs)
  • Consumes/Produces full support (swagger-jaxrs2)
  • JsonIdentity support (swagger-core)
  • MatrixParam annotation support

@frantuma
Copy link
Member Author

frantuma commented Oct 6, 2017

closing as implemented or replaced by other tickets

@frantuma frantuma closed this as completed Oct 6, 2017
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

3 participants