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

Code generation for unions is flawed #102

Open
felixbr opened this issue Aug 16, 2022 · 1 comment
Open

Code generation for unions is flawed #102

felixbr opened this issue Aug 16, 2022 · 1 comment
Labels

Comments

@felixbr
Copy link
Collaborator

felixbr commented Aug 16, 2022

I'll just document this here, sadly I don't have the time to fix it right now.

Version used: 0.16.4

Given the following query:

query Test {
    content {
      __typename

      ... on Foo {
        slug
      }

      ... on Bar {        
        id
        foo {          
          slug
        }
      }
    }
}    

The generated code looks like this (only the relevant part):

object Content {
  case class Foo(__typename: String, slug: String) extends Content
  object Foo {
    implicit val jsonDecoder: Decoder[Foo] = deriveDecoder[Foo]
    implicit val jsonEncoder: Encoder[Foo] = deriveEncoder[Foo]
  }

  case class Bar(__typename: String, id: Int, foo: Content.Foo) extends Content
  object Bar {
    case class Foo(__typename: String, slug: String)
    object Foo {
      implicit val jsonDecoder: Decoder[Foo] = deriveDecoder[Foo]
      implicit val jsonEncoder: Encoder[Foo] = deriveEncoder[Foo]
    }
    implicit val jsonDecoder: Decoder[Bar] = deriveDecoder[Bar]
    implicit val jsonEncoder: Encoder[Bar] = deriveEncoder[Bar]
  }

  implicit val jsonDecoder: Decoder[Content] = for {
    typeDiscriminator <- Decoder[String].prepare(_.downField("__typename"))
    value <- typeDiscriminator match {
      case "Foo" =>
        Decoder[Foo]
      case "Bar" =>
        Decoder[Bar]
      case other =>
        Decoder.failedWithMessage("invalid type: " + other)
    }
  } yield value
  implicit val jsonEncoder: Encoder[Content] = Encoder.instance[Content]({
    case v: Foo =>
      deriveEncoder[Foo].apply(v)
    case v: Bar =>
      deriveEncoder[Bar].apply(v)
  })
}

There are two bugs here:

  1. Foo and Bar have case class fields for __typename, which means that we have to request __typename in the query for both of them or the decoding will fail for no good reason. It would make more sense to generate constants, since we already know the value anyway and it never changes.
// Instead of
case class Foo(__typename: String, slug: String)

// we should generate
case class Foo(slug: String) {
  def __typename: String = "Foo"
}

// or maybe even
case class Foo(slug: String) {
  def __typename: String = Foo.__typename
}
object Foo {
  def __typename: String = "Foo"
}
  1. Inside of Bar we reference Foo for the second union case in the query. In the generated code we have a dedicated Bar.Foo generated but for some dumb reason, we use Content.Foo in Bar and Bar.Foo is unused. This means the whole thing doesn't work if the two cases use different fields from Foo.
    I'm not sure if there is a reason for this (fragments?) or if it's just an honest mistake.

  2. Not really a bug but strange: The implementation for Encoder[Content] derives encoders inline instead of using the existing ones. The equivalent Decoder doesn't do this. Imo we shouldn't derive something inline, especially if it already exists.

@muuki88 muuki88 added the bug label Aug 16, 2022
@muuki88
Copy link
Owner

muuki88 commented Aug 16, 2022

I have spent several hours to get my head around code gen for union types in #79 ... without success so far 😢
The best workaround at this point is to use the codgen directive introduced in #93 .

However we may fail the codegen if __typename is missing in the query document for a union type. Or add it and issue a warning. That at least safe guards for this scenario.

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

No branches or pull requests

2 participants