OAS 3.1 changes for schema model #1793

Azquelt · 2024-04-09T11:06:48Z

Implements the spec changes in microprofile/microprofile-open-api#598

Azquelt · 2024-04-09T11:12:36Z

Build failures are expected because this depends on spec changes.

Locally I have tests passing up to this point:

SmallRye: OpenAPI Test Data ........................ FAILURE [ 10.751 s]

With the failure:

[ERROR] 	[error]: Build step io.quarkus.smallrye.openapi.deployment.SmallRyeOpenApiProcessor#build threw an exception: java.lang.NoSuchFieldError: org/eclipse/microprofile/openapi/models/media/Schema$SchemaType.NULL
[ERROR] 	at io.smallrye.openapi.api.models.media.SchemaImpl.setNullable(SchemaImpl.java:330)
[ERROR] 	at io.smallrye.openapi.runtime.io.schema.SchemaFactory.readSchema(SchemaFactory.java:188)
[ERROR] 	at io.smallrye.openapi.runtime.io.schema.SchemaFactory.readSchema(SchemaFactory.java:119)
[ERROR] 	at io.smallrye.openapi.runtime.io.schema.SchemaFactory.readSchema(SchemaFactory.java:94)
[ERROR] 	at io.smallrye.openapi.runtime.scanner.OpenApiDataObjectScanner.readKlass(OpenApiDataObjectScanner.java:376)
[ERROR] 	at io.smallrye.openapi.runtime.scanner.OpenApiDataObjectScanner.depthFirstGraphSearch(OpenApiDataObjectScanner.java:251)

which looks like this part of the build is running with the built smallrye-open-api, but using an older version of the spec APIs without SchemaType.NULL.

Azquelt · 2024-04-09T12:57:13Z

Found the issue and updated the dependency for the testdata project and now my local build runs cleanly.

Azquelt · 2024-04-09T15:11:16Z

core/src/main/java/io/smallrye/openapi/api/models/MapBasedModelImpl.java

