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

It is not possible to create Schema with array of referenced items #425

Open
KrystianRos opened this issue May 5, 2020 · 25 comments
Open

Comments

@KrystianRos
Copy link

Versions: 1.0.1, 1.1.2, 2.0-MR1
I'm using the following annotation for schema:
@Schema(required = true, description = "Some description...", type = SchemaType.ARRAY, ref = "name1")
What I'm getting is:

variableName:
          $ref: '#/components/schemas/name1'

What I expect to get:

variableName:
          type: array
          description: Some description...
          items:
                    $ref: '#/components/schemas/name1'

It looks like ref is overriding all of data in @Schema object. I have tried a lot of modifications for mentioned annotation, but only pseudo-success is when I have add an 'implementation' element pointing to class- but that's not satisfying solution.
I'm sorry for not pasting original content, but it's protected by contract.

@EricWittmann
Copy link
Contributor

Yes that is the contract for the ref property. That property is mutually exclusive with all other properties on @Schema.

@EricWittmann
Copy link
Contributor

One thing you could try is creating a dummy class/interface like this:

interface NamedList extends List<Named>

Then you can use that as the implementation class for the annotation.

@EricWittmann
Copy link
Contributor

@MikeEdgar @arthurdm any thoughts on other workarounds? I feel like I'm missing an obvious approach, but I can't think of anything.

@MikeEdgar
Copy link
Member

What about the following where NamedBean is the class containing the schema you previously referenced as "name1" If you wanted to ensure the $ref is a particular value, you should annotate the NamedBean class with @Schema(name = "name1") as well.

@Schema(
    required = true,
    description = "Some description...", 
    type = SchemaType.ARRAY,
    implementation = NamedBean.class)

@KrystianRos
Copy link
Author

Hi @MikeEdgar, when I'm using 'implementation' in combination with SchemaType.ARRAY, then in result final document I'm getting a full description of schemas which are over all fields inside NamedBean, not $ref field (if I understand correctly. what you mean).

@MikeEdgar
Copy link
Member

@KrystianRos - did you try @EricWittmann's approach of using an interface?

@0xqab
Copy link

0xqab commented May 28, 2020

Sitting on the same boat as @KrystianRos.
I tried the approach with the interface, but at least on Payara Micro the NamedList interface is not listed in the components sections, hence the "$ref": "#/components/schemas/NamedList" points nowhere. I assume I have to create a class instead, which is really ugly because of the amount of functions which have to be implemented. And it still won't provide me a type: array definition.
Also, the approach with NamedList is not feasable for a huge API with lot's of lists.

@MikeEdgar
Copy link
Member

@KrystianRos, @0xqab - I would suggest you try either

(1) implementing an OASModelReader and having it produce the correct array Schema in components which can then be referenced by name using ref in @Schema annotations throughout your code or
(2) Specify the same schema in components in a static YAML or JSON file which can also be referenced with ref.

@0xqab mentioned the use of Payara, which implementation are you using @KrystianRos ? I expect the type = ARRAY with implementation = MyClass.class to work with runtimes using SmallRye.

Azquelt pushed a commit to Azquelt/microprofile-open-api that referenced this issue Mar 17, 2022
…lrye-smallrye-parent-23

Bump smallrye-parent from 22 to 23
@MikeEdgar
Copy link
Member

This issue was discussed in the 2023-10-02 meetup call. One proposed solution would be to allow for the @Schema annotation to be placed on TYPE_USE elements. This would split the items schema from the array schema into separate uses of the annotation.

With that enhancement, developers could write something like the code below and get the expected result.

@Schema(required = true, description = "Some description...")
List<@Schema(ref = "name") MyNameBean> variableName;
variableName:
  type: array
  description: Some description...
  items:
    $ref: '#/components/schemas/name1'

cc: @tjquinno @Azquelt

@Azquelt
Copy link
Member

Azquelt commented Oct 9, 2023

We discussed this a little more on the 2023-10-09 meetup call.

For reference, this should currently work, as long as you want to reference the schema generated for MyNameBean:

@Schema(required = true, description = "something")
List<MyNameBean> variableName;

Whether the schema for MyNameBean is put inline inside items or referenced as a component is up to the implementation.

