tapir icon indicating copy to clipboard operation
tapir copied to clipboard

Add a flag into `TSchemaToASchema` to distinguish between `mappings` or `consts` for discriminators.

Open kamilkloch opened this issue 1 year ago • 4 comments

Follow-up on https://github.com/softwaremill/tapir/issues/3764#issuecomment-2109808803_

kamilkloch avatar May 14 '24 10:05 kamilkloch

We spent some time trying to fix rendering of coproducts in async api, with no success so far.

Looking at TapirSchemaToJsonSchema, idea was to add sth like useDiscriminator: Boolean parameter, that would avoid rendering discriminator specificication, and add const to product schemas instead.

Note how this method works - it processes TSchema producing flat list of all nested schemas, and then iteratively does conversion.

First step works with TSchema instances. From SCoproduct we might extract extract discriminant value and attach it to specific Product schema, but TSchema it does not support const attribute, so there is no place to store it.

And if trying to do it at second step - its already too late - when converting Product it seems impossible to refer to owning Coproduct - this link was already lost when flattening.

It should be possible to implement this behavior in TapirSchemaToJsonSchema, but it probably will require bit more work to carry additional context information or somehow reorganize this method.

And finally just some thought. Looking at schema for the simple data model

sealed trait Fruit
case class Apple(weight: Double, isSweet: Boolean) extends Fruit
case class Plum(weight: Double) extends Fruit

TSchema (here pasted as json schema, but it is internally quite close) is:

{
  "$schema" : "http://json-schema.org/draft-04/schema#",
  "$defs" : {
    "Apple" : {
      "title" : "Apple",
      "type" : "object",
      "required" : [
        "weight",
        "isSweet",
        "name"
      ],
      "properties" : {
        "weight" : {
          "type" : "number",
          "format" : "double"
        },
        "isSweet" : {
          "type" : "boolean"
        },
        "name" : {
          "type" : "string"
        }
      }
    },
    "Plum" : {
      "title" : "Plum",
      "type" : "object",
      "required" : [
        "weight",
        "name"
      ],
      "properties" : {
        "weight" : {
          "type" : "number",
          "format" : "double"
        },
        "name" : {
          "type" : "string"
        }
      }
    }
  },
  "title" : "Fruit",
  "oneOf" : [
    {
      "$ref" : "#/$defs/Apple"
    },
    {
      "$ref" : "#/$defs/Plum"
    }
  ],
  "discriminator" : {
    "propertyName" : "name",
    "mapping" : {
      "apple" : "#/$defs/Apple",
      "plum" : "#/$defs/Plum"
    }
  }
}

Look at the Apple schema. Note this is not schema of Apple itself. This is schema of Apple in context of Fruit coproduct. It contains name property, that is needed only for apples stored next to other fruits. But note this schema alone is incomplete. All apples in the fruits basket should have {"name": "apple"} property set. Apple that has any other name is invalid. Shouldn't Apple schema not only define the name property, but also restrict it to one and only one possible value?

marcin

zorba128 avatar May 14 '24 15:05 zorba128

That's the way inheritance is presented in the OpenAPI docs: https://swagger.io/docs/specification/data-models/inheritance-and-polymorphism/

You might be right that the approach using const would be more precise, however what would have to be checked first is how e.g. swagger ui or rdoc handle such schemas - are they properly rendered.

Btw. if Apple is used both as part of a coproduct (where it needs to include the additional field), and stand-alone, you'd get two schemas (inventively named Apple and Apple1 ;) )

adamw avatar May 14 '24 18:05 adamw

Btw. each sttp.tapir.Schema has attributes - a typed map where you can put anything. It's used in a couple of places to add functionality without breaking bincompat

adamw avatar May 14 '24 19:05 adamw

Yes, trying to add const we got stuck with compatibility (and IntelliJ problems when working with Tapir sources...).

I have plan to implement TapirSchemaToJsonSchema with this additional useDiscriminator using tapir's schema with const added (but not binary compatible) - just to prove it will work.

I looked at attributes, but proper type for Schema[T].const is like Option[(T, Option[Any])] - as with examples/default values. This T will probably have to be erased if attribute is to be used.

And for sample from openapi - maybe I'll try to discuss there. To be honest I don't quite get what this whole discriminant thing is for - if oneOf + const is perfectly enough to represent this case. And it would then make three representations of polymorphism possible in same way:

  • with product nested in additional property: {"plum":{"weight":1.0}}
  • with additional discriminator field bound to constant value: {"name": "plum", "weight":1.0}
  • with all possible products having incompatible schemas - no discriminant/nesting is needed at all: {"weight":1.0}, {"currantColor":"red"}

All those boil up to having specialized schemas that are incompatible with each other (what either happens naturally, or requires some additional work like additional nesting/discriminator), and oneOf on top of that. And specification is local - it is enough to know Apple schema to produce valid apple product. Versus approach with discriminant, where one needs schemas both for Apple and its parent Fruit just to know where to put discriminant value (or - even more important - what value to use).

Maybe this all is bit too naive - I have experience with just few apis we have developed and tried to describe.

zorba128 avatar May 14 '24 21:05 zorba128