+    protected <T> void addToMapProperty(String propertyName, String key, T value) {
+        if (value != null) {
+            Map<String, T> map = (Map<String, T>) data.get(propertyName);


The add and remove methods for maps and lists risk a ClassCastException if the data stored in the property is not actually a map or a list.

We may want to improve this by:

providing a better exception

avoiding an exception

for add: overwriting the value with a new list or map

for remove: doing nothing

I've implemented the second option.

Azquelt · 2024-04-09T16:07:29Z

core/src/main/java/io/smallrye/openapi/api/models/media/SchemaImpl.java

    public static SchemaImpl copyOf(Schema other) {
-        SchemaImpl clone = (SchemaImpl) MergeUtil.mergeObjects(new SchemaImpl(), other);
-        clone.required = copy(clone.required, () -> new ArrayList<>(clone.required));
-        clone.enumeration = copy(clone.enumeration, () -> new ArrayList<>(clone.enumeration));
-        clone.items = copy(clone.items, () -> copyOf(clone.items));
-
-        clone.allOf = copy(clone.allOf, () -> clone.allOf
-                .stream()
-                .map(SchemaImpl::copyOf)
-                .collect(Collectors.toList()));
-
-        clone.properties = copy(clone.properties, () -> {
-            Map<String, Schema> copiedProperties = new LinkedHashMap<>(clone.properties.size());
-            clone.properties.forEach((k, v) -> copiedProperties.put(k, copyOf(v)));
-            return copiedProperties;
-        });
-
-        clone.additionalPropertiesSchema = copy(clone.additionalPropertiesSchema,
-                () -> copyOf(clone.additionalPropertiesSchema));
-
-        clone.xml = copy(clone.xml, () -> MergeUtil.mergeObjects(new XMLImpl(), clone.xml));
-        clone.externalDocs = copy(clone.externalDocs,
-                () -> MergeUtil.mergeObjects(new ExternalDocumentationImpl(), clone.externalDocs));
-
-        clone.oneOf = copy(clone.oneOf, () -> clone.oneOf
-                .stream()
-                .map(SchemaImpl::copyOf)
-                .collect(Collectors.toList()));
+        if (other == null) {
+            return new SchemaImpl();
+        }


The logic of copyOf is very similar to MapBasedModelImpl.mergeFrom, we may be able to remove one of them.

It's slightly different. When using mergeFrom, if from has a particular property but to does not, the value from from is used directly. When the method returns, both from and to reference the same object for that property. This matches the behaviour of MergeUtil.mergeObjects and means mergeFrom is only really suitable if you're going to throw both of the source objects away and just use the result object.

For this reason I recommend keeping both, even though the logic is similar.

MikeEdgar · 2024-04-10T11:03:54Z

@Azquelt I've updated the target branch to main-4.0

MikeEdgar · 2024-04-23T18:23:44Z

@Azquelt , the target branch did not have CI enabled, so I've added it. We should get the test jobs running against the SNAPSHOT MP OpenAPI 4.0 build on your next rebase/push.

Azquelt · 2024-04-25T14:45:15Z

I've updated the serialization code so that all the "xxxIO" objects are created and owned by IOContext and implemented the serialization of any model object as a child of a schema.

Unfortunately I found some mistakes in the tests in this area which are fixed by microprofile/microprofile-open-api#602. The code in this PR should pass all tests once that PR is merged.

Azquelt · 2024-04-30T16:05:26Z

.../src/main/java/io/smallrye/openapi/runtime/scanner/dataobject/AnnotationTargetProcessor.java

-            Schema::getExtensions,
+            //            Schema::getExtensions,


Should this be reverted?

I think it should be, but uncommenting this line alone causes test failures in the TCK because getExtensions() doesn't currently behave like other getter methods. In particular it will never return null and the code that uses this list of getters is explicitly checking for whether they return null.

I've fixed this with e04261e. I had to update the extensions methods on SchemaImpl to behave like other getter/setter sets and differentiate between being null and being empty. ModelConstructionTest.processExtensible from the TCK also expects this behaviour, but the exact way it was testing it wasn't tripped up by our current implementation.

Azquelt · 2024-04-30T16:39:30Z

core/src/main/java/io/smallrye/openapi/runtime/scanner/spi/AnnotationScanner.java

-                if (schema != null && schema.getNullable() == null && TypeUtil.isOptional(returnType)) {
-                    schema.setNullable(Boolean.TRUE);
+                if (schema != null && SchemaImpl.getNullable(schema) == null && TypeUtil.isOptional(returnType)) {
+                    if (schema.getType() != null) {
+                        schema.addType(Schema.SchemaType.NULL);
+                    }
+                    if (schema.getRef() != null) {
+                        Schema nullSchema = new SchemaImpl().type(singletonList(Schema.SchemaType.NULL));
+                        // Move reference to type into its own subschema
+                        Schema refSchema = new SchemaImpl().ref(schema.getRef());
+                        schema.setRef(null);
+                        if (schema.getAnyOf() == null) {
+                            schema.addAnyOf(refSchema).addAnyOf(nullSchema);
+                        } else {
+                            Schema anyOfSchema = new SchemaImpl().addAnyOf(refSchema).addAnyOf(nullSchema);
+                            schema.addAllOf(anyOfSchema);
+                        }
+                    }
                }


@MikeEdgar you were correct. The old code here did not consider that schema might have $ref set. If $ref was set, then the value for nullable set here got discarded when the schema was serialized.

It looked like the intent of the old code was to add nullable = true when a resource method returned Optional<T> so I made the new code handle this case whether or not the response schema is a reference.

MikeEdgar · 2024-05-01T11:39:07Z

@Azquelt I just fast-forwarded main-4.0 to the current main. There are conflicts, but nothing looks terrible.

Azquelt · 2024-05-01T12:23:50Z

@MikeEdgar Thanks, I'll have a look

This test took a noticable amount of time and more than 50% was string contatentation building the test file.

For consistency with other model setters.

Azquelt · 2024-05-03T10:48:46Z

@MikeEdgar I've rebased this on the updated main-4.0 branch to resolve the conflicts and I think it's ready for review

MikeEdgar

This looks good to me, as expected. I left a few comments, but nothing major.

@phillip-kruger, this is for the MP OpenAPI 4.0 spec update. Please take a pass through if you have a chance.

MikeEdgar · 2024-05-06T13:36:59Z

core/src/main/java/io/smallrye/openapi/runtime/scanner/spi/AbstractParameterProcessor.java

+                && param.getSchema().getType() != null
+                && param.getSchema().getType().contains(SchemaType.STRING)


Edge case, but I wonder if this method should only return true if the type array has "string" and size == 1. No need to change, just making a mental bookmark.

Possibly, though I note that only string instances are constrained by pattern. If pattern was set and type was ["number", "string"], any numeric value would be valid, but a string value would only be valid if it conformed to pattern.

MikeEdgar · 2024-05-06T14:00:12Z

core/src/main/java/io/smallrye/openapi/api/models/media/SchemaImpl.java

+    @Override
+    public Schema addExtension(String name, Object value) {
+        setProperty(name, value);
+        explicitSetExtensions = true;
+        return this;
+    }


The new mutator methods are missing calls to incrementModCount. I wonder if it would be better to move modCount up to MapBasedModelImpl as protected and consolidate usage of incrementModCount. Either way is fine with me.

Good spot, I've pushed up changes moving the incrementModCount() calls into the setProperty methods on SchemaImpl.

I've kept it all in SchemaImpl for the moment since we already have additional logic we do before getting and setting properties on a schema.

phillip-kruger · 2024-05-07T00:03:25Z

Let go, we can roll forward. Would this cause a breaking change in Quarkus, or is it backwards compatible ?

MikeEdgar · 2024-05-07T09:43:55Z

Let go, we can roll forward. Would this cause a breaking change in Quarkus, or is it backwards compatible ?

This and other MP OpenAPI changes are going into the main-4.0 branch for now, so we won't release that until everything is completed and we'll bump the major version at that point.

phillip-kruger · 2024-05-07T10:40:01Z

Just branch of current main into a tag for in case we need to do work on that branch.

- Change schema to hold a freeform map of objects - New getters and setters added - Type now holds an array, but is output as a single string if it only contains one item - $ref is now valid with other fields so we cannot remove other fields at serialization time. - When merging objects, some array properties should be merged, but others (e.g. examples) shouldn't. - Old behaviour of setType and setNullable is maintained, as we rely on it fairly heavily to build up schema data from multiple sources

Make a copy of a ref schema before returning it to the caller after registering a new type to prevent the caller from corrupting the registry entry. This is more likely now that $ref is valid alongside other properties. We already did a copy when a ref was retrieved from the registry, but not when it was first registered.

- Type can now be an array, but will still be output as a string if it only has one element - The schema construction for "this object, but also allow null" is different. Previously we could combine a $ref with nullable = true, but nullable has been removed and we need to instead state that null is allowed as a type. This requires us to use anyOf instead. - When generating from annotations, examples is used instead of example

Existing code relying on the old semantics of these methods was changed to use static methods on SchemaImpl which preserves the old behavior.

Make the IOContext class the creator of all IO objects. This makes it possible for the SchemaIO class to get hold of whichever IO objects it needs to serialize generic Constructible objects.

When adding or removing an item from a schema property that is expected to be a collection or map, ensure that the property actually contains a collection or map before attempting to modify it.

This means schema extensions should be maintained when a schema references another schema but has different extensions set. To make this work, we need to maintain the old semantics of getExtensions and setExtensions which differentiated between returning null and returning an empty map.

New field in 4.0, same purpose as example which was already in the list.

MikeEdgar

LGTM, merging to main-4.0

Azquelt force-pushed the schema-model-3.1 branch from a523705 to 5556253 Compare April 9, 2024 15:09

Azquelt commented Apr 9, 2024

View reviewed changes

Azquelt force-pushed the schema-model-3.1 branch 2 times, most recently from 887cef7 to 504806b Compare April 9, 2024 15:47

Azquelt commented Apr 9, 2024

View reviewed changes

MikeEdgar changed the base branch from main to main-4.0 April 10, 2024 11:03

Azquelt mentioned this pull request Apr 16, 2024

Schema annotations 3.1 #1801

Merged

Azquelt marked this pull request as ready for review April 16, 2024 16:00

Azquelt force-pushed the schema-model-3.1 branch from 504806b to 38e9254 Compare April 23, 2024 15:49

Azquelt force-pushed the schema-model-3.1 branch 2 times, most recently from a2a678f to 2769a35 Compare April 25, 2024 14:34

Azquelt force-pushed the schema-model-3.1 branch 2 times, most recently from 6520570 to f3619a9 Compare April 30, 2024 15:12

Azquelt commented Apr 30, 2024

View reviewed changes

Azquelt force-pushed the schema-model-3.1 branch from 3b2c5a3 to 9007961 Compare May 1, 2024 16:54

Azquelt added 4 commits May 1, 2024 17:55

Speed up testParsingBigStaticYamlFile

46b9216

This test took a noticable amount of time and more than 50% was string contatentation building the test file.

Allow setSchemes to be called with null

1f3821b

For consistency with other model setters.

Depend on snapshot API

4bc0a7a

Update MP API dependency version for test data

acf6ab2

Azquelt force-pushed the schema-model-3.1 branch 3 times, most recently from 7ef458c to 2ad30f4 Compare May 2, 2024 13:50

MikeEdgar reviewed May 6, 2024

View reviewed changes

Azquelt force-pushed the schema-model-3.1 branch from 2ad30f4 to 52e6311 Compare May 7, 2024 15:25

Azquelt added 11 commits May 7, 2024 16:28

Remove deprecated Schema.getType and getNullable

47d7aac

Existing code relying on the old semantics of these methods was changed to use static methods on SchemaImpl which preserves the old behavior.

Add support for serializing short and char to JSON

b4b3528

Create IO objects in IOContext

eec5cbd

Make the IOContext class the creator of all IO objects. This makes it possible for the SchemaIO class to get hold of whichever IO objects it needs to serialize generic Constructible objects.

Enable serialization of model objects within schemas

9933519

Safer Schema collection property operations

75f45cc

When adding or removing an item from a schema property that is expected to be a collection or map, ensure that the property actually contains a collection or map before attempting to modify it.

Add Javadoc to JsonIO

69e4f6d

Treat examples as "annotating" the schema model

3d536d7

New field in 4.0, same purpose as example which was already in the list.

Azquelt force-pushed the schema-model-3.1 branch from 52e6311 to 3d536d7 Compare May 7, 2024 15:28

MikeEdgar approved these changes May 7, 2024

View reviewed changes

MikeEdgar merged commit 21a6193 into smallrye:main-4.0 May 7, 2024
2 of 5 checks passed

MikeEdgar added this to the 4.0.0 milestone May 9, 2024

MikeEdgar mentioned this pull request Oct 21, 2024

Map-backed OAS model classes from annotations #2021

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

OAS 3.1 changes for schema model #1793

OAS 3.1 changes for schema model #1793

Azquelt commented Apr 9, 2024

Azquelt commented Apr 9, 2024

Azquelt commented Apr 9, 2024

Azquelt Apr 9, 2024

Azquelt Apr 30, 2024

Azquelt Apr 9, 2024

Azquelt Apr 30, 2024 •

edited

Loading

MikeEdgar commented Apr 10, 2024

MikeEdgar commented Apr 23, 2024

Azquelt commented Apr 25, 2024

Azquelt Apr 30, 2024

Azquelt Apr 30, 2024 •

edited

Loading

Azquelt Apr 30, 2024 •

edited

Loading

MikeEdgar commented May 1, 2024

Azquelt commented May 1, 2024

Azquelt commented May 3, 2024

MikeEdgar left a comment

MikeEdgar May 6, 2024

Azquelt May 7, 2024

MikeEdgar May 6, 2024

Azquelt May 7, 2024

phillip-kruger commented May 7, 2024

MikeEdgar commented May 7, 2024

phillip-kruger commented May 7, 2024

MikeEdgar left a comment

		&& param.getSchema().getType() != null
		&& param.getSchema().getType().contains(SchemaType.STRING)

OAS 3.1 changes for schema model #1793

OAS 3.1 changes for schema model #1793

Conversation

Azquelt commented Apr 9, 2024

Azquelt commented Apr 9, 2024

Azquelt commented Apr 9, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Azquelt Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

MikeEdgar commented Apr 10, 2024

MikeEdgar commented Apr 23, 2024

Azquelt commented Apr 25, 2024

Choose a reason for hiding this comment

Azquelt Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Azquelt Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

MikeEdgar commented May 1, 2024

Azquelt commented May 1, 2024

Azquelt commented May 3, 2024

MikeEdgar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phillip-kruger commented May 7, 2024

MikeEdgar commented May 7, 2024

phillip-kruger commented May 7, 2024

MikeEdgar left a comment

Choose a reason for hiding this comment

Azquelt Apr 30, 2024 •

edited

Loading

Azquelt Apr 30, 2024 •

edited

Loading

Azquelt Apr 30, 2024 •

edited

Loading