Here are the options we came up with for allowing a different schema to be referenced:

  1. Allowing @Schema to target TYPE_USE:

    @Schema(required = true, description = "Some description...")
    List<@Schema(ref = "name") MyNameBean> variableName;

    This is nice because it's consistent with the way we currently allow @Schema on a class declaration and also on a field where that class is used.
    It would, however, allow @Schema to be used in lots of places where it's not relevant

  2. Add an itemRef property to the annotation (only valid when type=ARRAY):

    @Schema(required = true, description = "Some description...", itemRef = "name")
    List<MyNameBean> variableName;

    This is nice because it's simple, but it's inconsistent with the way we specify oneOf and allOf.

  3. Do something similar to oneOf and allOf:

    @Schema(required = true, description = "Some description...", item = A.class)
    List<MyNameBean> variableName;

@MikeEdgar noted that option 1 would also let you specify the schema for objects in a map like this:

Map<String, @Schema(oneOf={A.class, B.class}) Object> variableName;

Currently you can do this with additionalProperties, but you need an extra class annotated with @Schema(oneOf={A.class, B.class}) because additionalProperties takes a class.

@Azquelt
Copy link
Member

Azquelt commented Oct 16, 2023

We noted a few shortcomings for Option 1:

  • If the field is an array, I don't think there's a place to put the annotation for the item schema:
    @Schema(required = true, description = "Some description...")
    MyNameBean[] variableName;
    
  • If the array maps to a single Java object, rather than a collection type, there's also no place to put the item schema:
    @Schema(required = true, description = "Some description...", type = ARRAY)
    MyNameBeansHolder variableName;
    
  • If the schema is declared within @Components(schemas = { ... }), there's no way to declare the item schema
    • I guess there's not really a good way around this. The current solution for allOf requires another object anyway, it can't encompass the schemas within the @Components annotation.

In these three scenarios, Option 3 would provide a way to declare the item schema, at the expense of requiring an extra object to host the @Schema annotation.

I guess it depends how often we expect this to come up. If this is fairly rare, then I think it's more important to do something that covers every case, even if it's a bit clunky - it's better to have something clunky than to have to say "sorry, you can't do that". If we expect it to come up fairly often, then it may be worth considering doing both since I do think Option 1 is nicer (at least, when you don't have very much to put in the @Schema annotation).

@MikeEdgar
Copy link
Member

  • If the field is an array, I don't think there's a place to put the annotation for the item schema:
    @Schema(required = true, description = "Some description...") MyNameBean[] variableName;

It looks odd, but this is actually possible:

MyNameBean @Schema(required = true, description = "Some description...")[] variableName;
  • If the array maps to a single Java object, rather than a collection type, there's also no place to put the item schema

I agree, this would not be supported without some other class to reference, either using implementation = ItemBean[].class or a new items = ItemBean.class. The first one using an array for implementation still has all the limitations of this issue, so it's not really an option for many cases.

  • If the schema is declared within @Components(schemas = { ... }), there's no way to declare the item schema

Agreed, possibly this placement of the annotation is the strongest argument for adding Class<?> items() to @Schema. Even though there would technically be two ways to describe array items, I still think TYPE_USE provides greater functionality beyond just arrays - e.g. additionalProperties and certainly others we're not thinking about.

@MikeEdgar
Copy link
Member

Annotating the array type as suggested above may not actually be what is wanted. I need to look further into how the annotated is represented.

@MikeEdgar
Copy link
Member

It appears that when annotating with TYPE_USE, the annotation directly on the array element type is the one that would be considered the items schema. The same annotation also appears on the field in the Java class, so in cases like this the field annotation would need to be discarded by a scanner implementation. The annotation on the array type [] would then be the outer schema and should support arbitrary levels of nesting.

It's probably not what people are used to seeing since it would be reversed from the typical way of annotating fields, parameters, etc. However, it technically would allow for the schema to be expressed as needed when using arrays.

@Schema(description = "Item description") String 
  @Schema(description = "Array description")[] variableName;

Resulting schema:

"variableName": {
  "type": "array",
  "description": "Array description",
  "items": {
    "type": "string",
    "description": "Item description"
  }
}

@Azquelt
Copy link
Member

Azquelt commented Oct 23, 2023

We would want to make sure that we don't change the meaning of this:

@Schema(description = "Item/array description?") String[] variableName;

Today, this would refer to the field type (i.e. the array) whereas in the above example, it refers to the item type.

@MikeEdgar also noted that there could be similar confusion over whether a method or its return type is the target of an annotation.

@Azquelt
Copy link
Member

