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

interfaces' field description derived incorrectly #1951

Closed
satorg opened this issue Oct 20, 2023 · 3 comments · Fixed by #1976
Closed

interfaces' field description derived incorrectly #1951

satorg opened this issue Oct 20, 2023 · 3 comments · Fixed by #1976
Labels
good first issue Good for newcomers server Issue related to caliban server

Comments

@satorg
Copy link
Contributor

satorg commented Oct 20, 2023

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

ghostdogpr commented Oct 21, 2023

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
Copy link
Contributor Author

satorg commented Oct 21, 2023

@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:

    @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:

    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:

    @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:

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

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.

@ghostdogpr ghostdogpr added good first issue Good for newcomers server Issue related to caliban server core labels Oct 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers server Issue related to caliban server
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants