ghostdogpr / caliban

Functional GraphQL library for Scala
https://ghostdogpr.github.io/caliban/
Apache License 2.0
947 stars 248 forks source link

interfaces' field description derived incorrectly #1951

Closed satorg closed 1 year ago

satorg commented 1 year ago

Consider a snippet:

object InterfaceDescriptionApp extends ZIOAppDefault {
  @GQLInterface
  sealed trait Pet { @GQLDescription("pet name") def name: String }
  case class Cat(@GQLDescription("cat name") name: String) extends Pet
  case class Dog(@GQLDescription("dog name") name: String) extends Pet

  final case class Query(pets: List[Pet])

  override def run: Task[Unit] =
    Console.printLine(render(Schema.gen[Any, Query]))
}

Expected output:

interface Pet {
  "pet name"
  name: String!
}

type Cat implements Pet {
  "cat name"
  name: String!
}

type Dog implements Pet {
  "dog name"
  name: String!
}

type Query {
  pets: [Pet!]!
}

Actual output:

interface Pet {
  "cat name" # "pet name" is expected here!
  name: String!
}

type Cat implements Pet {
  "cat name"
  name: String!
}

type Dog implements Pet {
  "dog name"
  name: String!
}

type Query {
  pets: [Pet!]!
}
ghostdogpr commented 1 year ago

Typeclass derivation doesn't give access to def, only case class fields so this is not supported.

The workaround is to make a custom Schema for your interface that can use the auto-generated one but override the description.

satorg commented 1 year ago

@ghostdogpr thank you for the clarification!

Typeclass derivation doesn't give access to def, only case class fields

Do I understand correctly, that it is a restriction imposed by an upstream library (I guess Magnolia or something)?

Even if it cannot be entirely supported, I suppose the experience can be improved quite a bit. Specifically, I'm thinking about two assumptions that IMO should be feasible to implement:

  1. If the same method in every type implementing some interface has the same description, then copy the description into the corresponding method of the interface (because most likely the description was simply copied among all types and their interface all together).

    Source ADT: ```scala @GQLInterface sealed trait Pet { def name: String } case class Cat(@GQLDescription("pet name") name: String) extends Pet case class Dog(@GQLDescription("pet name") name: String) extends Pet ``` Result GraphQL: ```graphql interface Pet { "pet name" # copied from the implementers because they all have the same description name: String! } type Cat implements Pet { "pet name" name: String! } type Dog implements Pet { "pet name" name: String! } ```
  2. If the same method in different types has different descriptions, then discard the description in the corresponding method of their interface.

    Source ADT: ```scala @GQLInterface sealed trait Pet { def name: String } case class Cat(@GQLDescription("cat name") name: String) extends Pet case class Dog(@GQLDescription("dog name") name: String) extends Pet ``` Result GraphQL: ```graphql interface Pet { # no description because different implementers have different descriptions name: String! } type Cat implements Pet { "cat name" name: String! } type Dog implements Pet { "dog name" name: String! } ```

It would not be perfect of course, but I believe it would be way more intuitive to users and could support more real use cases without falling into manual schema adjustments.

Also I feel it would be really helpful to have a new paragraph in the Schemas chapter (e.g. "Known Limitations") where users could read about issues like this one.

ghostdogpr commented 1 year ago

Do I understand correctly, that it is a restriction imposed by an upstream library (I guess Magnolia or something)?

Yep, that's right: Magnolia on Scala 2 and direct typeclass derivation on Scala 3.

I agree with the heuristic proposed, and it should not be hard to implement.