Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
kamilkloch opened this issue May 14, 2024 · 4 comments

Comments

@kamilkloch
Copy link
Contributor

Follow-up on #3764 (comment)_

@zorba128
Copy link

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

@adamw
Copy link
Member

adamw commented May 14, 2024

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
Copy link
Member

adamw commented May 14, 2024

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

@zorba128
Copy link

zorba128 commented May 14, 2024

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants