search

eclipse / microprofile-open-api

Microprofile open api
Apache License 2.0
131 stars 81 forks source link

Support Schema `additionalProperties` via annotation #512

Closed MikeEdgar closed 2 years ago

MikeEdgar commented 2 years ago

Fixes #423

MikeEdgar commented 2 years ago

Let's discuss this on the next call. I took a slightly different approach here than last described in #423. By adding support for two special cases of a Schema (rendered as true or false) this simplifies the specification of additionalProperties and also can be leveraged in the future with OpenAPI version 3.1.0+ which use JSON Schema directly. In JSON schema, any schema can be specified as a boolean to match everything (true) or nothing (false).

Reference: https://datatracker.ietf.org/doc/html/draft-bhutton-json-schema-00#section-4.3.2

Azquelt commented 2 years ago

Hi, sorry it's taken me a while to get back to this. I've had another think about it.

  1. I'd forgotten how severe the restrictions on annotations are - particularly that you can't have an annotation refer to itself. With that in mind, it makes much more sense that we use the Class<?> approach to reference nested schemas. It's not ideal as we may end up needing to make dummy classes just to get our schema looking right, but there's not much we can do about it within the limits imposed by Java's annotations.

  2. I noticed that in the spec, additionalProperties is the only property within a Schema object that's allowed to be either true, false or another Schema object. This is reflected in our model as we have both setAdditionalPropertiesSchema and setAdditionalPropertiesBoolean.

    With this in mind, I think that TrueSchema and FalseSchema should not extend Schema and should not be in the models package. The spec doesn't allow true and false to be used as a schema in any place other than under additionalProperties and making TrueSchema a subtype of Schema gives the impression that it can be used anywhere in place of a Schema.

  3. I wonder if we need TrueSchema or FalseSchema at all. Would it make sense to use Object to represent true (any object is valid in an additional property) and Void to represent false (no object types are valid as additional properties)?

  4. Given the special nature of additionalProperties in the OpenAPI spec and the way it's reflected in our model, would it also make sense to have a TCK to check that the model presented to a filter is correct when additionalProperties is set to a boolean value with an annotation?

Azquelt commented 2 years ago

I noticed that in the spec, additionalProperties is the only property within a Schema object that's allowed to be either true, false or another Schema object.

On a second look, it seems like this is the case for OAS 3.0, but not for OAS 3.1?

MikeEdgar commented 2 years ago

I noticed that in the spec, additionalProperties is the only property within a Schema object that's allowed to be either true, false or another Schema object.

On a second look, it seems like this is the case for OAS 3.0, but not for OAS 3.1?

Exactly, that's what made me think it would be good to take the approach of providing TrueSchema and FalseSchema to enable a smooth transition when OAS 3.1 support is added. The caveat is that it is only supported prior to OAS 3.1 in the way used in this PR. A more general usage should probably require that implementations provide singleton/immutable instances via the OASFactory, if not instances defined in the spec/API directly.

Azquelt commented 2 years ago

Exactly, that's what made me think it would be good to take the approach of providing TrueSchema and FalseSchema to enable a smooth transition when OAS 3.1 support is added.

I can see that making sense, but I'm not sure that this is the way we'd want to represent it. Having separate classes for true and false in the model seems really odd (even if we need to have separate classes for the annotations). In the model, I'd expect maybe separate BooleanSchema and ObjectSchema interfaces with a Schema super-interface, or possibly something more like the JsonValue and JsonObject interfaces in JSON-P.

I also still don't like using model classes within annotations. If we later decided to change or remove TrueSchema and FalseSchema, we'd end up affecting both the annotation classes and the model classes and I don't see what we would gain by using the same class in both the model and the annotations. I'd still really prefer it if these were separate classes in the annotations package that didn't extend Schema, even if we were to later introduce similar classes in the model package when we add support for OpenAPI 3.1 in a future version.

I'm fairly sure OpenAPI 3.1 will cause breaking changes in our model, so I'm not too concerned about having forward-looking support for representing true and false schemas in the model now. I'd rather be free to design that API when we're looking at supporting everything in 3.1 and can make a breaking change if that seems like the best way forward.

MikeEdgar commented 2 years ago

I also still don't like using model classes within annotations. If we later decided to change or remove TrueSchema and FalseSchema, we'd end up affecting both the annotation classes and the model classes and I don't see what we would gain by using the same class in both the model and the annotations. I'd still really prefer it if these were separate classes in the annotations package that didn't extend Schema, even if we were to later introduce similar classes in the model package when we add support for OpenAPI 3.1 in a future version.

You've convinced me :) I'll update it to keep the changes local to the annotations package.

MikeEdgar commented 2 years ago

@Azquelt I think I've addressed all the comments. This has been squashed/rebased as well.