OAS 3.1 Schema model updates #598
Conversation
api/src/main/java/org/eclipse/microprofile/openapi/models/media/Schema.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/eclipse/microprofile/openapi/models/media/Schema.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/eclipse/microprofile/openapi/models/media/Schema.java
Show resolved
Hide resolved
| * the boolean value of this schema, or {@code null} if it is not a boolean schema | ||
| * @since 4.0 | ||
| */ | ||
| void setBooleanSchema(Boolean booleanSchema); |
There was a problem hiding this comment.
Implementation note: having setBooleanSchema made it near-impossible to implement Schema as a thin wrapper around a Jackson representation of some JSON.
All the other setters merely change properties on the root object, but to implement this setter you have to change the root object itself. In Jackson, this means setting a new object on the parent and Jackson also doesn't allow you to traverse up a JSON tree (parents know about their children, but children don't know about their parents).
The other way of implementing this would be to have factory methods for an immutable boolean schema on OASFactory, but that would be very different than the way the rest of the MP OpenAPI model is designed.
There were other issues with implementing it as a wrapper around Jackson, e.g. calling code may assume that two calls to getAllOf() will return the same Schema instances, so I gave up on that approach.
api/src/main/java/org/eclipse/microprofile/openapi/models/media/Schema.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/eclipse/microprofile/openapi/models/media/Schema.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/eclipse/microprofile/openapi/models/media/Schema.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/eclipse/microprofile/openapi/models/media/Schema.java
Outdated
Show resolved
Hide resolved
api/src/main/java/org/eclipse/microprofile/openapi/models/media/Schema.java
Show resolved
Hide resolved
api/src/main/java/org/eclipse/microprofile/openapi/models/media/Schema.java
Show resolved
Hide resolved
api/src/main/java/org/eclipse/microprofile/openapi/models/media/Schema.java
Outdated
Show resolved
Hide resolved
| * <li>{@code String} | ||
| * <li>{@code BigDecimal} | ||
| * <li>{@code BigInteger} | ||
| * <li>Any type which {@link OASFactory} can create |
There was a problem hiding this comment.
Noted that in the smallrye implementation, this would require quite a bit of refactoring since the class handling IO for each model type needs to have the IO class for every other model type passed in.
I still think it's a reasonable requirement, but would be open to changing it.
There was a problem hiding this comment.
Is this because of on-the-fly serialization in the smallrye PR?
There was a problem hiding this comment.
At the moment, there's a separate IO class for each model class. When a model object has child model objects, the IO class needs a reference for the IO classes for those child object types. This is either passed into or created in the constructor.
This change means the SchemaIO class needs a reference to every other IO object, which makes for a rather large constructor. We also probably can't have it create a new instance in the constructor since we need to avoid creating a loop. I would propose moving the IO class references to IOContext and having every IO class retrieve them from there.
That's an irritating amount of refactoring though so I would like to make sure you think that's ok before I go ahead and do it.
There was a problem hiding this comment.
Ok, I see your point now. We can discuss that in the other PR.
|
I've pushed commits to address the comments above and to add tests for reading and passing through a custom schema dialect via a model reader and a static document. I think these are all the tests needed for the model parts of #584, so if these are approved I will squash the review change commits and merge this PR. |
- if/then/else - $dialect - comment - depedentSchemas - prefixItems - contains - patternProperties - propertyNames - unevaluatedItems - unevaluatedProperties - const - maxContains - minContains - dependentRequired - contentEncoding - contentMediaType - contentSchema Co-authored-by: Michael Edgar <[email protected]>
Signifies that this schema represents a single boolean value, rather than a schema object.
Now that OpenAPI allows boolean schemas in general, we don't need a special case for additionalProperties.
These properties are now valid on all schemas.
Example is still a valid keyword in OpenAPI 3.1, though it is deprecated in favor of examples.
Change exclusiveMinimum and exclusiveMaximum from boolean to BigDecimal. Also update the javadoc for minimum and maximum to make the difference in meaning between the properties more clear.
Nullable has been removed, type is now permitted to be an array of permitted types and may include null. Attempt to keep the old methods as far as they don't clash with the new ones and can still be defined in a way that makes sense.
Example changed to examples and now must be an array.
Schema.type can now be either a string or an array of strings
exclusiveMinimum and exclusiveMaximum are now standalone numeric properties, rather than boolean properties interpreted in conjunction with minimum and maximum.
The exception type and stack-trace is important here.
Tests for Schema.getAdditionalPropertiesBoolean and getAdditionalPropertiesSchema.
This avoids conflicts between mutually exclusive properties now present in Schema. If setBooleanSchema is called with a non-null value, other methods may fail.
These constructors are deprecated and cause problems when the model tests call assertSameInstance.
Test that example and examples do not affect each other.
Test building a schema with a custom dialect in a model reader and ensure it is serialized correctly.
For #584