guardrail icon indicating copy to clipboard operation
guardrail copied to clipboard

Add support for `oneOf` in openapi3 documents

Open sledorze opened this issue 5 years ago • 12 comments

It fails while trying to access getAllOf which is null (tested with version 0.44.0)

        m.getAllOf.asScala.toList.get(1).flatMap {

OpenApi doc:

  "openapi": "3.0.0",
  "info": {
    "title": "---",
    "version": "0.1.0"
  },
  "components": {
    "schemas": {
      "TypeB": {
        "type": "object",
        "properties": {
          "type": {
            "type": "string"
          }
        },
        "description": "TypeB",
        "required": [
          "type"
        ]
      },
      "TypeA": {
        "type": "object",
        "properties": {
          "type": {
            "type": "string"
          }
        },
        "description": "TypeA",
        "required": [
          "type"
        ]
      },
      "Foo": {
        "type": "object",
        "description": "Foo",
        "oneOf": [
          {
            "$ref": "#/components/schemas/TypeA"
          },
          {
            "$ref": "#/components/schemas/TypeB"
          }
        ]
      }
    }
  },
  "paths": {
    "/foobar": {
      "get": {
        "operationId": "getFoo",
        "responses": {
          "200": {
            "description": "Response 200: FOO",
            "content": {
              "application/json": {
                "schema": {
                  "$ref": "#/components/schemas/Foo"
                }
              }
            }
          }
        }
      }
    }
  }
}

However, it does not fail if the oneOf is surrounded by a allOf, like so:

      "Foo": {
        "type": "object",
        "description": "Foo",
        "allOf": [{
          "type": "object",
          "oneOf": [
            {
              "$ref": "#/components/schemas/TypeA"
            },
            {
              "$ref": "#/components/schemas/TypeB"
            }
          ]
        }]
      }

sledorze avatar Mar 06 '19 16:03 sledorze

Thanks for the report! We currently don't have support for oneOf, only allOf.

That being said, I suspect most of the allOf infrastructure could be generalized to support the oneOf case, likely by generating a sealed trait representing all options withAnyVal members

blast-hardcheese avatar Mar 06 '19 17:03 blast-hardcheese

@blast-hardcheese thanks for the feedback, I thought it was supported.

sledorze avatar Mar 06 '19 20:03 sledorze

I run into the same issue, one note, the wrapping in allOf only works in that it does not end with an NPE, the generated code is incorrect, repeating arguments in the generated case class if there are repeated names in the "oneOf" member schemas.

Are you planning to put this enhancement in the backlog of the project?

jmpicnic avatar May 28 '19 18:05 jmpicnic

@jmpicnic Pull requests in this direction are entirely welcome! Other than that, I can't promise when it'll be triaged by us.

If you're interested in contributing, I'd love to brainstorm some directions we can take!

blast-hardcheese avatar Jun 05 '19 19:06 blast-hardcheese

We would like to add support for oneOf in the best case via sealed types so the generated types can be used for exhaustive pattern matching.

This is our current plan. Given this schema:

openapi: 3.0.0
info:
  description: "This is a sample server Petstore server.  You can find out more about     Swagger at [http://swagger.io](http://swagger.io) or on [irc.freenode.net, #swagger](http://swagger.io/irc/).      For this sample, you can use the api key `special-key` to test the authorization     filters."
  version: "1.0.0"
  title: "Swagger Petstore"
components:
  schemas:
    Pet:
      oneOf:
        - $ref: '#/components/schemas/Dog'
        - $ref: '#/components/schemas/Cat'
      discriminator:
        propertyName: case
        mapping:
          dog: '#/components/schemas/Dog'
          cat: '#/components/schemas/Cat'
    Dog:
      type: object
      properties:
        bark:
          type: boolean
        breed:
          type: string
          enum: [Dingo, Husky, Retriever, Shepherd]
    Cat:
      type: object
      properties:
        hunts:
          type: boolean
        age:
          type: integer
paths:
  /pet/{id}:
    post:
      summary: "Add a new pet to the store"
      description: ""
      operationId: "addPet"
      parameters:
      - name: id
        in: path
        description: "Pet object that needs to be added to the store"
        required: true
        schema:
          $ref: "#/components/schemas/Pet"
      responses:
        405:
          description: "Invalid input"

We would like to produce:

/*
 * This file was generated by Guardrail (https://github.com/twilio/guardrail).
 * Modifications will be overwritten; instead edit the OpenAPI/Swagger spec file.
 */
