ghostdogpr / caliban

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

Caliban Client: Allow query to not specify every subtype for the Union Type #446

Closed anotherhale closed 2 years ago

anotherhale commented 4 years ago

I would like to be able to write queries for a Union Type and not have to specify every subtype.

Currently when you query a Union Type you must provide all of the subtype implementations or you will get a compile error. If I remove the Pilot.shipName.map(Role.Pilot) from the role query:

    val character = {
      import caliban.client.Client.Character._
      (name ~
        nicknames ~
        origin ~
        role(
          Captain.shipName.map(Role.Captain),
          Engineer.shipName.map(Role.Engineer),
          Mechanic.shipName.map(Role.Mechanic),
        )).mapN(Character)
    }

I get the following compile time error:

Error:(27, 13) not enough arguments for method role: (onCaptain: caliban.client.SelectionBuilder[caliban.client.Client.Captain,A], onEngineer: caliban.client.SelectionBuilder[caliban.client.Client.Engineer,A], onMechanic: caliban.client.SelectionBuilder[caliban.client.Client.Mechanic,A], onPilot: caliban.client.SelectionBuilder[caliban.client.Client.Pilot,A])caliban.client.SelectionBuilder[caliban.client.Client.Character,Option[A]].
Unspecified value parameter onPilot.
        role(

I discussed this with @ghostdogpr and we determined that this is doable by returning the __typename for the case classes that are not specified and returning an Option[A] instead of the A.

ghostdogpr commented 4 years ago

The related code is https://github.com/ghostdogpr/caliban/blob/master/codegen/src/main/scala/caliban/codegen/ClientWriter.scala#L240. I think it would be nice if we kept the existing method (no need to get an Option when you're willing to provide all cases) and add a new one with that behavior, where each parameter would be optional. I'm guessing the new method should return a ChoiceOf(OptionOf(...)) and each union member not provided would be a choice returning None. Let me know if you need help.

anotherhale commented 4 years ago

Actually if I could get some help with this that would be great.

The new reference to the above https://github.com/ghostdogpr/caliban/blob/master/tools/src/main/scala/caliban/tools/ClientWriter.scala#L243

First to make sure we are on the same page this is how I envision this working. For union type queries you should be able to specify only the results that you want. The following examples are based on the Caliban example data:

(You can use this query with graphiQL to see the result)

{
  characters {
    name
    role {
      ... on Engineer {
        shipName
      }
    }
  }
}

This would result in the following GraphQL response:

{
  "data": {
    "characters": [
      {
        "name": "Naomi Nagata",
        "role": {
          "shipName": "Rocinante"
        }
      },
      {
        "name": "Amos Burton",
        "role": {}
      },
      {
        "name": "Alex Kamal",
        "role": {}
      },
      {
        "name": "Chrisjen Avasarala",
        "role": null
      },
      {
        "name": "Josephus Miller",
        "role": null
      },
      {
        "name": "Roberta Draper",
        "role": null
      }
    ]
  }

Because the query only specified the shipName property the results that returned an empty role result or a null result should be decoded as None:

List(
  Character(Naomi Nagata, None),
  Character(Amos Burton,Some(Mechanic(Some(Rocinante)))),
  Character(Alex Kamal,None),
  Character(Chrisjen Avasarala,None),
  Character(Josephus Miller,None),
  Character(Roberta Draper,None)
)

The Client Generated code for this query could look like this:

  type Character
  object Character {
    def name: SelectionBuilder[Character, String] = Field("name", Scalar())
    def role[A](
        onCaptain: Option[SelectionBuilder[Captain, A]] = None,
        onEngineer: Option[SelectionBuilder[Engineer, A]] = None,
        onMechanic: Option[SelectionBuilder[Mechanic, A]] = None,
        onPilot: Option[SelectionBuilder[Pilot, A]] = None
    ): SelectionBuilder[Character, Option[Option[A]]] =
      Field(
        "role",
        OptionOf(
          ChoiceOf(
            Map(
              "Captain" -> OptionOf(
                Obj(onCaptain)
              ),
              "Engineer" -> OptionOf(
                Obj(onEngineer)
              ),
              "Mechanic" -> OptionOf(
                Obj(onMechanic)
              ),
              "Pilot"    -> OptionOf(
                Obj(onPilot)
              )
            )
          )
        )
      )
  }

I don't have access to my computer that I was working this on to remember some of the challenges with this approach. I do remember that the overridden methods fromGraphQL and toSelectionSet did not match the def signature. I can work on this if you help point me in the right direction or we architect it out some more.

ghostdogpr commented 4 years ago

How about the following implementation:

def roleOption[A](
  onCaptain: Option[SelectionBuilder[Captain, A]] = None,
  onEngineer: Option[SelectionBuilder[Engineer, A]] = None,
  onMechanic: Option[SelectionBuilder[Mechanic, A]] = None,
  onPilot: Option[SelectionBuilder[Pilot, A]] = None
): SelectionBuilder[Character, Option[Option[A]]] =
  Field(
    "role",
    OptionOf(
      ChoiceOf(
        Map(
          "Captain"  -> onCaptain.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a))),
          "Engineer" -> onEngineer.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a))),
          "Mechanic" -> onMechanic.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a))),
          "Pilot"    -> onPilot.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a)))
        )
      )
    )
  )

This introduces NullField like this:

case object NullField extends FieldBuilder[Option[Nothing]] {
  override def fromGraphQL(value: Value): Either[DecodingError, Option[Nothing]] = Right(None)
  override def toSelectionSet: List[Selection]                                   = Nil
}

One more thing to do is in ChoiceOf#toSelectionSet to remove the members that have an empty selection when forming the GraphQL query.

anotherhale commented 4 years ago

@ghostdogpr Yes that works. I filtered the builderMap in the toSelectionSet with the following code (I added the isNullField helper function:

  case object NullField extends FieldBuilder[Option[Nothing]] {
    override def fromGraphQL(value: Value): Either[DecodingError, Option[Nothing]] = Right(None)
    override def toSelectionSet: List[Selection]                                   = Nil
  }

  def isNullField(p : Any): Boolean = p match {
    case  NullField => true
    case _ => false
  }

  case class ChoiceOf[A](builderMap: Map[String, FieldBuilder[A]]) extends FieldBuilder[A] {
    override def fromGraphQL(value: Value): Either[DecodingError, A] =
      value match {
        case ObjectValue(fields) =>
          for {
            typeNameValue <- fields.find(_._1 == "__typename").map(_._2).toRight(DecodingError("__typename is missing"))
            typeName <- typeNameValue match {
              case StringValue(value) => Right(value)
              case _                  => Left(DecodingError("__typename is not a String"))
            }
            fieldType <- builderMap.get(typeName).toRight(DecodingError(s"type $typeName is unknown"))
            result    <- fieldType.fromGraphQL(value)
          } yield result
        case _ => Left(DecodingError(s"Field $value is not an object"))
      }

    override def toSelectionSet: List[Selection] = {
      val filteredBuilderMap = builderMap.filter((f) => !isNullField(f._2) );
      Selection.Field(None, "__typename", Nil, Nil, Nil, 0) ::
        filteredBuilderMap.map { case (k, v) => { Selection.InlineFragment(k, v.toSelectionSet) }}.toList
    }
  }

If this looks good to you I will try to update the code generator to generate the proper client code.

For the ClientExampleApp I also updated the query to demonstrate named parameters for Role works well.

    val character = {
      import caliban.client.Client.Character._
      (name ~
        nicknames ~
        origin ~
        role(
          onEngineer = Some(Engineer.shipName.map(Role.Engineer)),
          onPilot = Some(Pilot.shipName.map(Role.Pilot))
        )
      ).mapN(Character)
    }
ghostdogpr commented 4 years ago

@anotherhale that looks good! About the code generation, should it be an additional function or replace the existing one? I feel that the existing one will be nicer when you want to specify all cases because you don't need to wrap the selection in Some(...) so I tend to prefer having both. What do you think?

anotherhale commented 4 years ago

I agree it would be good to have a different function. I do feel like ChoiceOf would be a better name for this function and the function that requires all of the union types would be AllOf. I understand that is a change in API though, so if you want to keep ChoiceOf the same and maybe name the new function AnyOf or something similar that works too.

ghostdogpr commented 4 years ago

@anotherhale I think both functions can use ChoiceOf no?

anotherhale commented 4 years ago

Yes. I see what you mean now. The code would just generate both a role function and a roleOption function?

type Character
  object Character {
    def name: SelectionBuilder[Character, String]            = Field("name", Scalar())
    def nicknames: SelectionBuilder[Character, List[String]] = Field("nicknames", ListOf(Scalar()))
    def origin: SelectionBuilder[Character, Origin]          = Field("origin", Scalar())
    def role[A](
      onCaptain: SelectionBuilder[Captain, A],
      onEngineer: SelectionBuilder[Engineer, A],
      onMechanic: SelectionBuilder[Mechanic, A],
      onPilot: SelectionBuilder[Pilot, A]
    ): SelectionBuilder[Character, Option[A]] =
      Field(
        "role",
        OptionOf(
          ChoiceOf(
            Map(
              "Captain"  -> Obj(onCaptain),
              "Engineer" -> Obj(onEngineer),
              "Mechanic" -> Obj(onMechanic),
              "Pilot"    -> Obj(onPilot)
            )
          )
        )
      )
    def roleOption[A](
                 onCaptain: Option[SelectionBuilder[Captain, A]] = None,
                 onEngineer: Option[SelectionBuilder[Engineer, A]] = None,
                 onMechanic: Option[SelectionBuilder[Mechanic, A]] = None,
                 onPilot: Option[SelectionBuilder[Pilot, A]] = None
               ): SelectionBuilder[Character, Option[Option[A]]] =
      Field(
        "role",
        OptionOf(
          ChoiceOf(
            Map(
              "Captain"  -> onCaptain.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a))),
              "Engineer" -> onEngineer.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a))),
              "Mechanic" -> onMechanic.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a))),
              "Pilot"    -> onPilot.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a)))
            )
          )
        )
      )
  }