Azquelt commented Oct 23, 2023

From the call: we could look at how bean validation (or any other specs that have type_use annotations) handles type annotations on array fields like this.

@mklueh
Copy link

mklueh commented Nov 4, 2023

I was stumbling over this issue as well, trying to integrate OpenAPI in my application, and use oneOf / anyOf together with type ARRAY in my response object in the most (in my opinion) logical and intuitive way:

@Data
@Builder
@AllArgsConstructor
public class SomeResponseObject {

    private long page;

    private long pages;

    @Schema(type = ARRAY, oneOf = {SomeItem.class, SomeOtherItem.class})
    private List<BaseClass> items;
}

This however will lead to

 "SomeResponseObject" : {
        "type" : "object",
        "properties" : {
          "page" : {
            "format" : "int64",
            "type" : "integer"
          },
          "pages" : {
            "format" : "int64",
            "type" : "integer"
          },
          "items" : {
            "type" : "array",
            "items" : {
              "$ref" : "#/components/schemas/BaseClass"
            },
             "oneOf" : [ {
              "$ref" : "#/components/schemas/SomeItem"
            }, {
              "$ref" : "#/components/schemas/SomeOtherItem"
            } ]
          }
        }
      },

which generates his typescript object

    SomeResponseObject: {
      /** Format: int64 */
      page?: number;
      /** Format: int64 */
      pages?: number;
      items?: components["schemas"]["SomeItem"] | components["schemas"]["SomeOtherItem"];
    };

without any array type.

This is the current vs the expected result discussed here openapi-ts/openapi-typescript#1421 (comment):

image

While it does not do what I expected, I'm wondering what use case the actual result has and why it can't do what it should instead technically.

I mean, when do I want to annotate a list with type array and anyOf / oneOf and really want something different as a result than specifying a type array that is one of the n types?

Edit:

I'm wondering if it would be possible to just do something like this to solve the issue or if there is any technical drawback / misunderstanding from my side, to just take anyOf etc into account if provided:

image

@Azquelt
Copy link
Member

Azquelt commented Nov 13, 2023

@mklueh Thank you for your detailed breakdown of the problems you've had trying to the types right for an array. I'm sorry I've been really busy and still haven't had time to sit down and go through what you've written but I hope to get to it later this week.

@Azquelt
Copy link
Member

Azquelt commented Nov 21, 2023

@mklueh's suggestion amounts to saying that when @Schema is placed on an array element, some of the properties should apply to the array itself, while others should apply to the array items.

I'm not sure this is a good idea because while there are some fields that might be unambiguous, there are others which are not. In particular:

  • type has to refer to the array itself, in the example we have type=ARRAY, but defining the type of the item is also something you may want to do
  • nullable could refer to the array itself being null, or to allowing null as an item in the array
  • extensions could refer to the array itself, or to the type of the items
  • all of the documentation fields (title, description, example, externalDocs) could refer to either type
  • I would argue that allOf, oneOf and anyOf could be valid for the array itself - you could want to say "this property can be either an array or a string"

However, I note that this is also a problem we have for objects which we solved with the properties field, which takes an array of @PropertySchema (which itself has most of the same fields as @Schema).

Would it make sense to have an items or itemSchema field which holds a @PropertySchema (or another similar annotation)?

For @mklueh's example, you could have

    @Schema(type = ARRAY, items = @PropertySchema(oneOf = {BaseClass.class, SomeItem.class, SomeOtherItem.class}))
    private List<BaseClass> items;