package swagger.definitions
import io.circe._
import io.circe.syntax._
import io.circe.generic.semiauto._
import cats.syntax.either._
sealed trait Pet
object Pet {
  val discriminator: String = "case"
  implicit val encoder: Encoder[Pet] = Encoder.instance({
    case e: Dog =>
      e.asJsonObject.add(discriminator, "Dog".asJson).asJson
    case e: Cat =>
      e.asJsonObject.add(discriminator, "Cat".asJson).asJson
  })
  implicit val decoder: Decoder[Pet] = Decoder.instance { c => 
    val discriminatorCursor = c.downField(discriminator)
    discriminatorCursor.as[String].flatMap({
      case "Dog" =>
        c.as[Dog]
      case "Cat" =>
        c.as[Cat]
      case tpe =>
        Left(DecodingFailure("Unknown value " ++ tpe ++ " (valid: Dog, Cat)", discriminatorCursor.history))
    })
  }
}
​
case class Cat(hunts: Option[Boolean] = None, age: Option[BigInt] = None) extends Pet
object Cat {
  implicit val encodeCat: ObjectEncoder[Cat] = {
    val readOnlyKeys = Set[String]()
    Encoder.forProduct2("hunts", "age") { (o: Cat) => (o.hunts, o.age) }.mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
  }
  implicit val decodeCat: Decoder[Cat] = Decoder.forProduct2("hunts", "age")(Cat.apply _)
}
​
case class Dog(bark: Option[Boolean] = None, breed: Option[String] = None) extends Pet
object Dog {
  implicit val encodeDog: ObjectEncoder[Dog] = {
    val readOnlyKeys = Set[String]()
    Encoder.forProduct2("bark", "breed") { (o: Dog) => (o.bark, o.breed) }.mapJsonObject(_.filterKeys(key => !(readOnlyKeys contains key)))
  }
  implicit val decodeDog: Decoder[Dog] = Decoder.forProduct2("bark", "breed")(Dog.apply _)
}

All necessery pieces seem to be already in place, we only have to add a sealed modifier on the base type & put all subtypes into the same file.

If a case appears in multiple oneOf statements, all super types including their sub types should be rendered into the same file. Having this makes it possible to even support the rare cases where multiple inheritance of sealed traits is needed.

Does sound reasonable, any hints, suggestions or pointers?

realvictorprm avatar Oct 09 '19 15:10 realvictorprm

@realvictorprm This sounds very reasonable. The one challenge is going to be coming up with a good way to model "all super types including their sub types should be rendered into the same file".

