codegen: Support a wider range of oneOfs#5354
Conversation
| } | ||
| } | ||
|
|
||
| private def genCirceWrappedOneOfSerde( |
There was a problem hiding this comment.
The circe and zio variants I let an LLM write after I had jsoniter working. I guess it does save time sometimes, once there's a pattern to follow :)
EDIT: Nope, I tell a lie, it ballsed up at least the zio implementation and hid it by not writing a test.
EDIT x2: No actually, that was me, I'm the idiot
EDIT x3: I have no idea if anyone cares which bits are llm and which not, but leaning into honest disclosure as my own personal approach.
| private def canBeDisambiguated(doc: OpenapiDocument, schemaName: String, s: Seq[OpenapiSchemaSimpleType]): Boolean = { | ||
| def bail(msg: String) = throw new RuntimeException(s"Unable to constructing internal representation for oneOf '$schemaName': $msg'") | ||
| val classify: OpenapiSchemaSimpleType => Int = { | ||
| case _: OpenapiSchemaBinary | _: OpenapiSchemaByte => bail("Binary/byte variants not supported on oneOf") |
There was a problem hiding this comment.
This can be implemented later, I ran out of f's, and I'm not sure it's particularly likely to come up outside of things like converting another standard into an openapi representation (I remember FHIR had this sort of ambiguity in theory like for PDF vs plain text on some attachments?)
| case JsonSerdeLib.Zio if !queryParamRefs.contains(name) => s" extends java.lang.Enum[$name]" | ||
| case JsonSerdeLib.Zio => s" extends java.lang.Enum[$name] derives enumextensions.EnumMirror" | ||
| case JsonSerdeLib.Zio if !queryParamRefs.contains(name) => " derives zio.json.JsonCodec" | ||
| case JsonSerdeLib.Zio => " derives zio.json.JsonCodec, enumextensions.EnumMirror" |
There was a problem hiding this comment.
Unrelated but this was failing some tests. Noticed when adding sbt2 support, just remembered about it.
ab6a04b to
607baf3
Compare
| sealed trait FooOrStringOrInt | ||
| case class FooOrStringOrIntWrapA(v: WrapA) extends FooOrStringOrInt | ||
| case class FooOrStringOrIntWrapB(v: WrapB) extends FooOrStringOrInt | ||
| case class FooOrStringOrIntInt(v: Int) extends FooOrStringOrInt |
There was a problem hiding this comment.
This silly double-wrapped non-discriminated oneOf structure is easy to accidentally break in jsoniter parsing, it turns out 😅 Added extra tests there.
|
Annoyingly haven't managed to get a clean CI run yet, but the failure is unrelated to these changes |
|
@hughsimpson yeah sorry about the flaky CI, I re-ran the failing test |
|
Oh wait. There was a failing ah ok, it's a java version thing. Will push a fix. |
260c8f6 to
f45a52c
Compare
|
Sorry about that. It's the test I just enabled, it passed when I ran locally so hadn't considered it as a culprit 🤦 All should be fixed now 🤞 |
|
hurrah, got there in the end |
| object VersionCheck { | ||
| def runTest(jsonSerde: String)(test: => Unit): Unit = if (jsonSerde != "zio") test else () | ||
| def runTest(jsonSerde: String)(test: => Unit): Unit = | ||
| if (jsonSerde == "zio" && Option(System.getProperty("java.version")).exists(_.startsWith("11"))) () |
There was a problem hiding this comment.
This does run against scala 3 + zio on java 21, and passes there, so fine to skip this
|
Ah, not that flake'y after all! (this time ;) ) |
Until now, oneOfs have been handled exclusively for 'object' type children, but that doesn't cover all eventualities... This pr supports derivation of wrappers to handle a wider range of oneOf combinations. A hugely contrived test example of the wrappers is:
and a more realistic one:
Some combinations can't be handled, but disambiguation can often be achieved by having sub-formats come before the parents
EDIT: I think in scala 3 you could probably also define a
def v: T1 | T2 | T3alias in the parent, but that might not actually be useful, idk. I haven't bothered here.