For the original example where you want to refer to a component schema for the item type you could have

    @Schema(required = true, description = "Some description...", items = @PropertySchema(ref="name))
    List<MyNameBean> variableName;

As-is, I don't think we can quite just use @PropertySchema because the name field is currently required. We could either add a default value for name() (which I think would be backwards compatible), or we could add a new similar annotation @ItemSchema.

@mklueh
Copy link

mklueh commented Nov 21, 2023

@Azquelt thanks for your detailed response.

@mklueh's suggestion amounts to saying that when @Schema is placed on an array element, some of the properties should apply to the array itself, while others should apply to the array items.

Now I see the problem. The annotation would always refer to the types of the field it is annotating.

What if Microprofile would automatically declare any inheritors of the used base type of the collection as allOf by default? Then no one would even have to bother with this case, except for more restrictive edge cases. (ex. only specific child classes should be allowed)

I'm not sure this is a good idea because while there are some fields that might be unambiguous, there are others which are not. In particular:

  • type has to refer to the array itself, in the example we have type=ARRAY, but defining the type of the item is also something you may want to do

In case you want to refer to the item, wouldn't you then place the annotation on top of the item's class instead?

  • I would argue that allOf, oneOf and anyOf could be valid for the array itself - you could want to say "this property can be either an array or a string"

Just to make sure I understand you correctly: Does that mean you could declare a field:

@Schema(type = ARRAY, oneOf = { String.class})
private Collection<BaseClass> items;

to tell Microprofile the Collection type should be seen as a String?
If so, what sense does it make or would this only be a valid case if the field would be of type Object?

And why is the type = Array property even needed? Wouldn't it be possible to determine the type automatically if the field's type is Collection or Array?

However, I note that this is also a problem we have for objects which we solved with the properties field, which takes an array of @PropertySchema (which itself has most of the same fields as @Schema).

Would it make sense to have an items or itemSchema field that holds a @PropertySchema (or another similar annotation)?

For @mklueh's example, you could have

    @Schema(type = ARRAY, items = @PropertySchema(oneOf = {BaseClass.class, SomeItem.class, SomeOtherItem.class}))
    private List<BaseClass> items;

That would also be nice. Or the property should be named

     @Schema(type = ARRAY,  itemSchema = @ItemSchema(oneOf = {BaseClass.class, SomeItem.class, SomeOtherItem.class}))
     private List<BaseClass> items;

to make it a bit clearer

For the original example where you want to refer to a component schema for the item type you could have

    @Schema(required = true, description = "Some description...", items = @PropertySchema(ref="name))
    List<MyNameBean> variableName;

As-is, I don't think we can quite just use @PropertySchema because the name field is currently required. We could either add a default value for name() (which I think would be backward compatible), or we could add a new similar annotation @ItemSchema.

Also nice, but not sure which of both variants would be more straightforward from a user's perspective...I'd generally advocate against yet another annotation, but thinking about it, maybe @ItemSchema would be even clearer considering that @Schema is only for the field type.

@FieldSchema and @ItemSchema would be perfectly distinguishable but would break backward compatibility ofc.

@MikeEdgar
Copy link
Member

I agree that using @PropertySchema with items seems to be a reasonable solution here. Not as powerful as allowing TYPE_USE, but more readable and probably easier to understand.

Adding a default for name() should be backward compatible, but we'll probably want to add something to the TCK to require a non-blank value when @PropertySchema is being used in properties. I think I prefer reusing that annotation to adding @ItemSchema that is otherwise identical.

@Azquelt
Copy link
Member

Azquelt commented Nov 27, 2023

On the call we broadly agreed that reusing @PropertySchema is preferable to adding another nearly identical annotation, even if the naming is not ideal.

I also note that this would provide another way to do #445 if you can't annotate the item class.

@mklueh
Copy link

mklueh commented Nov 30, 2023

Great! Is there a rough timeframe for when a solution might be released?

@Azquelt @MikeEdgar Considering this issue is already 3.5 years old, are those types of changes rather slow to push forward in this project, because of standardization and compatibility issues or is there no real hurdle?

@Azquelt
Copy link
Member

Azquelt commented Dec 11, 2023

The things that need to be done to get this in the next release are:

  • come up with a good design (I think we have this now 😄)
  • create a PR with changes to the API or spec text as needed
  • create a PR to add TCK tests

The next release is likely to be some time next year, around the next major MicroProfile release. Additionally, before we can do a release, we also need someone to have implemented it and passed the TCK (both so that people can actually use it, and to ensure we haven't put anything in the spec that's actually impossible to implement).

The main hurdle is that most of us are tied up working on other projects so there hasn't been a lot of work on MP OpenAPI recently. If you'd like to propose a PR to make the API changes and add TCKs for this feature that would be very welcome, otherwise one of us will get to it eventually.

@mklueh
Copy link

mklueh commented Dec 14, 2023

@Azquelt thanks :)

One thing confuses me when looking at the TCK

image

It seems the implementation property applies to the item type of the list while the oneOf, anyOf, allOf properties apply to the list type.

Wouldn't it be better to deprecate implementation in favor of using @Schema(type= Array, items = { @PropertySchema( oneOf=Flight.class ) }) to get some more consistency?

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

No branches or pull requests

6 participants