The strategy we come up with here in order to do this object graph and relocation could also be used for another issue (Unify identical objects, #77) since if we have this introspective power over the generated structures it may be possible to unify inlined object literals or even structures from different clients and servers, permitting client.getFoo().fold(handleOK = respond.OK(_), ...), which is currently not possible because there are different JVM paths to the objects, despite having the same structure. My assumption was that the only way to implement that generally would be to implement shapeless record datatypes instead of emitting case classes, but I haven't thought too much about the implementation.


As far as first steps towards implementation, we need to consider:

  • Delaying full file render until after required components have been gathered
  • Flexible grouping and ordering infrastructure that isn't too tailored to Scala (ie: The Java impl would put the classes into a file named identically to the only public interface in the file)
  • Using NonEmptyList[<Component>] to ensure we don't end up with empty files

Additional complexity comes from how references currently exist in the namespace of currently generated objects. If possible, it would be great to move towards a uniquely descriptive identifier system that we could use to uniquely track parent-child relationships:

case class Component(id: UniquePointer, defn: Defn, parents: List[UniquePointer])

would likely need to translate into something similar to

val parent: (UniquePointer, FileName, NonEmptyList[(UniquePointer, Defn)])

This is still fairly abstract in my mind currently, but just thinking about where we'd need to go here, this is how I see it. Is this enough to go on for now, or do you have any further clarifying questions?

blast-hardcheese avatar Oct 09 '19 21:10 blast-hardcheese

@blast-hardcheese is there any progress with this ticket? Is there a possibility that I can contribute to it?

jantoebes avatar Aug 13 '20 10:08 jantoebes

@jantoebes I don't think anyone is working on this right now. Not sure if @realvictorprm has had a chance to start on this. If you're game to take a look yourself, that would be awesome!

kelnos avatar Aug 13 '20 18:08 kelnos

Would also like to see this feature added! We're able to hack around it at the moment because it seems like when Guardrail encounters a oneOf in a schema, it gracefully degrades to a definition of io.circe.Json. You can then manually (de)serialize in the handler. It's not my favourite but it seems to work, and most importantly the API users are none the wiser - they just see oneOf in the spec/docs as usual.

At some point I may have time to try and help out here, not sure when though. I agree with the direction proposed above.

dvgica avatar Nov 17 '20 22:11 dvgica

Would also like to see this feature added! We're able to hack around it at the moment because it seems like when Guardrail encounters a oneOf in a schema, it gracefully degrades to a definition of io.circe.Json. You can then manually (de)serialize in the handler. It's not my favourite but it seems to work, and most importantly the API users are none the wiser - they just see oneOf in the spec/docs as usual.

At some point I may have time to try and help out here, not sure when though. I agree with the direction proposed above.

Can you expand on that? :D

Btw I would like this feature added too, it's a shame that I cannot model my datatypes in Scala using GADTs but using a lot of optional fields to manually check for existence or not.

TonioGela avatar Aug 10 '22 14:08 TonioGela

@TonioGela not much to say besides that! The hack only works at the top-level request or response IIRC.

I haven't worked on this at all, and ultimately we moved to Tapir because of this and a few other pain points.

dvgica avatar Aug 10 '22 15:08 dvgica

I haven't worked on this at all, and ultimately we moved to Tapir because of this and a few other pain points.

Tapir doesn't generate code from an open-api definition but generates the definition once you define the endpoints with his DSL as far as I know, so sadly is not an option for me :(

TonioGela avatar Aug 10 '22 15:08 TonioGela

Hi! Any progress on this issue? I can generate code for simple cases, but unfortunately I can't generate code for production specs (we use oneOf a lot to describe tagged unions ) 😞

KineticCookie avatar Sep 24 '22 14:09 KineticCookie

Hi! Any progress on this issue? I can generate code for simple cases, but unfortunately I can't generate code for production specs (we use oneOf a lot to describe tagged unions ) 😞

No progress on this so far, but if all the branches of your oneOf are defined as components you could do a workaround like:

x-scala-type: Either[Either[C, B], A]
oneOf:
  - $ref: .../A
  - $ref: .../B
  - $ref: .../C

which isn't the best, but it could help get unblocked for the time being. You'll need to be sure to order the types sensibly, as if one is able to be represented as a subset of another, you won't be able to decode successfully without discriminator support.

A better solution would be to write something along the lines of:

sealed trait MyOneOf3[+A, +B, +C] {
  def fold[Z](fromA: A => Z, fromB: B => Z, fromC: C => Z): Z // implement this in the branches
}
final case class MyOneOf3A[A](value: A) extends MyOneOf3[A, Nothing, Nothing]
final case class MyOneOf3B[B](value: B) extends MyOneOf3[Nothing, B, Nothing]
final case class MyOneOf3C[C](value: C) extends MyOneOf3[Nothing, Nothing, C]

object MyOneOf3 {
  implicit def encoder[A, B, C](implicit A: Encoder[A], B: Encoder[B], C: Encoder[C]): Encoder[MyOneOf3[A, B, C]] =
    Encoder.instance[MyOneOf3[A, B, C]] {
      case MyOneOf3A(value) => A(value)
      // ... etc
    }
  implicit def decoder[A, B, C](implicit A: Decoder[A], B: Decoder[B], C: Decoder[C]): Decoder[MyOneOf3[A, B, C]] =
    A.map(MyOneOf3A.apply).or(B.map(MyOneOf3B.apply).or(C.map(MyOneOf3C.apply)))
}

... and stick that into a package somewhere, then use the imports configuration parameter in your sbt file like imports=List("com.example.foo.hacks._") which will add that import to all generated files. Then you can improve things like:

x-scala-type: MyOneOf3[A, B, C]
oneOf:
  - $ref: .../A
  - $ref: .../B
  - $ref: .../C

You'd still need to be aware of the ordering, but it would give you better ergonomics for use


To that end, what I have written above is very similar to how the response type mappers are managed, and generating this on-the-fly where we need oneOf support seems pretty reasonable.

One of the hard problems here (one of the reasons this is not currently implemented) is that object literals would need to be represented as nested case classes, with their own codecs and so on. Similarly, the oneOf itself would need to be defined as a child of wherever it is used.

A simplifying first step would be to just restrict oneOf to named components, both in which components it references and that it only occurs in top-level component definitions, which at least gives the required functionality for those that are able to accept this restriction.

blast-hardcheese avatar Sep 26 '22 17:09 blast-hardcheese

Trying to generate a client out of this a spec that has the following among responses

    CreateLinkedClientSuccess:
      description: successful operation
      content:
        application/json:
          schema:
            x-scala-type: Either[CreateLinkedClientSuccessIndividual, CreateLinkedClientSuccessBusiness]
            oneOf:
              - $ref: '#/components/schemas/CreateLinkedClientSuccessBusiness'
              - $ref: '#/components/schemas/CreateLinkedClientSuccessIndividual'

I get

Error:Unknown type for the following structure (No type definition, class: io.swagger.v3.oas.models.media.ComposedSchema, .paths./api/linkedClient.operations.POST.responses.200.content.application/json.schema):
  Tracker(class ComposedSchema {
      class Schema {
      }
      oneOf: [class Schema {
          $ref: #/components/schemas/CreateLinkedClientSuccessBusiness
      }, class Schema {
          $ref: #/components/schemas/CreateLinkedClientSuccessIndividual
      }]
  }, Vector(.paths, ./api/linkedClient, .operations, .POST, .responses, .200, .content, .application/json, .schema))

What am I doing wrong?

ArinRayAtCornerStoneFS avatar Nov 29 '22 15:11 ArinRayAtCornerStoneFS

Sorry for the delay in response.

oneOf is not currently supported natively, though

x-scala-type: Either[CreateLinkedClientSuccessIndividual, CreateLinkedClientSuccessBusiness]

may get you what you need.

blast-hardcheese avatar Dec 24 '22 16:12 blast-hardcheese

Hello 👋

Any news on this? The suggested workaround above doesn't seem to work. I tried:

schemas:
  A:
    type: string
  B:
    type: number
  C:
    x-scala-type: Either[String, Long]
    oneOf:
    - $ref: #/components/schemas/A
    - $ref: #/components/schemas/B

but the definition for C isn't getting generated at all.

sammy-da avatar Jan 26 '23 22:01 sammy-da

I would expect any reference to C to just be replaced with the Either[String, Long], is that not happening?

blast-hardcheese avatar Feb 03 '23 04:02 blast-hardcheese

sadly no, it's not

sammy-da avatar Mar 02 '23 06:03 sammy-da

Hello,

Is there any progress on this issue?

WBR

mikkka avatar Jun 16 '23 12:06 mikkka

Thanks again for this super useful library. Checking in to see what the current thinking on support for oneOf and anyOf is. Prior to full-fledged support, is it possible/desirable to have the generated code fall back to just i.e. io.circe.Json? Would love to assist, just not sure I would know where to begin. Thanks!

djm1329 avatar Nov 14 '23 19:11 djm1329

I've been struggling with this one for a while, unfortunately, due to inherent complexity and modeling. io.circe.Json is exposed and accepted, and I'm leaning towards just using Either[A, Either[B, C]] as the underlying representation.

One of the unsolved problems that could use some assistance is ordering A,B, and C. String or Boolean are simple enough, but case class Foo(a: String) and case class Bar(a: String, b: Long) should attempt Bar before Foo, no matter the ordering in the specfile.

I'll push up a partial branch shortly once I've got the basic plumbing situated, if I've got time to finish it I'm happy to, if somebody else gets to it first then that would be more than appreciated.

blast-hardcheese avatar Nov 15 '23 22:11 blast-hardcheese

@blast-hardcheese Cool, looking forward to trying it, and happy to help test it out.

djm1329 avatar Nov 17 '23 18:11 djm1329

Ok, https://github.com/guardrail-dev/guardrail/pull/1881/ is an extraordinarily rough draft that gets close to where we need to be for this feature.

Concerns raised above pertaining to ordering are important to solve before merge, and also whether a single type representing Foo as an enumeration of the branches would be more ergonomic.

I don't have time to pursue this further at the moment, but the groundwork is done at least. I'm happy to collaborate if someone wants to take this over.

blast-hardcheese avatar Nov 18 '23 06:11 blast-hardcheese

#1881 is now closer. From the following specification,

Spec is basically to support:
oneOf: [TypeA, TypeB, TypeC, TypeD, TypeE]
openapi: 3.0.0
info:
  title: '---'
  version: 0.1.0
components:
  schemas:
    TypeA:
      type: object
      properties: { type: { type: string } }
      required: [ type ]
    TypeB:
      type: object
      properties: { type: { type: string } }
      required: [ type ]
    TypeC:
      type: object
      properties: { type: { type: string } }
      required: [ type ]
    TypeD:
      type: object
      properties: { type: { type: string } }
      required: [ type ]
    TypeE:
      type: object
      properties: { type: { type: string } }
      required: [ type ]
    Foo:
      type: object
      description: Foo
      x-scala-type: Bogus
      oneOf:
        - $ref: '#/components/schemas/TypeD'
        - $ref: '#/components/schemas/TypeA'
        - $ref: '#/components/schemas/TypeB'
        - $ref: '#/components/schemas/TypeC'
        - $ref: '#/components/schemas/TypeE'
paths:
  /foobar:
    get:
      operationId: getFoo
      responses:
        '200':
          description: 'Response 200: FOO'
          content:
            application/json:
              schema:
                $ref: '#/components/schemas/Foo'

I was able to generate the following:

/*
 * This file was generated by guardrail (https://github.com/guardrail-dev/guardrail).
 * Modifications will be overwritten; instead edit the OpenAPI/Swagger spec file.
 */
package issues.issue195.server.akkaHttp.definitions
import cats.syntax.either._
import io.circe.syntax._
import cats.instances.all._
import _root_.issues.issue195.server.akkaHttp.Implicits._
sealed abstract class Foo
object Foo {
  implicit def encodeFoo: _root_.io.circe.Encoder[Foo] = _root_.io.circe.Encoder.instance({
    case members.TypeD(member) =>
      member.asJson
    case members.TypeA(member) =>
      member.asJson
    case members.TypeB(member) =>
      member.asJson
    case members.TypeC(member) =>
      member.asJson
    case members.TypeE(member) =>
      member.asJson
  })
  implicit def decodeFoo: _root_.io.circe.Decoder[Foo] = _root_.io.circe.Decoder[TypeD].map[_root_.issues.issue195.server.akkaHttp.definitions.Foo](members.TypeD.apply _).or(_root_.io.circe.Decoder[TypeA].map[_root_.issues.issue195.server.akkaHttp.definitions.Foo](members.TypeA.apply _)).or(_root_.io.circe.Decoder[TypeB].map[_root_.issues.issue195.server.akkaHttp.definitions.Foo](members.TypeB.apply _)).or(_root_.io.circe.Decoder[TypeC].map[_root_.issues.issue195.server.akkaHttp.definitions.Foo](members.TypeC.apply _)).or(_root_.io.circe.Decoder[TypeE].map[_root_.issues.issue195.server.akkaHttp.definitions.Foo](members.TypeE.apply _))
  object members {
    case class TypeD(value: _root_.issues.issue195.server.akkaHttp.definitions.TypeD) extends Foo
    case class TypeA(value: _root_.issues.issue195.server.akkaHttp.definitions.TypeA) extends Foo
    case class TypeB(value: _root_.issues.issue195.server.akkaHttp.definitions.TypeB) extends Foo
    case class TypeC(value: _root_.issues.issue195.server.akkaHttp.definitions.TypeC) extends Foo
    case class TypeE(value: _root_.issues.issue195.server.akkaHttp.definitions.TypeE) extends Foo
  }
}

No discriminator support currently, but that should be straightforward now that the rest of this is in place. Note the order of attempts to decode matches the order of $ref's in oneOf.

blast-hardcheese avatar Dec 10 '23 07:12 blast-hardcheese

Alright. Found some more time to push this over the line.

oneOf is now supported, including discriminators and mapping. If anyone is interested in giving it a shot,

sbt> runIssue scala akka-http modules/sample/src/main/resources/issues/issue195.yaml

from the guardrail repo. That alias expands to (basically) the following:

sbt> cli scala --defaults --specPath modules/sample/src/main/resources/issues/issue195.yaml --outputPath modules/sample-akkaHttp/target/generated --framework akka-http --client --packageName issues.issue195.client.akkaHttp --server --packageName issues.issue195.server.akkaHttp

... which you can use to point at your own project in case you want to experiment in something more real.

blast-hardcheese avatar Dec 11 '23 07:12 blast-hardcheese

Considering that the original specifications that required allOf have been merged for some time and oneOf has been merged, I'm going to close this issue. If there's any follow-up to what has been merged, please open a new issue.

Thank you.

blast-hardcheese avatar Dec 11 '23 07:12 blast-hardcheese

Thank you!

danielporterda avatar Dec 12 '23 17:12 danielporterda

Indeed, this is awesome . Thank you so much !!! I will try it out within the next few days. I cannot thank you enough! Sent from my iPhoneOn Dec 12, 2023, at 11:49, danielporterda @.***> wrote: Thank you!

—Reply to this email directly, view it on GitHub, or unsubscribe.You are receiving this because you commented.Message ID: @.***>

djm1329 avatar Dec 12 '23 18:12 djm1329

Closing the loop, as communication channels are fractured currently:

1.0.0-SNAPSHOT request for feedback: Tweet, Matrix channel, LinkedIn

If you have capacity to test any of the recent stuff, it would really be appreciated. Thanks to everyone who has helped get us to this point 🙂

blast-hardcheese avatar Dec 14 '23 21:12 blast-hardcheese