And then the query would could use either one like so: For the role:

    val character = {
      import caliban.client.Client.Character._
      (name ~
        nicknames ~
        origin ~
        role(
          Captain.shipName.map(Role.Captain),
          Engineer.shipName.map(Role.Engineer),
          Mechanic.shipName.map(Role.Mechanic),
          Pilot.shipName.map(Role.Pilot)
        )).mapN(Character)
    }

Or the roleOption:

    val character = {
      import caliban.client.Client.Character._
      (name ~
        nicknames ~
        origin ~
        roleOption(
          onEngineer = Some(Engineer.shipName.map(Role.Engineer))
        )).mapN(Character)
    }
ghostdogpr commented 4 years ago

Yep, that's what I was suggesting šŸ‘

anotherhale commented 4 years ago

This is the change that I made to the ClientWriter to generate the roleOption type.

      } else if (unionTypes.nonEmpty) {
        (
          s"[$typeLetter]",
          s"(${unionTypes.map(t => s"""on${t.name}: Option[SelectionBuilder[${t.name}, $typeLetter]] = None""").mkString(", ")})",
          s"Option[${writeType(field.ofType).replace(fieldType, typeLetter)}]",
          writeTypeBuilder(
            field.ofType,
            s"ChoiceOf(Map(${unionTypes.map(t => s""""${t.name}" -> on${t.name}.fold[FieldBuilder[Option[A]]](NullField)(a => OptionOf(Obj(a)))""").mkString(", ")}))"
          )
        )

https://github.com/ghostdogpr/caliban/blob/master/tools/src/main/scala/caliban/tools/ClientWriter.scala#L250-L260

I did not see how to generate both without creating another TypeDefinition or passing a flag to generate this optional type instead. Do you have any ideas?

ghostdogpr commented 4 years ago

Yeah I see the problem. Maybe add an intermediate function before writeField is called, and there you call writeField twice with different parameters?

anotherhale commented 4 years ago

Sorry been busy with work and life. I will give this a try this week.

anotherhale commented 4 years ago

I have PR that I can push soon. I just need to get it approved for release by my company first.

ghostdogpr commented 4 years ago

@anotherhale great! šŸ˜„

askmrsinh commented 3 years ago

@anotherhale Hi, are you still waiting on the approval?

anotherhale commented 3 years ago

Sorry finally making some progress. The wheels of bureaucracy turn slowly... I will post the PR once approved.

askmrsinh commented 3 years ago

@anotherhale I am afraid that the wheels of bureaucracy might have fallen off. Would you mind if I take a shot at this?

anotherhale commented 3 years ago

I will try to see if I can expedite it.

anotherhale commented 3 years ago

I have it all completed with docs and examples too. Everyone is on vacation for the holidays - probably won't see anything until January.

anotherhale commented 3 years ago

I just got approval on releasing this. I will open a PR as soon as I merge in latest changes and resolve any conflicts.

anotherhale commented 3 years ago

Sorry this took so long. This request to contribute to open source was a pioneer effort in my organization and now they have put in place policies and procedures to make contributing back to the open source community much easier and quicker. :+1:

ghostdogpr commented 3 years ago

Great to hear šŸ‘

askmrsinh commented 3 years ago

This is great news. Thank you!