codegen: implement oneOf support #3624
Conversation
|
It was a bit of a faff getting the libs to respect the discriminator arg (or lack thereof). At this point I kinda want to have a test that the autogenerated serdes are doing the right thing with the ADTs... it's not totally obvious to me how to write that test, other than to have a separate testing submodule for each param combo we want to validate. |
| val apiTypes = schema.types.collect { case ref: OpenapiSchemaRef if jsonParamRefs.contains(ref.stripped) => ref } | ||
| if (apiTypes.nonEmpty) | ||
| throw new NotImplementedError( | ||
| s"A class cannot be used both in a oneOf at the top level when using jsoniter serdes at $name (problematic children: ${apiTypes})" |
There was a problem hiding this comment.
So the problem here is that if there's a serde declared for a subtype of the ADT, then the discriminator is not added on serialization via the parent trait (I guess because jsoniter sees the subtype serde and goes 'cool I'll use that one to serialise'). I don't know how to get around this. Or to test for it. I might open up a pr to set up some downstream test modules that use the code gerated here to serialise and stuff, 'cause how else do you even test this?
|
Will resolve the merge conflicts in a few mins |
| | case (Some(v), _) => Some(v) | ||
| | case (None, next) => | ||
| | scala.util.Try(next.asInstanceOf[$jsoniterPkgCore.JsonValueCodec[$name]].decodeValue(in, default)) | ||
| | .fold(_ => { in.rollbackToMark(); None }, Some(_)) |
There was a problem hiding this comment.
This feels clunky and gross and inefficient, but then I suppose that's why it's a good idea to use discriminators.
|
I'm going to stop twiddling with this pr for now. If I need anything else I'll add it to a follow-up... (Excluding any requests for changes, of course) |
|
So notably this won't generate discriminators for the adt schemas. I do have a branch that fixes that, but it requires deriving all the schemas semi-automatically; and, furthermore, splitting the output into even more files under some circumstances because it turns out that you hit the javac class size limit in an irreconcilable way when doing that. I finally have it working though and can push it up here, though, if there's interest... Or I can hold it back for another pr. I'm also looking at fixing, improving and then enabling the plugin tests, but that might take me a while to complete. |
…code attempt on non-discriminated oneOf types
…ted. Add flag to permit disabling check
|
Any comments welcome! Even if it's just 'sorry, probably won't get to this for a while' or something. This would nearly meet my needs for a 10yo application so would be great to get over the line (I say nearly. Would still need a couple of tiny things around file uploads and whatnot...) |
There was a problem hiding this comment.
Thanks! A few comments, but nothing major. Impressive work, especially around handling non-discriminated values. I would be even OK with leaving such cases aside as unsupported (initially), but it's great you've put the effort into supporting it where possible.
openapi-codegen/cli/src/main/scala/sttp/tapir/codegen/GenScala.scala
Outdated
Show resolved
Hide resolved
openapi-codegen/core/src/main/scala/sttp/tapir/codegen/ClassDefinitionGenerator.scala
Outdated
Show resolved
Hide resolved
openapi-codegen/core/src/main/scala/sttp/tapir/codegen/ClassDefinitionGenerator.scala
Outdated
Show resolved
Hide resolved
openapi-codegen/core/src/main/scala/sttp/tapir/codegen/ClassDefinitionGenerator.scala
Outdated
Show resolved
Hide resolved
openapi-codegen/core/src/main/scala/sttp/tapir/codegen/JsonSerdeGenerator.scala
Outdated
Show resolved
Hide resolved
|
Thanks! I'll address these comments in a the next couple of days. Edit: addressed comments |
|
@kciesielski @adamw I know you're looking at the netty-cats pr at the moment but do you reckon you can squeeze this one into the next release? Would be awesome for me because I'm actually using this stuff, but also I think probably just quite nice for the codegen in general. |
|
@hughsimpson sure, once this gets merged we'll do a release :) I'll leave the review/merging to @kciesielski, though |
|
LGTM, happy to merge. Looking forward to #3647 to get some wider test coverage. |
Supports
oneOfschemas in codegen.For both circe and jsoniter, when a
oneOfschema has no discriminator, on decode we iterate over the subtype schemas in their declared order and stop on the first successfully decoded type. Obviously this means that it's possible to roundtrip an object to a different type if the serialised form can be validly decoded to multiple different subtypes, so I've added in a check that the models can be safely disambiguated this way (the check can be disabled withopenapiValidateNonDiscriminatedOneOfs := falsein case of false positives or if you want to live like a cowboy). This falling-back method is probably not the most efficient implementation, but writing a more efficient one generically is beyond me.For jsoniter-scala support this requires the serdes to go into a separate file to the case class defns if oneOf is used (because macros I guess), so this pr splits all json serdes out into a new
${openapiObject}JsonSerdesfile;this in turn requires that the routes cannot be declared in the same object, so the codegen will throw an error if any endpoints are declared in the base object and the schemas contain a oneOf defn and the serde lib of choice is jsoniter.((actually this check turned out to be excessive, and it's perfectly legal to just define the serdes into another file and import them back into the base object.))Another remaining error condition is if we have an ADT Foo with a discriminator and a subtype Bar, Bar is used at the 'top level' as a req or resp JSON body, and the JSON lib is jsoniter-scala. Comment on throw site, but basically I dunno how to support that .
This also splits the codec definitions out of the object declarations, so that we can retain a bit more consistency between the treatment of the various cases (i.e.
fooJsonDecoder/fooJsonCodecwill now live on its own inside the ${base}JsonSerdes object, rather than in theFoocompanion object). Except for enums when using circe. Serdes are still configured directly on those.