Map-backed OAS model classes from annotations

[MikeEdgar]

This PR replaces the current *Impl model classes with a set of map-backed models generated from annotations (private to the project). This brings a few benefits -

• All model classes map-backed, not just Schema (map-backed since OAS 3.1 changes for schema model #1793)
• Simplifies reading and writing. The model can generically be walked and written to JSON/YAML, likewise for reading. This resulted in the majority of JSON I/O methods being removed with this change. The pattern can likely be extended to much of the annotation reading in a future PR.
• Allows for better preservation of the ordering of properties in a user's static file, when provided.

The bulk of the change is the changing of the Impl model classes to extend the new models, deprecating the old models, and replacing existing instances of new XyzImpl() with OASFactory.createXyz(). A new model module is added along with a tools/model-apt module to contain the base model and APT processor respectively.

This currently depends on a SNAPSHOT build of the TCK and can be merged once version 4.0.2 of the spec is released.

[phillip-kruger]

@MikeEdgar Let me know when you ready. I thin we need to get this in and roll forward it there are any issues

[sonarqubecloud]

Quality Gate passed

Issues
38 New issues
0 Accepted issues

Measures
0 Security Hotspots
90.5% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarCloud

[phillip-kruger]

Let's go !

[Azquelt]

I think we need to check the node type before casting to (O) here. Not sure what we do want if it turns out node is a value or array, just fall through to fromJson(node)?

I think this is the cause of this exception (though I haven't yet worked out what in my test prompted it):

java.lang.ClassCastException: com.fasterxml.jackson.databind.node.NullNode incompatible with com.fasterxml.jackson.databind.node.ObjectNode
	at io.smallrye.openapi.runtime.io.JacksonJsonIO.properties(JacksonJsonIO.java:33)
	at io.smallrye.openapi.runtime.io.ModelIO.readObject(ModelIO.java:262)
	at io.smallrye.openapi.runtime.io.ModelIO.readValue(ModelIO.java:376) <-- I assume `node` is a `NullNode` at this point
	at io.smallrye.openapi.runtime.io.ModelIO.readJson(ModelIO.java:340)
	at io.smallrye.openapi.runtime.io.ModelIO.readJson(ModelIO.java:309)
	at io.smallrye.openapi.runtime.io.ModelIO.readObject(ModelIO.java:272)
	at io.smallrye.openapi.runtime.io.ModelIO.readValue(ModelIO.java:376)
	at io.smallrye.openapi.runtime.io.ModelIO.readJson(ModelIO.java:316)
	at io.smallrye.openapi.runtime.io.ModelIO.readObject(ModelIO.java:272)
	at io.smallrye.openapi.runtime.io.OpenAPIDefinitionIO.readObject(OpenAPIDefinitionIO.java:58)
	at io.smallrye.openapi.runtime.io.OpenAPIDefinitionIO.readObject(OpenAPIDefinitionIO.java:12)
	at java.base/java.util.Optional.map(Optional.java:260)
	at io.smallrye.openapi.runtime.io.ModelIO.readValue(ModelIO.java:390)

[MikeEdgar]

I was able to reproduce it also with this input.

---
openapi: 3.1.0
info:
  title: My Title
  version: "1.0.0"
  contact: null
paths: {}

Maybe something like this to avoid it?

if (Constructible.class.isAssignableFrom(desiredType)) {
    if (jsonIO().isObject(node)) {
        return readObject((Class<? extends Constructible>) desiredType, (O) node);
    } else if (jsonIO().isNull(node)) { // new `isNull` method
        return null;
    } else {
        // Log that value type cannot be read as a desiredType.getName(); 
        return null;
    }
}

[Azquelt]

That would work.

My initial thought was

        if (Constructible.class.isAssignableFrom(desiredType) && jsonIO().isObject(node)) {
            return readObject((Class<? extends Constructible>) desiredType, (O) node);
        }

and allow unexpected types to fall through to jsonIO().fromJson(node), but that causes unexpected types to end up in the model.

Dropping the unexpected type on read would match the old IO objects, though SchemaIO allowed unexpected types in and did a type check in its getter methods. It looks like with this PR that SchemaIO no longer does that which I'd have thought would have caused tests to fail so I might be missing something there.

[MikeEdgar]

I would also expect a failure if invalid types were allowed in before. Ultimately they should be written back out as long as the version is 3.1.0. Let me know what you find on that. I suppose it should be OK unless they've set an alternate schema dialect, in which case everything should pass through as-is.

[Azquelt]

I think this generates a call to BaseExtensibleModel.getProperty(String) which does not do any type checking (vs getProperty(String, Class<?>) which returns null if the property is not the right type.

I guess that's ok as long as we're certain that what's in the map is valid (e.g. we validate it on read). What's the overall strategy for ensuring we don't get ClassCastExceptions at runtime if the user feeds us an invalid openapi.yaml?

[MikeEdgar]

Generally, we should be protected from invalid input by the expected types declared via the DataType instances. The generated SmallRyeOASModels class contains a map of all Constructibles as keys, and values containing the known properties and their DataTypes. It ends up being similar to the logic that was in SchemaIO.

[Azquelt]

For Schema we did two things:

1. In SchemaIO when reading, try to read the JSON as the desired type
   • fall back to reading it as a basic type if it can't be read as a desired type
2. In SchemaImpl when getting a value, check if it's the correct type before returning it
   • if it's not the correct type, return null
   • The actual value could still be obtained with Schema.get(String)

I think this part is doing the getting, and unlike SchemaImpl, it isn't checking the type before returning the value, resulting in a ClassCastException when Java checks the return type of the generated getter method against the value being returned if it's not of the expected type.

[Azquelt]

I think we can either implement the same logic in the getters as SchemaImpl did, or ensure that any invalid data is dropped when reading as I think the old IO classes would do.

[MikeEdgar]

I think +1 to dropping invalid data in IO as well as the type checking on the getters. I think we are close to that in the current IO code, with the exception of the class cast bug you noted above. I'll put up a PR with the changes.

[Azquelt]

This change means that some methods which could previously return null will instead return an empty map.

• APIResponses.getAPIResponses()
• Content.getMediaTypes()
• Callback.getPathItems()
• Paths.getPathItems()
• SecurityRequirement.getSchemes()

I'm unsure if this is worth worrying about, but it is probably worth noting as a change in behaviour since 3.x. It did require changes in a couple of places in OpenLiberty.

[MikeEdgar]

Right, these objects are quasi-maps themselves so non-null seems to make sense there if the map returned is a map form of the object itself.

[Azquelt]

A similar change that I don't think is important to fix but we should note: some model classes used to extend Map and now don't. This changed behaviour in one place in OpenLiberty where we were reflecting on an object to check whether it was a map.

[Azquelt]

With the removal of this readObject, the call will now go to ModelIO.readObject which will throw an UnsupportedOperationException.

It looks like only OpenAPIDefinitionIO still has readObject implemented which delegates to ModelIO.readObject(Class, O). I assume that one was kept to keep backwards compatibility?

OpenLiberty was previously using InfoIO.readObject to parse an Info object for our configuration. I can change that to InfoIO.readObject(Info.class, node); but I am wondering if that's a reasonable way to parse an arbitrary model object.

[MikeEdgar]

I think that is a reasonable way to parse it. The only thing remaining with special/overridden JSON handling is the SchemaIO.

We'll need to talk through an approach for how a parsing API should look more generally so there can be a stable contract that is exposed for it. I think it's close, with ModelIO.readObject(Class, O) basically having the needed functionality, but there is still some further refactoring to do I think.

[MikeEdgar]

A similar change that I don't think is important to fix but we should note: some model classes used to extend Map and now don't. This changed behaviour in one place in OpenLiberty where we were reflecting on an object to check whether it was a map.

This was an artifact of MP OpenAPI 1.x where several of the interfaces extended Map [1]. Was it a significant change in OpenLiberty to remove that?

[1] eclipse/microprofile-open-api#287

[Azquelt]

Was it a significant change in OpenLiberty to remove that?

No, it was not a problem to remove.